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