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

[Richard Smith]

On Wed, Nov 21, 2012 at 6:15 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> 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.

I see, this is enough to crash Clang:

__int128 n, m = n*n;

It seems we try to produce a libcall which returns 4 i32s, and the
calling convention doesn't support that?