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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Thanks for the feedback guys.



================
Comment at: lib/Support/APInt.cpp:592
+void APInt::insertBits(const APInt &subBits, unsigned bitPosition) {
+  unsigned subBitWidth = subBits.getBitWidth();
+  assert((subBitWidth + bitPosition) <= BitWidth && "Illegal bit insertion");
----------------
hans wrote:
> I suppose subBitWidth can't be zero, but maybe assert that? Otherwise we'd run into trouble with the shifts and so on.
No problem - but tbh many APInt methods would fail with zero BitWidths.


================
Comment at: lib/Support/APInt.cpp:617
+    pVal[loWord] &= ~(mask << loBit);
+    pVal[loWord] |= (subBits.VAL << loBit);
+    return;
----------------
hans wrote:
> 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.
I can merge these no problem, it won't particularly affect readability.


================
Comment at: lib/Support/APInt.cpp:638
+
+  // General case - set/clear individual bits in dst based on src.
+  for (unsigned i = 0; i != subBitWidth; ++i) {
----------------
hans wrote:
> 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?
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.


================
Comment at: unittests/ADT/APIntTest.cpp:1674
+  EXPECT_EQ(48u, i127.countTrailingOnes());
+  EXPECT_EQ(105u, i127.countPopulation());
+
----------------
filcab wrote:
> Why not this?
> ```EXPECT_EQ((i127&UINT64_MAX).getZExtValue(), 0x3456ffffffffffff);
> EXPECT_EQ((i127>>64).getZExtValue(), 0x7ffffffffff8012);```
> (Please double check the values)
OK, although I might actually use the i127.extracteBits().getZExtValue() pattern instead for clarity.


Repository:
  rL LLVM

https://reviews.llvm.org/D30780