[PATCH] D32415: [APInt] Add ashrInPlace method and rewrite in ashr using it.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Support/APInt.cpp:1029
 /// @brief Arithmetic right-shift function.
-APInt APInt::ashr(const APInt &shiftAmt) const {
-  return ashr((unsigned)shiftAmt.getLimitedValue(BitWidth));
+void APInt::ashrInPlace(const APInt &shiftAmt) {
+  ashrInPlace((unsigned)shiftAmt.getLimitedValue(BitWidth));
----------------
hans wrote:
> Maybe this should be inline since it's so simple?
Possibly. Though both ashrInPlace and getLimitedValue will expand to include slow case calls.

All 3 shifts types have this out of line so I'll look at them together.


================
Comment at: lib/Support/APInt.cpp:1039
+    return;
 
+  // Save the original sign bit for later.
----------------
hans wrote:
> Did you lose the "Invalid shift amount" assert?
It was moved to the inline single word case.


================
Comment at: lib/Support/APInt.cpp:1043
 
-    // Shift the break word. In this case there are no bits from the next word
-    // to include in this word.
-    val[breakWord] = pVal[breakWord+offset] >> wordShift;
-
-    // Deal with sign extension in the break word, and possibly the word before
-    // it.
-    if (isNegative()) {
-      if (wordShift > bitsInWord) {
-        if (breakWord > 0)
-          val[breakWord-1] |=
-            WORD_MAX << (APINT_BITS_PER_WORD - (wordShift - bitsInWord));
-        val[breakWord] |= WORD_MAX;
-      } else
-        val[breakWord] |= WORD_MAX << (bitsInWord - wordShift);
+  // WordShift is the inter-part shift; BitShift is is intra-part shift.
+  unsigned WordShift = ShiftAmt / APINT_BITS_PER_WORD;
----------------
hans wrote:
> nit: "is is intra-part shift"
Seems that tcShiftRight and tcShiftLeft have the same mistake. Thanks!


https://reviews.llvm.org/D32415