[Libclc-dev] [PATCH] integer: Update integer limits to comply with spec

[Tom Stellard via Libclc-dev]

On Thu, Sep 10, 2015 at 08:28:02PM -0500, Aaron Watry wrote:
> On Thu, Sep 10, 2015 at 12:10 PM, Tom Stellard <tom at stellard.net> wrote:
> 
> > On Thu, Sep 10, 2015 at 10:14:34AM -0500, Aaron Watry via Libclc-dev wrote:
> > > The values for the char/short/integer/long minimums were declared with
> > their actual values, not the definitions from the CL spec (v1.1).  As a
> > result, (-2147483648) was actually being treated as a long by the
> > compiler, not an int, which caused issues when trying to add/subtract that
> > value from a vector.
> > >
> > > Update the definitions to use the values declared by the spec, and also
> > add explicit casts for the char/short minimums so that the compiler
> > actually treats them as shorts/chars. Without those casts, they actually
> > end up stored as integers.
> > >
> > > The compiler can sign extend the values if it needs to conver the
> > char->short or short->int.
> > >
> > > Reported-by: Moritz Pflanzer <moritz.pflanzer14 at imperial.ac.uk>
> > > CC: Moritz Pflanzer <moritz.pflanzer14 at imperial.ac.uk>
> > > Signed-off-by: Aaron Watry <awatry at gmail.com>
> > > ---
> > >  generic/include/clc/integer/definitions.h | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/generic/include/clc/integer/definitions.h
> > b/generic/include/clc/integer/definitions.h
> > > index a407974..505002f 100644
> > > --- a/generic/include/clc/integer/definitions.h
> > > +++ b/generic/include/clc/integer/definitions.h
> > > @@ -1,14 +1,14 @@
> > >  #define CHAR_BIT 8
> > >  #define INT_MAX 2147483647
> > > -#define INT_MIN -2147483648
> > > +#define INT_MIN (-2147483647 - 1)
> >
> > Is there some reason why
> >
> > #define INT_MIN ((int)-2147483648)
> >
> > won't work?
> >
> > I also don't understand how subtracting one guarantees the result will
> > be an integer and not long.
> >
> >
> You're right that ((int)-2147483648) would work as well, which is somewhat
> similar to what I did with the char/short minimums in this patch (forcibly
> casting the result). The only reason that I went with the value that I did
> for INT_MIN was that the spec seems pretty specific:
> 
> Section 6.11.3 of the 1.1 Spec:
> "The macro names given in the following list must use the values specified.
> The values shall all
> be constant expressions suitable for use in #if preprocessing directives."
> 
> <snip>
> #define INT_MIN (-2147483647 – 1)
> 
> The only reason that I put an explicit cast in for the char/short values
> was because they would not actually work correctly for charN/shortN casts
> without them, and it seems to be the intention behind the spec that the
> MIN/MAX of a given type should be stored as that type.
> 
> I don't personally care that much what we end up with, as the result after
> preprocessing should be identical...  as long as the new unit tests pass,
> I'm good.
> 

I don't mind using the values from the spec.  The only change I would request
is that we cast INT_MIN to int like we do for char and short.  I think the
compiler is still allowed to evaluate that expression to long if it wants to.

-Tom

> --Aaron
> 
> 
> > -Tom
> >
> > >  #define LONG_MAX  0x7fffffffffffffffL
> > > -#define LONG_MIN -0x8000000000000000L
> > > +#define LONG_MIN (-0x7fffffffffffffffL - 1)
> > > +#define CHAR_MAX SCHAR_MAX
> > > +#define CHAR_MIN SCHAR_MIN
> > >  #define SCHAR_MAX 127
> > > -#define SCHAR_MIN -128
> > > -#define CHAR_MAX 127
> > > -#define CHAR_MIN -128
> > > +#define SCHAR_MIN ((char)(-127 - 1))
> > >  #define SHRT_MAX 32767
> > > -#define SHRT_MIN -32768
> > > +#define SHRT_MIN ((short)(-32767 - 1))
> > >  #define UCHAR_MAX 255
> > >  #define USHRT_MAX 65535
> > >  #define UINT_MAX 0xffffffff
> > > --
> > > 2.1.4
> > >
> > > _______________________________________________
> > > Libclc-dev mailing list
> > > Libclc-dev at lists.llvm.org
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/libclc-dev
> >