On Nov 21, 2012, at 8:44 PM, Richard Smith <[email protected]> wrote:
> On Wed, Nov 21, 2012 at 3:48 PM, Stephen Canon <[email protected]> wrote: >> >> On Nov 21, 2012, at 5:10 PM, Dmitri Gribenko <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
