[2/2] cl: Change data types of char/short buffers in integer limits tests

Submitted by Aaron Watry on Sept. 10, 2015, 3:12 p.m.

Details

Message ID 1441897970-4005-2-git-send-email-awatry@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Aaron Watry Sept. 10, 2015, 3:12 p.m.
The char/short return buffers were declared as ints.

Signed-off-by: Aaron Watry <awatry@gmail.com>
---
 tests/cl/program/execute/int-definitions.cl | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/cl/program/execute/int-definitions.cl b/tests/cl/program/execute/int-definitions.cl
index 3d8ee63..a438fe4 100644
--- a/tests/cl/program/execute/int-definitions.cl
+++ b/tests/cl/program/execute/int-definitions.cl
@@ -12,12 +12,12 @@  global_size: 1 0 0
 [test]
 name: Char Definitions
 kernel_name: test_char
-arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
+arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
 
 [test]
 name: Short Definitions
 kernel_name: test_short
-arg_out: 0 buffer int[3] 32767 -32768 65535
+arg_out: 0 buffer short[3] 32767 -32768 65535
 
 [test]
 name: Int Definitions
@@ -32,7 +32,7 @@  arg_out: 0 buffer long[3] 9223372036854775807 \
                           18446744073709551615
 !*/
 
