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

[Hans Wennborg via Phabricator via llvm-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

lgtm with some comments



================
Comment at: include/llvm/ADT/APInt.h:911
+      if (ShiftAmt == BitWidth)
+        VAL = SExtVAL >> (APINT_BITS_PER_WORD - 1); // undefined
+      else
----------------
What does the "// undefined" comment mean? That we don't really define what happens when ShiftAmt == BitWidth but we try to do something reasonable? Maybe spell that out, I hadn't seen that before.


================
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));
----------------
Maybe this should be inline since it's so simple?


================
Comment at: lib/Support/APInt.cpp:1039
+    return;
 
+  // Save the original sign bit for later.
----------------
Did you lose the "Invalid shift amount" assert?


================
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;
----------------
nit: "is is intra-part shift"


================
Comment at: lib/Support/APInt.cpp:1051
+    pVal[getNumWords()-1] = SignExtend64(pVal[getNumWords()-1],
+                                      ((BitWidth-1) % APINT_BITS_PER_WORD) + 1);
+
----------------
Maybe run clang-format on this line? There should usually be spaces around the '-' operator for example.


================
Comment at: lib/Support/APInt.cpp:1060
+        pVal[i] = (pVal[i + WordShift] >> BitShift) |
+                  (pVal[i + WordShift + 1] << (APINT_BITS_PER_WORD-BitShift));
+
----------------
nit: spaces around the `-` here too


https://reviews.llvm.org/D32415