On Wed, Nov 21, 2012 at 5: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: >> >>> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
