[Piglit] [PATCH 2/2] cl: Change data types of char/short buffers in integer limits tests
Aaron Watry
awatry at gmail.com
Tue Sep 22 07:11:58 PDT 2015
On Tue, Sep 22, 2015 at 12:02 AM, Jan Vesely <jan.vesely at 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 at 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 at 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 at rutgers.edu>
> > > > > > wrote:
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Tue, Sep 15, 2015 at 7:28 AM, Jan Vesely <
> > > > > > > jan.vesely at 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 at gmail.com>
> > > > > > > >
> > > > > > > > Reviewed-by: Jan Vesely <jan.vesely at 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;
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20150922/c6583936/attachment-0001.html>
More information about the Piglit
mailing list