This is an archive of the discontinued LLVM Phabricator instance.

[APInt] Add APInt::extractBits() method to extract APInt subrange
ClosedPublic

Authored by RKSimon on Feb 24 2017, 5:52 AM.

Details

Summary

The current pattern for extract bits in range is typically:

Mask.lshr(BitOffset).trunc(SubSizeInBits);

Which can be particularly slow for large APInts (MaskSizeInBits > 64) as they require the allocation of memory for the temporary variable.

This is another of the compile time issues identified in PR32037 (see also D30265).

This patch adds the APInt::extractBits() helper method which avoids the temporary memory allocation.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Feb 24 2017, 5:52 AM
filcab edited edge metadata.Feb 24 2017, 8:35 AM

LGTM

lib/Support/APInt.cpp
640

Looks deprecated

/// Equivalent to APInt(numBits, ArrayRef<uint64_t>(bigVal, numWords)), but
/// deprecated because this constructor is prone to ambiguity with the
/// APInt(unsigned, uint64_t, bool) constructor.
///
/// If this overload is ever deleted, care should be taken to prevent calls
/// from being incorrectly captured by the APInt(unsigned, uint64_t, bool)
/// constructor.
APInt(unsigned numBits, unsigned numWords, const uint64_t bigVal[]);

Maybe return APInt(numBits, makeArrayRef(pVal + loWord, 1 + hiWord - loWord));?

davide accepted this revision.Feb 24 2017, 8:38 AM

LGTM

This revision is now accepted and ready to land.Feb 24 2017, 8:38 AM
This revision was automatically updated to reflect the committed changes.
hans added inline comments.Feb 24 2017, 10:25 AM
lib/Support/APInt.cpp
624

This is very nit-picky, but I find numBits > 0 much more natural than 0 < numBits.
And I'd suggest splitting up the second assert into two, to make it clearer what went wrong if it fires.

628

For the single-word case we know bitPosition is in the first word, so there's no need for the remainder operation in whichBit. I'd suggest moving the if-statement before computing loBit, and just using bitPosition in the shift.

645

I'm not sure this loop is correct. It will never set Result.pVal[hiWord]?

The only test case I see that hits this loop is EXPECT_EQ(-8388481, i256.extractBits(128, 1).getSExtValue());

In that one, the result is computed from three words, but indeed fits into two words.

But what if it didn't -- what about i256.extractBits(129, 1)?

unittests/ADT/APIntTest.cpp
1441

Would it be possible to use hex constants here and in the expectations somehow? Especially for the last one, it's hard to see what the expected result should be..

Thanks for catching that Hans - I've reverted the commit and will recommit with your recommendations later on.