[Piglit] [PATCH 2/2] cl: Change data types of char/short buffers in integer limits tests
Jan Vesely
jan.vesely at rutgers.edu
Mon Sep 21 22:02:15 PDT 2015
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".
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 --------------
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/20150922/e7b40e5e/attachment-0001.sig>
More information about the Piglit
mailing list