[Piglit] [PATCH 2/2] cl: Change data types of char/short buffers in integer limits tests
Jan Vesely
jan.vesely at rutgers.edu
Fri Sep 25 22:55:49 PDT 2015
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 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 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;
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20150926/e1ec44df/attachment.sig>
More information about the Piglit
mailing list