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

[Eli Friedman]

On Wed, Nov 21, 2012 at 6:26 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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?

Yes, and nobody has bothered to fix it because the relevant functions
aren't available on 32-bit platforms anyway.

-Eli