[PATCH] D30265: [APInt] Add APInt::setBits() method to set all bits in range

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Nice!



================
Comment at: lib/Support/APInt.cpp:582
+      // Set bits are all within the same word, create a [loBit,hiBit) mask.
+      uint64_t mask = ~uint64_t(0ULL);
+      mask >>= (APINT_BITS_PER_WORD - (hiBit - loBit));
----------------
Other parts of the file uses UINT64_MAX instead, which I think is nicer.


================
Comment at: lib/Support/APInt.cpp:591
+      uint64_t loBitMask = maskBit(loBit);
+      uint64_t hiBitMask = maskBit(hiBit - 1);
+      pVal[loWord] |= ~(loBitMask - 1);
----------------
I'd suggest computing the final mask here, instead of starting in this declaration and then doing some more computation when or-ing below.


================
Comment at: lib/Support/APInt.cpp:592
+      uint64_t hiBitMask = maskBit(hiBit - 1);
+      pVal[loWord] |= ~(loBitMask - 1);
+      pVal[hiWord] |= hiBitMask | (hiBitMask - 1);
----------------
This is elegant, but maybe `UINT64_MAX << maskBit(loBit)` is more efficient?


================
Comment at: lib/Support/APInt.cpp:593
+      pVal[loWord] |= ~(loBitMask - 1);
+      pVal[hiWord] |= hiBitMask | (hiBitMask - 1);
+      for (unsigned word = loWord + 1; word != hiWord; ++word)
----------------
Would this be more efficient? `UINT64_MAX >> (APINT_BITS_PER_WORD - maskBit(hiBit))`


================
Comment at: lib/Support/APInt.cpp:594
+      pVal[hiWord] |= hiBitMask | (hiBitMask - 1);
+      for (unsigned word = loWord + 1; word != hiWord; ++word)
+        pVal[word] = ~uint64_t(0ULL);
----------------
I would have gone for `word < hiWord` here, not that it should matter.


================
Comment at: lib/Support/APInt.cpp:595
+      for (unsigned word = loWord + 1; word != hiWord; ++word)
+        pVal[word] = ~uint64_t(0ULL);
+    }
----------------
UINT64_MAX here too.


================
Comment at: unittests/ADT/APIntTest.cpp:204
+  EXPECT_EQ(static_cast<int64_t>((~0ull << 60) | 15), s256.getSExtValue());
 }
 
----------------
I think there's still no test for loBit == hiBit?


Repository:
  rL LLVM

https://reviews.llvm.org/D30265