This is an archive of the discontinued LLVM Phabricator instance.

[APInt] Add APInt::insertBits() method to insert an APInt into a larger APInt
ClosedPublic

Authored by RKSimon on Mar 9 2017, 5:52 AM.

Details

Summary

We currently have to insert bits via a temporary variable of the same size as the target with various shift/mask stages, resulting in further temporary variables, all of which require the allocation of memory for large APInts (MaskSizeInBits > 64).

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

This patch adds the APInt::insertBits() helper method which avoids the temporary memory allocation and masks/inserts the raw bits directly into the target.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Mar 9 2017, 5:52 AM
filcab accepted this revision.Mar 9 2017, 7:08 AM

LGTM, but maybe wait a bit to let other people chime in.
Thank you!

unittests/ADT/APIntTest.cpp
1674

Why not this?

EXPECT_EQ((i127&UINT64_MAX).getZExtValue(), 0x3456ffffffffffff);
EXPECT_EQ((i127>>64).getZExtValue(), 0x7ffffffffff8012);

(Please double check the values)

This revision is now accepted and ready to land.Mar 9 2017, 7:08 AM
hans added inline comments.Mar 9 2017, 2:19 PM
lib/Support/APInt.cpp
592

I suppose subBitWidth can't be zero, but maybe assert that? Otherwise we'd run into trouble with the shifts and so on.

597

I would assert bitPosition == 0 here, just to make it easier for the reader

617

This code is the same as for the isSingleWord() case except for pVal[loWord] instead of VAL.

If you really wanted to unify the two, we could use getRawData()[loWord] & = instead.

Not sure if it's worth it though.

638

It seems we could do better. Can't we do something similar to setBitsSlowCase, so we process word-by-word even when bitPosition isn't aligned?

Thanks for the feedback guys.

lib/Support/APInt.cpp
592

No problem - but tbh many APInt methods would fail with zero BitWidths.

617

I can merge these no problem, it won't particularly affect readability.

638

I'll add a TODO - there is scope for improvement here, but I've been struggling to find use cases. All the places I've converted to insertBits have been on who byte boundaries or better, and none actually cross multiple words - all other cases are current only in the unit tests. If more appear I'll gladly revisit this.

unittests/ADT/APIntTest.cpp
1674

OK, although I might actually use the i127.extracteBits().getZExtValue() pattern instead for clarity.

This revision was automatically updated to reflect the committed changes.