<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Sep 22, 2015 at 12:02 AM, Jan Vesely <<a href="mailto:jan.vesely@rutgers.edu" target="_blank">jan.vesely@rutgers.edu</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">On Mon, 2015-09-21 at 23:07 -0500, Aaron Watry wrote: > On Thu, Sep 17, 2015 at 7:06 PM, Jan Vesely <<a href="mailto:jan.vesely@rutgers.edu">jan.vesely@rutgers.edu</a>> > wrote: > > > On Thu, 2015-09-17 at 10:34 -0500, Aaron Watry wrote: > > > On Wed, Sep 16, 2015 at 7:33 PM, Jan Vesely < > > > <a href="mailto:jan.vesely@rutgers.edu">jan.vesely@rutgers.edu</a>> > > > wrote: > > > > > > > On Wed, 2015-09-16 at 17:18 -0500, Aaron Watry wrote: > > > > > On Tue, Sep 15, 2015 at 9:42 AM, Jan Vesely < > > > > > <a href="mailto:jan.vesely@rutgers.edu">jan.vesely@rutgers.edu</a>> > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely < > > > > > > <a href="mailto:jan.vesely@rutgers.edu">jan.vesely@rutgers.edu</a> > > > > > > > > > > > > > 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 <<a href="mailto:awatry@gmail.com">awatry@gmail.com</a>> > > > > > > > > > > > > > > Reviewed-by: Jan Vesely <<a href="mailto:jan.vesely@rutgers.edu">jan.vesely@rutgers.edu</a>> > > > > > > > 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). </div></div>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". </blockquote><div> </div><div>Ahh, I get it now. </div><div>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. </div><div>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. </div><div>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. </div><div>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. </div><div> </div><div>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) </div><div>And leave the rest alone. </div><div>Sound good? </div><div> </div><div>--Aaron </div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Jan <div class=""><div class="h5"> > > 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/<a href="http://int-definitions.cl" rel="noreferrer" target="_blank">int-definitions.cl</a> | 8 ++++-- > > > > > > > > -- > > > > > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > > > diff --git a/tests/cl/program/execute/int > > > > > > > > -<a href="http://definitions.cl" rel="noreferrer" target="_blank">definitions.cl</a> > > > > > > > > b/tests/cl/program/execute/<a href="http://int-definitions.cl" rel="noreferrer" target="_blank">int-definitions.cl</a> > > > > > > > > index 3d8ee63..a438fe4 100644 > > > > > > > > --- a/tests/cl/program/execute/<a href="http://int-definitions.cl" rel="noreferrer" target="_blank">int-definitions.cl</a> > > > > > > > > +++ b/tests/cl/program/execute/<a href="http://int-definitions.cl" rel="noreferrer" target="_blank">int-definitions.cl</a> > > > > > > > > @@ -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; > > > > > > > > > > > > > > > > > > > > > > > > > </div></div></blockquote></div> </div></div>