[cfe-commits] [PATCH] Improved handling of 128-bit integer literals

[Stephen Canon]

On Nov 21, 2012, at 8:44 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Wed, Nov 21, 2012 at 3:48 PM, Stephen Canon <scanon at apple.com> wrote:
>> 
>> On Nov 21, 2012, at 5:10 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>> 
>>> I see that the code is much cleaner when FitsIn* are computed upfront,
>>> but this leads to some extra work -- each isIntN() boils down to
>>> counting leading zeros.  Is there a clean way to defer the computation
>>> to the point where it is required?  I don't know how hot this code is,
>>> so maybe this is not worth doing.
>> 
>> In theory the compiler can do this optimization for us, at least in the common case of "small" literals (where I believe everything is in the APInt header).
> 
> You could directly call ResultVal.getActiveBits() once, rather than
> repeatedly calling isIntN.

Good idea!

> For the MS suffix case, how about...
> 
> if (Literal.isLongLong) {
>  Width = Context.getTargetInfo().getLongLongWidth();
>  Ty = Literal.isUnsigned ? Context.UnsignedLongLongTy : Context.LongLongTy;
> } else if (Literal.isLong) {
>  // ...
> } else {
>  // ...

I was going for code that is easy instead of code that is simple.  However, certainly for the MS case this is straightforward enough.

>>> This LGTM with tests and code style changes mentioned above, but
>>> please wait for Richard Smith's review.
>> 
>> Great, I'll add the tests you requested and fix the typos in the meantime.
> 
> +    // If we are in MSVC mode, we pretend that "LL" is a microsoft literal
> +    // suffix in order to get the expected (wrong) behavior.
> +    if (getLangOpts().MicrosoftExt && Literal.isLongLong) {
> +      Literal.isMicrosoftInteger = true;
> +    }
> 
> This should check MicrosoftMode, not MicrosoftExt, since it changes
> the behavior of conforming code. Also, no braces here.

Ok; I was matching the existing behavior here, but if the existing behavior is wrong, I'll change it.

> +      if (ResultVal.getBitWidth() != Width)
> +        ResultVal = ResultVal.trunc(Width);
> 
> Have you considered producing the warn_integer_too_large diagnostic if
> we truncate here?

We don't ever truncate non-zero bits here because of the earlier checks; there's no need for a diagnostic.

> +      // We will evaluate literals in an "extended integer type" as allowed by
> +      // the C and C++ standards.  On LP64 platforms (which have __[u]int128_t)
> +      // we use that type.  However, we can't use it on other platforms, or
> +      // else we would generate arithmetic using those types and crash when we
> +      // try to codegen.  If we don't have LP64, we use [unsigned] long long
> +      // instead.
> 
> We currently provide __int128 on all platforms. If the legalizer can't
> cope with that on some platform, then we have a problem. You're right
> that we only provide the __int128_t and __uint128_t typedefs on
> platforms with 64-bit pointers, though that restriction dates back to
> r70480, when I would expect the legalizer was significantly more
> limited. We might want to revisit that now.

As Eli noted, you expose yourself to a lot of crashers if you blindly generate __int128 (I tried that first).  Supporting i128 everywhere would be nice, but is outside the scope of this patch.

- Steve