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

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
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");
----------------
I suppose subBitWidth can't be zero, but maybe assert that? Otherwise we'd run into trouble with the shifts and so on.


================
Comment at: lib/Support/APInt.cpp:597
+  if (subBitWidth == BitWidth) {
+    *this = subBits;
+    return;
----------------
I would assert bitPosition == 0 here, just to make it easier for the reader


================
Comment at: lib/Support/APInt.cpp:617
+    pVal[loWord] &= ~(mask << loBit);
+    pVal[loWord] |= (subBits.VAL << loBit);
+    return;
----------------
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.


================
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) {
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D30780