-kernel void test_char(global int* out) {
+kernel void test_char(global char* out) {
   int i = 0;
   out[i++] = CHAR_BIT;
   out[i++] = CHAR_MAX;
@@ -42,7 +42,7 @@  kernel void test_char(global int* out) {
   out[i++] = UCHAR_MAX;
 }
 
-kernel void test_short(global int* out) {
+kernel void test_short(global short* out) {
   int i = 0;
   out[i++] = SHRT_MAX;
   out[i++] = (SHRT_MIN - (short2)(0)).s0;

Comments

On Thu, 2015-09-10 at 10:12 -0500, Aaron Watry wrote:
> The char/short return buffers were declared as ints.
> 
> Signed-off-by: Aaron Watry <awatry@gmail.com>

Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu>
For both patches.

though, I agree with Serge that a spec reference would be nice.

Jan

> ---
>  tests/cl/program/execute/int-definitions.cl | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/cl/program/execute/int-definitions.cl
> b/tests/cl/program/execute/int-definitions.cl
> index 3d8ee63..a438fe4 100644
> --- a/tests/cl/program/execute/int-definitions.cl
> +++ b/tests/cl/program/execute/int-definitions.cl
> @@ -12,12 +12,12 @@ global_size: 1 0 0
>  [test]
>  name: Char Definitions
>  kernel_name: test_char
> -arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
> +arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
>  
>  [test]
>  name: Short Definitions
>  kernel_name: test_short
> -arg_out: 0 buffer int[3] 32767 -32768 65535
> +arg_out: 0 buffer short[3] 32767 -32768 65535
>  
>  [test]
>  name: Int Definitions
> @@ -32,7 +32,7 @@ arg_out: 0 buffer long[3] 9223372036854775807 \
>                            18446744073709551615
>  !*/
>  
> -kernel void test_char(global int* out) {
> +kernel void test_char(global char* out) {
>    int i = 0;
>    out[i++] = CHAR_BIT;
>    out[i++] = CHAR_MAX;
> @@ -42,7 +42,7 @@ kernel void test_char(global int* out) {
>    out[i++] = UCHAR_MAX;
>  }
>  
> -kernel void test_short(global int* out) {
> +kernel void test_short(global short* out) {
>    int i = 0;
>    out[i++] = SHRT_MAX;
>    out[i++] = (SHRT_MIN - (short2)(0)).s0;
On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely <jan.vesely@rutgers.edu> wrote:

> On Thu, 2015-09-10 at 10:12 -0500, Aaron Watry wrote:
> > The char/short return buffers were declared as ints.
> >
> > Signed-off-by: Aaron Watry <awatry@gmail.com>
>
> Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu>
> For both patches.
>
> though, I agree with Serge that a spec reference would be nice.
>

PS: don't we need to test this for *_MAX too? I'd expect at least char and
short to have the same problem.

>
> Jan
>
> > ---
> >  tests/cl/program/execute/int-definitions.cl | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/cl/program/execute/int-definitions.cl
> > b/tests/cl/program/execute/int-definitions.cl
> > index 3d8ee63..a438fe4 100644
> > --- a/tests/cl/program/execute/int-definitions.cl
> > +++ b/tests/cl/program/execute/int-definitions.cl
> > @@ -12,12 +12,12 @@ global_size: 1 0 0
> >  [test]
> >  name: Char Definitions
> >  kernel_name: test_char
> > -arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
> > +arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
> >
> >  [test]
> >  name: Short Definitions
> >  kernel_name: test_short
> > -arg_out: 0 buffer int[3] 32767 -32768 65535
> > +arg_out: 0 buffer short[3] 32767 -32768 65535
> >
> >  [test]
> >  name: Int Definitions
> > @@ -32,7 +32,7 @@ arg_out: 0 buffer long[3] 9223372036854775807 \
> >                            18446744073709551615
> >  !*/
> >
> > -kernel void test_char(global int* out) {
> > +kernel void test_char(global char* out) {
> >    int i = 0;
> >    out[i++] = CHAR_BIT;
> >    out[i++] = CHAR_MAX;
> > @@ -42,7 +42,7 @@ kernel void test_char(global int* out) {
> >    out[i++] = UCHAR_MAX;
> >  }
> >
> > -kernel void test_short(global int* out) {
> > +kernel void test_short(global short* out) {
> >    int i = 0;
> >    out[i++] = SHRT_MAX;
> >    out[i++] = (SHRT_MIN - (short2)(0)).s0;
>
On Tue, Sep 15, 2015 at 9:42 AM, Jan Vesely <jan.vesely@rutgers.edu> wrote:

>
>
> On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely <jan.vesely@rutgers.edu>
> wrote:
>
>> On Thu, 2015-09-10 at 10:12 -0500, Aaron Watry wrote:
>> > The char/short return buffers were declared as ints.
>> >
>> > Signed-off-by: Aaron Watry <awatry@gmail.com>
>>
>> Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu>
>> For both patches.
>>
>> though, I agree with Serge that a spec reference would be nice.
>>
>
> PS: don't we need to test this for *_MAX too? I'd expect at least char and
> short to have the same problem.
>


Yes, it seems that the _MAX values are also broken because the values are
upgraded to ints by llvm.


>
>> Jan
>>
>> > ---
>> >  tests/cl/program/execute/int-definitions.cl | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/tests/cl/program/execute/int-definitions.cl
>> > b/tests/cl/program/execute/int-definitions.cl
>> > index 3d8ee63..a438fe4 100644
>> > --- a/tests/cl/program/execute/int-definitions.cl
>> > +++ b/tests/cl/program/execute/int-definitions.cl
>> > @@ -12,12 +12,12 @@ global_size: 1 0 0
>> >  [test]
>> >  name: Char Definitions
>> >  kernel_name: test_char
>> > -arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
>> > +arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
>> >
>> >  [test]
>> >  name: Short Definitions
>> >  kernel_name: test_short
>> > -arg_out: 0 buffer int[3] 32767 -32768 65535
>> > +arg_out: 0 buffer short[3] 32767 -32768 65535
>> >
>> >  [test]
>> >  name: Int Definitions
>> > @@ -32,7 +32,7 @@ arg_out: 0 buffer long[3] 9223372036854775807 \
>> >                            18446744073709551615
>> >  !*/
>> >
>> > -kernel void test_char(global int* out) {
>> > +kernel void test_char(global char* out) {
>> >    int i = 0;
>> >    out[i++] = CHAR_BIT;
>> >    out[i++] = CHAR_MAX;
>> > @@ -42,7 +42,7 @@ kernel void test_char(global int* out) {
>> >    out[i++] = UCHAR_MAX;
>> >  }
>> >
>> > -kernel void test_short(global int* out) {
>> > +kernel void test_short(global short* out) {
>> >    int i = 0;
>> >    out[i++] = SHRT_MAX;
>> >    out[i++] = (SHRT_MIN - (short2)(0)).s0;
>>
>
>
On Wed, 2015-09-16 at 17:18 -0500, Aaron Watry wrote:
> On Tue, Sep 15, 2015 at 9:42 AM, Jan Vesely <jan.vesely@rutgers.edu>
> wrote:
> 
> > 
> > 
> > On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely <jan.vesely@rutgers.edu
> > >
> > wrote:
> > 
> > > On Thu, 2015-09-10 at 10:12 -0500, Aaron Watry wrote:
> > > > The char/short return buffers were declared as ints.
> > > > 
> > > > Signed-off-by: Aaron Watry <awatry@gmail.com>
> > > 
> > > Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu>
> > > For both patches.
> > > 
> > > though, I agree with Serge that a spec reference would be nice.
> > > 
> > 
> > PS: don't we need to test this for *_MAX too? I'd expect at least
> > char and
> > short to have the same problem.
> > 
> 
> 
> Yes, it seems that the _MAX values are also broken because the values
> are
> upgraded to ints by llvm.

I checked the c99 specs, section 6.4.4.1 says that the type decimal
constants without suffix is the first of int, long int, long long int,
in which the value can be represented.
So I guess llvm is doing the right thing here.

Jan

> 
> 
> > 
> > > Jan
> > > 
> > > > ---
> > > >  tests/cl/program/execute/int-definitions.cl | 8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/tests/cl/program/execute/int-definitions.cl
> > > > b/tests/cl/program/execute/int-definitions.cl
> > > > index 3d8ee63..a438fe4 100644
> > > > --- a/tests/cl/program/execute/int-definitions.cl
> > > > +++ b/tests/cl/program/execute/int-definitions.cl
> > > > @@ -12,12 +12,12 @@ global_size: 1 0 0
> > > >  [test]
> > > >  name: Char Definitions
> > > >  kernel_name: test_char
> > > > -arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
> > > > +arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
> > > > 
> > > >  [test]
> > > >  name: Short Definitions
> > > >  kernel_name: test_short
> > > > -arg_out: 0 buffer int[3] 32767 -32768 65535
> > > > +arg_out: 0 buffer short[3] 32767 -32768 65535
> > > > 
> > > >  [test]
> > > >  name: Int Definitions
> > > > @@ -32,7 +32,7 @@ arg_out: 0 buffer long[3] 9223372036854775807
> > > > \
> > > >                            18446744073709551615
> > > >  !*/
> > > > 
> > > > -kernel void test_char(global int* out) {
> > > > +kernel void test_char(global char* out) {
> > > >    int i = 0;
> > > >    out[i++] = CHAR_BIT;
> > > >    out[i++] = CHAR_MAX;
> > > > @@ -42,7 +42,7 @@ kernel void test_char(global int* out) {
> > > >    out[i++] = UCHAR_MAX;
> > > >  }
> > > > 
> > > > -kernel void test_short(global int* out) {
> > > > +kernel void test_short(global short* out) {
> > > >    int i = 0;
> > > >    out[i++] = SHRT_MAX;
> > > >    out[i++] = (SHRT_MIN - (short2)(0)).s0;
> > > 
> > 
> >
On Wed, Sep 16, 2015 at 7:33 PM, Jan Vesely <jan.vesely@rutgers.edu> wrote:

> On Wed, 2015-09-16 at 17:18 -0500, Aaron Watry wrote:
> > On Tue, Sep 15, 2015 at 9:42 AM, Jan Vesely <jan.vesely@rutgers.edu>
> > wrote:
> >
> > >
> > >
> > > On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely <jan.vesely@rutgers.edu
> > > >
> > > wrote:
> > >
> > > > On Thu, 2015-09-10 at 10:12 -0500, Aaron Watry wrote:
> > > > > The char/short return buffers were declared as ints.
> > > > >
> > > > > Signed-off-by: Aaron Watry <awatry@gmail.com>
> > > >
> > > > Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu>
> > > > For both patches.
> > > >
> > > > though, I agree with Serge that a spec reference would be nice.
> > > >
> > >
> > > PS: don't we need to test this for *_MAX too? I'd expect at least
> > > char and
> > > short to have the same problem.
> > >
> >
> >
> > Yes, it seems that the _MAX values are also broken because the values
> > are
> > upgraded to ints by llvm.
>
> I checked the c99 specs, section 6.4.4.1 says that the type decimal
> constants without suffix is the first of int, long int, long long int,
> in which the value can be represented.
> So I guess llvm is doing the right thing here.
>
>
Yeah, I believe it...  It's just sad that the CL spec doesn't take into
account that the main usage of [CHAR|SHORT]_[MIN|MAX] will be for
comparisons that possibly include values of those types (or something
smaller than an int).

I'm updating the patches to send the _MAX values for each type through a
vector/scalar round trip.

While I'm at it, do we have any consensus on how the CHAR_BIT macro should
be defined?  The value of CHAR_BIT is the number of bits in a byte, but I'm
not really sure if it makes sense to cast it to char as well (probably
doesn't hurt anything), or if we should leave it as an int.


> Jan
>
> >
> >
> > >
> > > > Jan
> > > >
> > > > > ---
> > > > >  tests/cl/program/execute/int-definitions.cl | 8 ++++----
> > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/tests/cl/program/execute/int-definitions.cl
> > > > > b/tests/cl/program/execute/int-definitions.cl
> > > > > index 3d8ee63..a438fe4 100644
> > > > > --- a/tests/cl/program/execute/int-definitions.cl
> > > > > +++ b/tests/cl/program/execute/int-definitions.cl
> > > > > @@ -12,12 +12,12 @@ global_size: 1 0 0
> > > > >  [test]
> > > > >  name: Char Definitions
> > > > >  kernel_name: test_char
> > > > > -arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
> > > > > +arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
> > > > >
> > > > >  [test]
> > > > >  name: Short Definitions
> > > > >  kernel_name: test_short
> > > > > -arg_out: 0 buffer int[3] 32767 -32768 65535
> > > > > +arg_out: 0 buffer short[3] 32767 -32768 65535
> > > > >
> > > > >  [test]
> > > > >  name: Int Definitions
> > > > > @@ -32,7 +32,7 @@ arg_out: 0 buffer long[3] 9223372036854775807
> > > > > \
> > > > >                            18446744073709551615
> > > > >  !*/
> > > > >
> > > > > -kernel void test_char(global int* out) {
> > > > > +kernel void test_char(global char* out) {
> > > > >    int i = 0;
> > > > >    out[i++] = CHAR_BIT;
> > > > >    out[i++] = CHAR_MAX;
> > > > > @@ -42,7 +42,7 @@ kernel void test_char(global int* out) {
> > > > >    out[i++] = UCHAR_MAX;
> > > > >  }
> > > > >
> > > > > -kernel void test_short(global int* out) {
> > > > > +kernel void test_short(global short* out) {
> > > > >    int i = 0;
> > > > >    out[i++] = SHRT_MAX;
> > > > >    out[i++] = (SHRT_MIN - (short2)(0)).s0;
> > > >
> > >
> > >
>
On Thu, 2015-09-17 at 10:34 -0500, Aaron Watry wrote:
> On Wed, Sep 16, 2015 at 7:33 PM, Jan Vesely <jan.vesely@rutgers.edu>
> wrote:
> 
> > On Wed, 2015-09-16 at 17:18 -0500, Aaron Watry wrote:
> > > On Tue, Sep 15, 2015 at 9:42 AM, Jan Vesely <
> > > jan.vesely@rutgers.edu>
> > > wrote:
> > > 
> > > > 
> > > > 
> > > > On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely <
> > > > jan.vesely@rutgers.edu
> > > > > 
> > > > wrote:
> > > > 
> > > > > On Thu, 2015-09-10 at 10:12 -0500, Aaron Watry wrote:
> > > > > > The char/short return buffers were declared as ints.
> > > > > > 
> > > > > > Signed-off-by: Aaron Watry <awatry@gmail.com>
> > > > > 
> > > > > Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu>
> > > > > For both patches.
> > > > > 
> > > > > though, I agree with Serge that a spec reference would be
> > > > > nice.
> > > > > 
> > > > 
> > > > PS: don't we need to test this for *_MAX too? I'd expect at
> > > > least
> > > > char and
> > > > short to have the same problem.
> > > > 
> > > 
> > > 
> > > Yes, it seems that the _MAX values are also broken because the
> > > values
> > > are
> > > upgraded to ints by llvm.
> > 
> > I checked the c99 specs, section 6.4.4.1 says that the type decimal
> > constants without suffix is the first of int, long int, long long
> > int,
> > in which the value can be represented.
> > So I guess llvm is doing the right thing here.
> > 
> > 
> Yeah, I believe it...  It's just sad that the CL spec doesn't take
> into
> account that the main usage of [CHAR|SHORT]_[MIN|MAX] will be for
> comparisons that possibly include values of those types (or something
> smaller than an int).
> 
> I'm updating the patches to send the _MAX values for each type
> through a
> vector/scalar round trip.
> 
> While I'm at it, do we have any consensus on how the CHAR_BIT macro
> should
> be defined?  The value of CHAR_BIT is the number of bits in a byte,
> but I'm
> not really sure if it makes sense to cast it to char as well
> (probably
> doesn't hurt anything), or if we should leave it as an int.

I did a bit more digging, and it turns out that both c99 and OpenCL
require that "The values shall all be constant expressions suitable for
use in #if preprocessing directives." (section 5.2.4.2.1 and 6.12.3).
type casts don't work with preprocessor, so I think adding them is
against the specs.


Jan



> 
> 
> > Jan
> > 
> > > 
> > > 
> > > > 
> > > > > Jan
> > > > > 
> > > > > > ---
> > > > > >  tests/cl/program/execute/int-definitions.cl | 8 ++++----
> > > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/tests/cl/program/execute/int-definitions.cl
> > > > > > b/tests/cl/program/execute/int-definitions.cl
> > > > > > index 3d8ee63..a438fe4 100644
> > > > > > --- a/tests/cl/program/execute/int-definitions.cl
> > > > > > +++ b/tests/cl/program/execute/int-definitions.cl
> > > > > > @@ -12,12 +12,12 @@ global_size: 1 0 0
> > > > > >  [test]
> > > > > >  name: Char Definitions
> > > > > >  kernel_name: test_char
> > > > > > -arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
> > > > > > +arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
> > > > > > 
> > > > > >  [test]
> > > > > >  name: Short Definitions
> > > > > >  kernel_name: test_short
> > > > > > -arg_out: 0 buffer int[3] 32767 -32768 65535
> > > > > > +arg_out: 0 buffer short[3] 32767 -32768 65535
> > > > > > 
> > > > > >  [test]
> > > > > >  name: Int Definitions
> > > > > > @@ -32,7 +32,7 @@ arg_out: 0 buffer long[3]
> > > > > > 9223372036854775807
> > > > > > \
> > > > > >                            18446744073709551615
> > > > > >  !*/
> > > > > > 
> > > > > > -kernel void test_char(global int* out) {
> > > > > > +kernel void test_char(global char* out) {
> > > > > >    int i = 0;
> > > > > >    out[i++] = CHAR_BIT;
> > > > > >    out[i++] = CHAR_MAX;
> > > > > > @@ -42,7 +42,7 @@ kernel void test_char(global int* out) {
> > > > > >    out[i++] = UCHAR_MAX;
> > > > > >  }
> > > > > > 
> > > > > > -kernel void test_short(global int* out) {
> > > > > > +kernel void test_short(global short* out) {
> > > > > >    int i = 0;
> > > > > >    out[i++] = SHRT_MAX;
> > > > > >    out[i++] = (SHRT_MIN - (short2)(0)).s0;
> > > > > 
> > > > 
> > > > 
> >
On Thu, Sep 17, 2015 at 7:06 PM, Jan Vesely <jan.vesely@rutgers.edu> wrote:

> On Thu, 2015-09-17 at 10:34 -0500, Aaron Watry wrote:
> > On Wed, Sep 16, 2015 at 7:33 PM, Jan Vesely <jan.vesely@rutgers.edu>
> > wrote:
> >
> > > On Wed, 2015-09-16 at 17:18 -0500, Aaron Watry wrote:
> > > > On Tue, Sep 15, 2015 at 9:42 AM, Jan Vesely <
> > > > jan.vesely@rutgers.edu>
> > > > wrote:
> > > >
> > > > >
> > > > >
> > > > > On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely <
> > > > > jan.vesely@rutgers.edu
> > > > > >
> > > > > wrote:
> > > > >
> > > > > > On Thu, 2015-09-10 at 10:12 -0500, Aaron Watry wrote:
> > > > > > > The char/short return buffers were declared as ints.
> > > > > > >
> > > > > > > Signed-off-by: Aaron Watry <awatry@gmail.com>
> > > > > >
> > > > > > Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu>
> > > > > > For both patches.
> > > > > >
> > > > > > though, I agree with Serge that a spec reference would be
> > > > > > nice.
> > > > > >
> > > > >
> > > > > PS: don't we need to test this for *_MAX too? I'd expect at
> > > > > least
> > > > > char and
> > > > > short to have the same problem.
> > > > >
> > > >
> > > >
> > > > Yes, it seems that the _MAX values are also broken because the
> > > > values
> > > > are
> > > > upgraded to ints by llvm.
> > >
> > > I checked the c99 specs, section 6.4.4.1 says that the type decimal
> > > constants without suffix is the first of int, long int, long long
> > > int,
> > > in which the value can be represented.
> > > So I guess llvm is doing the right thing here.
> > >
> > >
> > Yeah, I believe it...  It's just sad that the CL spec doesn't take
> > into
> > account that the main usage of [CHAR|SHORT]_[MIN|MAX] will be for
> > comparisons that possibly include values of those types (or something
> > smaller than an int).
> >
> > I'm updating the patches to send the _MAX values for each type
> > through a
> > vector/scalar round trip.
> >
> > While I'm at it, do we have any consensus on how the CHAR_BIT macro
> > should
> > be defined?  The value of CHAR_BIT is the number of bits in a byte,
> > but I'm
> > not really sure if it makes sense to cast it to char as well
> > (probably
> > doesn't hurt anything), or if we should leave it as an int.
>
> I did a bit more digging, and it turns out that both c99 and OpenCL
> require that "The values shall all be constant expressions suitable for
> use in #if preprocessing directives." (section 5.2.4.2.1 and 6.12.3).
> type casts don't work with preprocessor, so I think adding them is
> against the specs.
>
>
Yeah, I read that part of the CL spec as well... But I'm not sure why
you're saying that casts don't work with the pre-processor.  It most
definitely does work to cast those values to the desired type without
altering the underlying numeric value (if they're needed as ints, they can
be sign/zero-extended without a change in the value), but given the wording
surrounding that section, maybe we do want to leave the CHAR/SHORT_MIN/MAX
values without an explicit cast (therefore as scalar ints).

I tested with the nvidia CL implementation and any char/short tests that
assume that [U][CHAR|SHORT]_[MIN|MAX] are of those respective types fail
because at least Nvidia treates them as ints.  I haven't tried out the AMD
implementation yet (it'd take a bit of setup work to do).

So where does that leave us?  Do we partially revert the libclc changes
(remove the explicit casts) to the [S]CHAR_MIN/SHORT_MIN values and leave
those values as ints, and then just modify the unit tests for
INT_MIN/INT_MAX to make sure they aren't upgraded to longs while leaving
the char/short tests alone?

I guess that's probably the way to go, but at this point, I'd like some
sort of consensus before I waste any more time going down the wrong
route... free time is in way too short a supply these days.

--Aaron



>
> Jan
>
>
>
> >
> >
> > > Jan
> > >
> > > >
> > > >
> > > > >
> > > > > > Jan
> > > > > >
> > > > > > > ---
> > > > > > >  tests/cl/program/execute/int-definitions.cl | 8 ++++----
> > > > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/tests/cl/program/execute/int-definitions.cl
> > > > > > > b/tests/cl/program/execute/int-definitions.cl
> > > > > > > index 3d8ee63..a438fe4 100644
> > > > > > > --- a/tests/cl/program/execute/int-definitions.cl
> > > > > > > +++ b/tests/cl/program/execute/int-definitions.cl
> > > > > > > @@ -12,12 +12,12 @@ global_size: 1 0 0
> > > > > > >  [test]
> > > > > > >  name: Char Definitions
> > > > > > >  kernel_name: test_char
> > > > > > > -arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
> > > > > > > +arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
> > > > > > >
> > > > > > >  [test]
> > > > > > >  name: Short Definitions
> > > > > > >  kernel_name: test_short
> > > > > > > -arg_out: 0 buffer int[3] 32767 -32768 65535
> > > > > > > +arg_out: 0 buffer short[3] 32767 -32768 65535
> > > > > > >
> > > > > > >  [test]
> > > > > > >  name: Int Definitions
> > > > > > > @@ -32,7 +32,7 @@ arg_out: 0 buffer long[3]
> > > > > > > 9223372036854775807
> > > > > > > \
> > > > > > >                            18446744073709551615
> > > > > > >  !*/
> > > > > > >
> > > > > > > -kernel void test_char(global int* out) {
> > > > > > > +kernel void test_char(global char* out) {
> > > > > > >    int i = 0;
> > > > > > >    out[i++] = CHAR_BIT;
> > > > > > >    out[i++] = CHAR_MAX;
> > > > > > > @@ -42,7 +42,7 @@ kernel void test_char(global int* out) {
> > > > > > >    out[i++] = UCHAR_MAX;
> > > > > > >  }
> > > > > > >
> > > > > > > -kernel void test_short(global int* out) {
> > > > > > > +kernel void test_short(global short* out) {
> > > > > > >    int i = 0;
> > > > > > >    out[i++] = SHRT_MAX;
> > > > > > >    out[i++] = (SHRT_MIN - (short2)(0)).s0;
> > > > > >
> > > > >
> > > > >
> > >
>
On Mon, 2015-09-21 at 23:07 -0500, Aaron Watry wrote:
> On Thu, Sep 17, 2015 at 7:06 PM, Jan Vesely <jan.vesely@rutgers.edu>
> wrote:
> 
> > On Thu, 2015-09-17 at 10:34 -0500, Aaron Watry wrote:
> > > On Wed, Sep 16, 2015 at 7:33 PM, Jan Vesely <
> > > jan.vesely@rutgers.edu>
> > > wrote:
> > > 
> > > > On Wed, 2015-09-16 at 17:18 -0500, Aaron Watry wrote:
> > > > > On Tue, Sep 15, 2015 at 9:42 AM, Jan Vesely <
> > > > > jan.vesely@rutgers.edu>
> > > > > wrote:
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely <
> > > > > > jan.vesely@rutgers.edu
> > > > > > > 
> > > > > > wrote:
> > > > > > 
> > > > > > > On Thu, 2015-09-10 at 10:12 -0500, Aaron Watry wrote:
> > > > > > > > The char/short return buffers were declared as ints.
> > > > > > > > 
> > > > > > > > Signed-off-by: Aaron Watry <awatry@gmail.com>
> > > > > > > 
> > > > > > > Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu>
> > > > > > > For both patches.
> > > > > > > 
> > > > > > > though, I agree with Serge that a spec reference would be
> > > > > > > nice.
> > > > > > > 
> > > > > > 
> > > > > > PS: don't we need to test this for *_MAX too? I'd expect at
> > > > > > least
> > > > > > char and
> > > > > > short to have the same problem.
> > > > > > 
> > > > > 
> > > > > 
> > > > > Yes, it seems that the _MAX values are also broken because
> > > > > the
> > > > > values
> > > > > are
> > > > > upgraded to ints by llvm.
> > > > 
> > > > I checked the c99 specs, section 6.4.4.1 says that the type
> > > > decimal
> > > > constants without suffix is the first of int, long int, long
> > > > long
> > > > int,
> > > > in which the value can be represented.
> > > > So I guess llvm is doing the right thing here.
> > > > 
> > > > 
> > > Yeah, I believe it...  It's just sad that the CL spec doesn't
> > > take
> > > into
> > > account that the main usage of [CHAR|SHORT]_[MIN|MAX] will be for
> > > comparisons that possibly include values of those types (or
> > > something
> > > smaller than an int).
> > > 
> > > I'm updating the patches to send the _MAX values for each type
> > > through a
> > > vector/scalar round trip.
> > > 
> > > While I'm at it, do we have any consensus on how the CHAR_BIT
> > > macro
> > > should
> > > be defined?  The value of CHAR_BIT is the number of bits in a
> > > byte,
> > > but I'm
> > > not really sure if it makes sense to cast it to char as well
> > > (probably
> > > doesn't hurt anything), or if we should leave it as an int.
> > 
> > I did a bit more digging, and it turns out that both c99 and OpenCL
> > require that "The values shall all be constant expressions suitable
> > for
> > use in #if preprocessing directives." (section 5.2.4.2.1 and
> > 6.12.3).
> > type casts don't work with preprocessor, so I think adding them is
> > against the specs.
> > 
> > 
> Yeah, I read that part of the CL spec as well... But I'm not sure why
> you're saying that casts don't work with the pre-processor.  It most
> definitely does work to cast those values to the desired type without
> altering the underlying numeric value (if they're needed as ints,
> they can
> be sign/zero-extended without a change in the value), but given the
> wording
> surrounding that section, maybe we do want to leave the
> CHAR/SHORT_MIN/MAX
> values without an explicit cast (therefore as scalar ints).

sorry, should have been more exact. Type casts don't work in
preprocessor expressions. Types (and casts) are not part of the
preprocessor language and only work in direct substitution. trying to
_use_ them in preprocessor fails. ex:

#define CHAR_MIN ((char)-127)

char c = CHAR_MIN; // Works OK

However,

#if CHAR_MIN < 0
#warning char is unsigned
#endif

fails.
Both in C and OCLC. I believe this is the situation the specs is
referring to in "use in #if preprocessing directives".

Jan




> 
> I tested with the nvidia CL implementation and any char/short tests
> that
> assume that [U][CHAR|SHORT]_[MIN|MAX] are of those respective types
> fail
> because at least Nvidia treates them as ints.  I haven't tried out
> the AMD
> implementation yet (it'd take a bit of setup work to do).
> 
> So where does that leave us?  Do we partially revert the libclc
> changes
> (remove the explicit casts) to the [S]CHAR_MIN/SHORT_MIN values and
> leave
> those values as ints, and then just modify the unit tests for
> INT_MIN/INT_MAX to make sure they aren't upgraded to longs while
> leaving
> the char/short tests alone?
> 
> I guess that's probably the way to go, but at this point, I'd like
> some
> sort of consensus before I waste any more time going down the wrong
> route... free time is in way too short a supply these days.
> 
> --Aaron
> 
> 
> 
> > 
> > Jan
> > 
> > 
> > 
> > > 
> > > 
> > > > Jan
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > > Jan
> > > > > > > 
> > > > > > > > ---
> > > > > > > >  tests/cl/program/execute/int-definitions.cl | 8 ++++--
> > > > > > > > --
> > > > > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/tests/cl/program/execute/int
> > > > > > > > -definitions.cl
> > > > > > > > b/tests/cl/program/execute/int-definitions.cl
> > > > > > > > index 3d8ee63..a438fe4 100644
> > > > > > > > --- a/tests/cl/program/execute/int-definitions.cl
> > > > > > > > +++ b/tests/cl/program/execute/int-definitions.cl
> > > > > > > > @@ -12,12 +12,12 @@ global_size: 1 0 0
> > > > > > > >  [test]
> > > > > > > >  name: Char Definitions
> > > > > > > >  kernel_name: test_char
> > > > > > > > -arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
> > > > > > > > +arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
> > > > > > > > 
> > > > > > > >  [test]
> > > > > > > >  name: Short Definitions
> > > > > > > >  kernel_name: test_short
> > > > > > > > -arg_out: 0 buffer int[3] 32767 -32768 65535
> > > > > > > > +arg_out: 0 buffer short[3] 32767 -32768 65535
> > > > > > > > 
> > > > > > > >  [test]
> > > > > > > >  name: Int Definitions
> > > > > > > > @@ -32,7 +32,7 @@ arg_out: 0 buffer long[3]
> > > > > > > > 9223372036854775807
> > > > > > > > \
> > > > > > > >                            18446744073709551615
> > > > > > > >  !*/
> > > > > > > > 
> > > > > > > > -kernel void test_char(global int* out) {
> > > > > > > > +kernel void test_char(global char* out) {
> > > > > > > >    int i = 0;
> > > > > > > >    out[i++] = CHAR_BIT;
> > > > > > > >    out[i++] = CHAR_MAX;
> > > > > > > > @@ -42,7 +42,7 @@ kernel void test_char(global int*
> > > > > > > > out) {
> > > > > > > >    out[i++] = UCHAR_MAX;
> > > > > > > >  }
> > > > > > > > 
> > > > > > > > -kernel void test_short(global int* out) {
> > > > > > > > +kernel void test_short(global short* out) {
> > > > > > > >    int i = 0;
> > > > > > > >    out[i++] = SHRT_MAX;
> > > > > > > >    out[i++] = (SHRT_MIN - (short2)(0)).s0;
> > > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> >
On Tue, Sep 22, 2015 at 12:02 AM, Jan Vesely <jan.vesely@rutgers.edu> wrote:

> On Mon, 2015-09-21 at 23:07 -0500, Aaron Watry wrote:
> > On Thu, Sep 17, 2015 at 7:06 PM, Jan Vesely <jan.vesely@rutgers.edu>
> > wrote:
> >
> > > On Thu, 2015-09-17 at 10:34 -0500, Aaron Watry wrote:
> > > > On Wed, Sep 16, 2015 at 7:33 PM, Jan Vesely <
> > > > jan.vesely@rutgers.edu>
> > > > wrote:
> > > >
> > > > > On Wed, 2015-09-16 at 17:18 -0500, Aaron Watry wrote:
> > > > > > On Tue, Sep 15, 2015 at 9:42 AM, Jan Vesely <
> > > > > > jan.vesely@rutgers.edu>
> > > > > > wrote:
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely <
> > > > > > > jan.vesely@rutgers.edu
> > > > > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > > > On Thu, 2015-09-10 at 10:12 -0500, Aaron Watry wrote:
> > > > > > > > > The char/short return buffers were declared as ints.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Aaron Watry <awatry@gmail.com>
> > > > > > > >
> > > > > > > > Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu>
> > > > > > > > For both patches.
> > > > > > > >
> > > > > > > > though, I agree with Serge that a spec reference would be
> > > > > > > > nice.
> > > > > > > >
> > > > > > >
> > > > > > > PS: don't we need to test this for *_MAX too? I'd expect at
> > > > > > > least
> > > > > > > char and
> > > > > > > short to have the same problem.
> > > > > > >
> > > > > >
> > > > > >
> > > > > > Yes, it seems that the _MAX values are also broken because
> > > > > > the
> > > > > > values
> > > > > > are
> > > > > > upgraded to ints by llvm.
> > > > >
> > > > > I checked the c99 specs, section 6.4.4.1 says that the type
> > > > > decimal
> > > > > constants without suffix is the first of int, long int, long
> > > > > long
> > > > > int,
> > > > > in which the value can be represented.
> > > > > So I guess llvm is doing the right thing here.
> > > > >
> > > > >
> > > > Yeah, I believe it...  It's just sad that the CL spec doesn't
> > > > take
> > > > into
> > > > account that the main usage of [CHAR|SHORT]_[MIN|MAX] will be for
> > > > comparisons that possibly include values of those types (or
> > > > something
> > > > smaller than an int).
> > > >
> > > > I'm updating the patches to send the _MAX values for each type
> > > > through a
> > > > vector/scalar round trip.
> > > >
> > > > While I'm at it, do we have any consensus on how the CHAR_BIT
> > > > macro
> > > > should
> > > > be defined?  The value of CHAR_BIT is the number of bits in a
> > > > byte,
> > > > but I'm
> > > > not really sure if it makes sense to cast it to char as well
> > > > (probably
> > > > doesn't hurt anything), or if we should leave it as an int.
> > >
> > > I did a bit more digging, and it turns out that both c99 and OpenCL
> > > require that "The values shall all be constant expressions suitable
> > > for
> > > use in #if preprocessing directives." (section 5.2.4.2.1 and
> > > 6.12.3).
> > > type casts don't work with preprocessor, so I think adding them is
> > > against the specs.
> > >
> > >
> > Yeah, I read that part of the CL spec as well... But I'm not sure why
> > you're saying that casts don't work with the pre-processor.  It most
> > definitely does work to cast those values to the desired type without
> > altering the underlying numeric value (if they're needed as ints,
> > they can
> > be sign/zero-extended without a change in the value), but given the
> > wording
> > surrounding that section, maybe we do want to leave the
> > CHAR/SHORT_MIN/MAX
> > values without an explicit cast (therefore as scalar ints).
>
> sorry, should have been more exact. Type casts don't work in
> preprocessor expressions. Types (and casts) are not part of the
> preprocessor language and only work in direct substitution. trying to
> _use_ them in preprocessor fails. ex:
>
> #define CHAR_MIN ((char)-127)
>
> char c = CHAR_MIN; // Works OK
>
> However,
>
> #if CHAR_MIN < 0
> #warning char is unsigned
> #endif
>
> fails.
> Both in C and OCLC. I believe this is the situation the specs is
> referring to in "use in #if preprocessing directives".
>
>
Ahh, I get it now.

If that's the case, then we don't have any real choice here.  I'll remove
the explicit casts, and see what else I think we need to do.

Since we can't do explicit casts, the *_MAX values should probably all be
left alone (e.g. 127, 32767, etc), and that's that.

For piglit, we take my first patch in this series, and remove the explicit
vectorization of the 8/16-bit types for testing purposes (because they're
actually ints), and we leave the vectorization of INT_MIN, and maybe add a
check for INT_MAX just to make sure it's actually stored as in integer and
not a long.

I believe that we can drop the second patch in this series, since it
doesn't really add much in this case (we already have other tests to make
sure that char/short data types actually work). I don't really care either
way.

Then for CLC, we just do the following to
generic/include/clc/integer/definitions.h:
-#define INT_MIN ((int)(-2147483647 - 1))
+#define INT_MIN (-2147483647 - 1)
-#define SCHAR_MIN ((char)(-127 - 1))
+#define SCHAR_MIN (-127 - 1)
-#define SHRT_MIN ((short)(-32767 - 1))
+#define SHRT_MIN (-32767 - 1)

And leave the rest alone.

Sound good?

--Aaron




> Jan
>
>
>
>
> >
> > I tested with the nvidia CL implementation and any char/short tests
> > that
> > assume that [U][CHAR|SHORT]_[MIN|MAX] are of those respective types
> > fail
> > because at least Nvidia treates them as ints.  I haven't tried out
> > the AMD
> > implementation yet (it'd take a bit of setup work to do).
> >
> > So where does that leave us?  Do we partially revert the libclc
> > changes
> > (remove the explicit casts) to the [S]CHAR_MIN/SHORT_MIN values and
> > leave
> > those values as ints, and then just modify the unit tests for
> > INT_MIN/INT_MAX to make sure they aren't upgraded to longs while
> > leaving
> > the char/short tests alone?
> >
> > I guess that's probably the way to go, but at this point, I'd like
> > some
> > sort of consensus before I waste any more time going down the wrong
> > route... free time is in way too short a supply these days.
> >
> > --Aaron
> >
> >
> >
> > >
> > > Jan
> > >
> > >
> > >
> > > >
> > > >
> > > > > Jan
> > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > Jan
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > >  tests/cl/program/execute/int-definitions.cl | 8 ++++--
> > > > > > > > > --
> > > > > > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/tests/cl/program/execute/int
> > > > > > > > > -definitions.cl
> > > > > > > > > b/tests/cl/program/execute/int-definitions.cl
> > > > > > > > > index 3d8ee63..a438fe4 100644
> > > > > > > > > --- a/tests/cl/program/execute/int-definitions.cl
> > > > > > > > > +++ b/tests/cl/program/execute/int-definitions.cl
> > > > > > > > > @@ -12,12 +12,12 @@ global_size: 1 0 0
> > > > > > > > >  [test]
> > > > > > > > >  name: Char Definitions
> > > > > > > > >  kernel_name: test_char
> > > > > > > > > -arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
> > > > > > > > > +arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
> > > > > > > > >
> > > > > > > > >  [test]
> > > > > > > > >  name: Short Definitions
> > > > > > > > >  kernel_name: test_short
> > > > > > > > > -arg_out: 0 buffer int[3] 32767 -32768 65535
> > > > > > > > > +arg_out: 0 buffer short[3] 32767 -32768 65535
> > > > > > > > >
> > > > > > > > >  [test]
> > > > > > > > >  name: Int Definitions
> > > > > > > > > @@ -32,7 +32,7 @@ arg_out: 0 buffer long[3]
> > > > > > > > > 9223372036854775807
> > > > > > > > > \
> > > > > > > > >                            18446744073709551615
> > > > > > > > >  !*/
> > > > > > > > >
> > > > > > > > > -kernel void test_char(global int* out) {
> > > > > > > > > +kernel void test_char(global char* out) {
> > > > > > > > >    int i = 0;
> > > > > > > > >    out[i++] = CHAR_BIT;
> > > > > > > > >    out[i++] = CHAR_MAX;
> > > > > > > > > @@ -42,7 +42,7 @@ kernel void test_char(global int*
> > > > > > > > > out) {
> > > > > > > > >    out[i++] = UCHAR_MAX;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > -kernel void test_short(global int* out) {
> > > > > > > > > +kernel void test_short(global short* out) {
> > > > > > > > >    int i = 0;
> > > > > > > > >    out[i++] = SHRT_MAX;
> > > > > > > > >    out[i++] = (SHRT_MIN - (short2)(0)).s0;
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > >
>
On Tue, 2015-09-22 at 09:11 -0500, Aaron Watry wrote:
> On Tue, Sep 22, 2015 at 12:02 AM, Jan Vesely <jan.vesely@rutgers.edu>
> wrote:
> 
> > On Mon, 2015-09-21 at 23:07 -0500, Aaron Watry wrote:
> > > On Thu, Sep 17, 2015 at 7:06 PM, Jan Vesely <
> > > jan.vesely@rutgers.edu>
> > > wrote:
> > > 
> > > > On Thu, 2015-09-17 at 10:34 -0500, Aaron Watry wrote:
> > > > > On Wed, Sep 16, 2015 at 7:33 PM, Jan Vesely <
> > > > > jan.vesely@rutgers.edu>
> > > > > wrote:
> > > > > 
> > > > > > On Wed, 2015-09-16 at 17:18 -0500, Aaron Watry wrote:
> > > > > > > On Tue, Sep 15, 2015 at 9:42 AM, Jan Vesely <
> > > > > > > jan.vesely@rutgers.edu>
> > > > > > > wrote:
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely <
> > > > > > > > jan.vesely@rutgers.edu
> > > > > > > > > 
> > > > > > > > wrote:
> > > > > > > > 
> > > > > > > > > On Thu, 2015-09-10 at 10:12 -0500, Aaron Watry wrote:
> > > > > > > > > > The char/short return buffers were declared as
> > > > > > > > > > ints.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Aaron Watry <awatry@gmail.com>
> > > > > > > > > 
> > > > > > > > > Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu>
> > > > > > > > > For both patches.
> > > > > > > > > 
> > > > > > > > > though, I agree with Serge that a spec reference
> > > > > > > > > would be
> > > > > > > > > nice.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > PS: don't we need to test this for *_MAX too? I'd
> > > > > > > > expect at
> > > > > > > > least
> > > > > > > > char and
> > > > > > > > short to have the same problem.
> > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Yes, it seems that the _MAX values are also broken
> > > > > > > because
> > > > > > > the
> > > > > > > values
> > > > > > > are
> > > > > > > upgraded to ints by llvm.
> > > > > > 
> > > > > > I checked the c99 specs, section 6.4.4.1 says that the type
> > > > > > decimal
> > > > > > constants without suffix is the first of int, long int,
> > > > > > long
> > > > > > long
> > > > > > int,
> > > > > > in which the value can be represented.
> > > > > > So I guess llvm is doing the right thing here.
> > > > > > 
> > > > > > 
> > > > > Yeah, I believe it...  It's just sad that the CL spec doesn't
> > > > > take
> > > > > into
> > > > > account that the main usage of [CHAR|SHORT]_[MIN|MAX] will be
> > > > > for
> > > > > comparisons that possibly include values of those types (or
> > > > > something
> > > > > smaller than an int).
> > > > > 
> > > > > I'm updating the patches to send the _MAX values for each
> > > > > type
> > > > > through a
> > > > > vector/scalar round trip.
> > > > > 
> > > > > While I'm at it, do we have any consensus on how the CHAR_BIT
> > > > > macro
> > > > > should
> > > > > be defined?  The value of CHAR_BIT is the number of bits in a
> > > > > byte,
> > > > > but I'm
> > > > > not really sure if it makes sense to cast it to char as well
> > > > > (probably
> > > > > doesn't hurt anything), or if we should leave it as an int.
> > > > 
> > > > I did a bit more digging, and it turns out that both c99 and
> > > > OpenCL
> > > > require that "The values shall all be constant expressions
> > > > suitable
> > > > for
> > > > use in #if preprocessing directives." (section 5.2.4.2.1 and
> > > > 6.12.3).
> > > > type casts don't work with preprocessor, so I think adding them
> > > > is
> > > > against the specs.
> > > > 
> > > > 
> > > Yeah, I read that part of the CL spec as well... But I'm not sure
> > > why
> > > you're saying that casts don't work with the pre-processor.  It
> > > most
> > > definitely does work to cast those values to the desired type
> > > without
> > > altering the underlying numeric value (if they're needed as ints,
> > > they can
> > > be sign/zero-extended without a change in the value), but given
> > > the
> > > wording
> > > surrounding that section, maybe we do want to leave the
> > > CHAR/SHORT_MIN/MAX
> > > values without an explicit cast (therefore as scalar ints).
> > 
> > sorry, should have been more exact. Type casts don't work in
> > preprocessor expressions. Types (and casts) are not part of the
> > preprocessor language and only work in direct substitution. trying
> > to
> > _use_ them in preprocessor fails. ex:
> > 
> > #define CHAR_MIN ((char)-127)
> > 
> > char c = CHAR_MIN; // Works OK
> > 
> > However,
> > 
> > #if CHAR_MIN < 0
> > #warning char is unsigned
> > #endif
> > 
> > fails.
> > Both in C and OCLC. I believe this is the situation the specs is
> > referring to in "use in #if preprocessing directives".
> > 
> > 
> Ahh, I get it now.
> 
> If that's the case, then we don't have any real choice here.  I'll
> remove
> the explicit casts, and see what else I think we need to do.
> 
> Since we can't do explicit casts, the *_MAX values should probably
> all be
> left alone (e.g. 127, 32767, etc), and that's that.
> 
> For piglit, we take my first patch in this series, and remove the
> explicit
> vectorization of the 8/16-bit types for testing purposes (because
> they're
> actually ints), and we leave the vectorization of INT_MIN, and maybe
> add a
> check for INT_MAX just to make sure it's actually stored as in
> integer and
> not a long.
> 
> I believe that we can drop the second patch in this series, since it
> doesn't really add much in this case (we already have other tests to
> make
> sure that char/short data types actually work). I don't really care
> either
> way.
> 
> Then for CLC, we just do the following to
> generic/include/clc/integer/definitions.h:
> -#define INT_MIN ((int)(-2147483647 - 1))
> +#define INT_MIN (-2147483647 - 1)
> -#define SCHAR_MIN ((char)(-127 - 1))
> +#define SCHAR_MIN (-127 - 1)
> -#define SHRT_MIN ((short)(-32767 - 1))
> +#define SHRT_MIN (-32767 - 1)
> 
> And leave the rest alone.
> 
> Sound good?
> 
> --Aaron
> 
> 
> 
> 
> > Jan
> > 
> > 
> > 
> > 
> > > 
> > > I tested with the nvidia CL implementation and any char/short
> > > tests
> > > that
> > > assume that [U][CHAR|SHORT]_[MIN|MAX] are of those respective
> > > types
> > > fail
> > > because at least Nvidia treates them as ints.  I haven't tried
> > > out
> > > the AMD
> > > implementation yet (it'd take a bit of setup work to do).
> > > 
> > > So where does that leave us?  Do we partially revert the libclc
> > > changes
> > > (remove the explicit casts) to the [S]CHAR_MIN/SHORT_MIN values
> > > and
> > > leave
> > > those values as ints, and then just modify the unit tests for
> > > INT_MIN/INT_MAX to make sure they aren't upgraded to longs while
> > > leaving
> > > the char/short tests alone?

I think we should just stick with spec definitions in libclc.
yeah, vectorization should work for (u)int and (u)long, so we can have
that.
It'd be nice to have a test that _MIN _MAX values actually fit in the
corresponding type, but I don't see a simple way to test it (casting
the constants in the test might work), and I don't think anyone would
ever fail such test so - meh.

Jan


> > > 
> > > I guess that's probably the way to go, but at this point, I'd
> > > like
> > > some
> > > sort of consensus before I waste any more time going down the
> > > wrong
> > > route... free time is in way too short a supply these days.
> > > 
> > > --Aaron
> > > 
> > > 
> > > 
> > > > 
> > > > Jan
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > Jan
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > > Jan
> > > > > > > > > 
> > > > > > > > > > ---
> > > > > > > > > >  tests/cl/program/execute/int-definitions.cl | 8
> > > > > > > > > > ++++--
> > > > > > > > > > --
> > > > > > > > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/tests/cl/program/execute/int
> > > > > > > > > > -definitions.cl
> > > > > > > > > > b/tests/cl/program/execute/int-definitions.cl
> > > > > > > > > > index 3d8ee63..a438fe4 100644
> > > > > > > > > > --- a/tests/cl/program/execute/int-definitions.cl
> > > > > > > > > > +++ b/tests/cl/program/execute/int-definitions.cl
> > > > > > > > > > @@ -12,12 +12,12 @@ global_size: 1 0 0
> > > > > > > > > >  [test]
> > > > > > > > > >  name: Char Definitions
> > > > > > > > > >  kernel_name: test_char
> > > > > > > > > > -arg_out: 0 buffer int[6] 8 127 -128 127 -128 255
> > > > > > > > > > +arg_out: 0 buffer char[6] 8 127 -128 127 -128 255
> > > > > > > > > > 
> > > > > > > > > >  [test]
> > > > > > > > > >  name: Short Definitions
> > > > > > > > > >  kernel_name: test_short
> > > > > > > > > > -arg_out: 0 buffer int[3] 32767 -32768 65535
> > > > > > > > > > +arg_out: 0 buffer short[3] 32767 -32768 65535
> > > > > > > > > > 
> > > > > > > > > >  [test]
> > > > > > > > > >  name: Int Definitions
> > > > > > > > > > @@ -32,7 +32,7 @@ arg_out: 0 buffer long[3]
> > > > > > > > > > 9223372036854775807
> > > > > > > > > > \
> > > > > > > > > >                            18446744073709551615
> > > > > > > > > >  !*/
> > > > > > > > > > 
> > > > > > > > > > -kernel void test_char(global int* out) {
> > > > > > > > > > +kernel void test_char(global char* out) {
> > > > > > > > > >    int i = 0;
> > > > > > > > > >    out[i++] = CHAR_BIT;
> > > > > > > > > >    out[i++] = CHAR_MAX;
> > > > > > > > > > @@ -42,7 +42,7 @@ kernel void test_char(global int*
> > > > > > > > > > out) {
> > > > > > > > > >    out[i++] = UCHAR_MAX;
> > > > > > > > > >  }
> > > > > > > > > > 
> > > > > > > > > > -kernel void test_short(global int* out) {
> > > > > > > > > > +kernel void test_short(global short* out) {
> > > > > > > > > >    int i = 0;
> > > > > > > > > >    out[i++] = SHRT_MAX;
> > > > > > > > > >    out[i++] = (SHRT_MIN - (short2)(0)).s0;
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > 
> > > > 
> >