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

[Eli Friedman]

On Wed, Nov 21, 2012 at 5: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:
>>
>>> There are similar issues in 'nonnull', 'ownership' and 'format'
>>> attributes.  I have an incomplete patch for all these, that refactors
>>> duplicated code over handle*() functions in SemaDeclAttr.cpp and fixes
>>> this 128-bit issue, but I decided not to submit it until the semantic
>>> analysis is fixed.
>>
>> Great, thanks.
>>
>>> 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. It'd also be great to factor out some of
> the repetition here.
>
> 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 {
>   // ...
>
>>> 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.
>
> +      if (ResultVal.getBitWidth() != Width)
> +        ResultVal = ResultVal.trunc(Width);
>
> Have you considered producing the warn_integer_too_large diagnostic if
> we truncate here?
>
> +      // 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.

We still don't have any way to legalize 128-bit multiplication and
division on 32-bit platforms.

-Eli