[PATCH] D32114: [APInt] Merge the multiword code from lshrInPlace and tcShiftRight into a single implementation

[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 a naming suggestion

Are there tests covering this? I couldn't find any direct tests, but are there at least indirect ones that would break if for example you change the function to not shift at all?

Do we have the same situation with tcShiftLeft and shlSlowCase? I suppose it's not quite the same since shlSlowCase isn't in-place, but maybe there's some code to be shared anyway?



================
Comment at: lib/Support/APInt.cpp:2690
 
 /* Shift a bignum right COUNT bits in-place.  Shifted in bits are
    zero.  There are no restrictions on COUNT.  */
----------------
Hmm, seems it's spelled COUNT like this in many other parts of the file, but I don't think we do that generally in LLVM; I'd prefer Count. Maybe not worth changing now :-/


================
Comment at: lib/Support/APInt.cpp:2697
 
-      if (i + jump >= parts) {
-        part = 0;
-      } else {
-        part = dst[i + jump];
-        if (shift) {
-          part >>= shift;
-          if (i + jump + 1 < parts)
-            part |= dst[i + jump + 1] << (APINT_BITS_PER_WORD - shift);
-        }
-      }
+  /* Jump is the inter-part jump; Shift is is intra-part shift.  */
+  unsigned Jump = std::min(Count / APINT_BITS_PER_WORD, Words);
----------------
Use // instead of /*


================
Comment at: lib/Support/APInt.cpp:2698
+  /* Jump is the inter-part jump; Shift is is intra-part shift.  */
+  unsigned Jump = std::min(Count / APINT_BITS_PER_WORD, Words);
+  unsigned Shift = Count % APINT_BITS_PER_WORD;
----------------
Actually, Jump doesn't seem like a great name. Maybe we could have WordShift and BitShift?


https://reviews.llvm.org/D32114