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

[Richard Smith]

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.