[PATCH] Fix APInt division algorithm

Yaron Keren yaron.keren at gmail.com
Thu Mar 26 01:07:53 PDT 2015


Minor nitpicks, LGTM.


REPOSITORY
  rL LLVM

================
Comment at: lib/Support/APInt.cpp:1510
@@ -1512,1 +1509,3 @@
+  // b denotes the base of the number system. In our case b is 2^32.
+  static const uint64_t b = uint64_t(1) << 32;
 
----------------
Could b be LLVM_CONSTEXPR ?


================
Comment at: lib/Support/APInt.cpp:1583
@@ -1582,3 +1582,3 @@
     for (unsigned i = 0; i < n; ++i) {
-      uint64_t u_tmp = uint64_t(u[j+i]) | (uint64_t(u[j+i+1]) << 32);
+      uint64_t u_tmp = (uint64_t(u[j+i+1]) << 32) | uint64_t(u[j+i]);
       uint64_t subtrahend = uint64_t(qp) * uint64_t(v[i]);
----------------
The parenthesis around the shift make the expression more readable, even if not required.

You flipped the | arguments so they would consistent with the usual u[j+n], u[j+n-1]...u[j] representation?


================
Comment at: lib/Support/APInt.cpp:1784
@@ -1800,3 +1783,3 @@
     // order words.
-    if (lhsWords == 1) {
+    if (lhsWords == 1) { // FIXME: Probably a dead case, lshWords is never 1
       uint64_t tmp =
----------------
I'd remove the FIXME and explain that *currently* this is a dead case since all callers in APInt calls either test for divide() with lhswords == rhswords == 1 or proivde the a small radix before calling divide() but it is worth keeping this code alive for correctness in case some new code does call divide() with lhswords == rhswords == 1.

http://reviews.llvm.org/D8448

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list