[PATCH] D28200: [BypassSlowDivision] Do not bypass division of hash-like values

Justin Lebar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Dec 31 10:20:30 PST 2016


jlebar added inline comments.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:78
 
+/// \brief Check if the value looks like a hash.
+///
----------------
s/the value/a value/


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:88
+/// And even if we are wrong and the value is not a hash, generally, it is still
+/// quite unlikely for such values to fit the shorter type.
+///
----------------
Suggest `s/quite//` and `s/generally//` -- we weaken it with "generally" then strengthen it with "quite", and then what do we mean exactly?  :)


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:88
+/// And even if we are wrong and the value is not a hash, generally, it is still
+/// quite unlikely for such values to fit the shorter type.
+///
----------------
jlebar wrote:
> Suggest `s/quite//` and `s/generally//` -- we weaken it with "generally" then strengthen it with "quite", and then what do we mean exactly?  :)
> unlikely that such values will fit into the shorter type

(If you want to use "for", you could use "uncommon for such values to fit into the shorter type", but I think that's less good here.)


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:105
+  case Instruction::Mul: {
+    // At this point long constants may be moved out of loops (as bitcasts) by
+    // Constant Hoisting. So, an additional check is needed to find out if the
----------------
s/At this point//


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:106
+    // At this point long constants may be moved out of loops (as bitcasts) by
+    // Constant Hoisting. So, an additional check is needed to find out if the
+    // operand is a constant actually.
----------------
No comma needed after "So" here.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:106
+    // At this point long constants may be moved out of loops (as bitcasts) by
+    // Constant Hoisting. So, an additional check is needed to find out if the
+    // operand is a constant actually.
----------------
jlebar wrote:
> No comma needed after "So" here.
I don't actually understand what you mean by the first sentence.  Can you try to rephrase wrt what the bitcast has to do with constant hoisting?


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:107
+    // Constant Hoisting. So, an additional check is needed to find out if the
+    // operand is a constant actually.
+    Value *Op1 = I->getOperand(1);
----------------
actually a constant


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:125
+  default:
+    break;
+  }
----------------
We don't need this case?


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:157
+  // values that are unlikely to fit the shorter type (including hashes) and
+  // return VALRNG_LONG for them to disable bypassing.
+  if (isHashLikeValue(Op, shortLen))
----------------
I'd just get rid of the last sentence entirely -- it's clear what's going on.


================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:159
+  if (isHashLikeValue(Op, shortLen))
+    return VALRNG_LONG;
+
----------------
We only want to do this check for the dividend, right?  I mean, I guess it doesn't really hurt to do it for the divisor too, but that isn't motivated by hashtables.


================
Comment at: test/CodeGen/X86/bypass-slow-division-64.ll:138
+; CHECK-NOT: divl
+  %c = mul i64 %a, 5229553307 ; the constant doesn't fit 32 bits
+  %res = urem i64 %c, %l
----------------
Can we test with a constant that *does* fit into 32 bits?


https://reviews.llvm.org/D28200





More information about the llvm-commits mailing list