[PATCH] D26699: [BypassSlowDivision] Handle division by constant numerators properly.

[Justin Lebar via llvm-commits]

jlebar added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/BypassSlowDivision.cpp:86-87
 
   if (isa<ConstantInt>(Divisor) ||
       (isa<ConstantInt>(Dividend) && isa<ConstantInt>(Divisor))) {
     // Operations with immediate values should have
----------------
tra wrote:
> This condition appears to be tautological.
> AFAICT, it's equivalent to just if(isa<ConstantInt>(Divisor)).
lol, `if (A || (A && B))`.

I will fix this in a separate patch.


================
Comment at: llvm/lib/Transforms/Utils/BypassSlowDivision.cpp:160
+
+  // We bailed out above that the divisor is not a constant, but the dividend
+  // may still be a constant.  Set OrV to our non-constant operands OR'ed
----------------
tra wrote:
> This does not parse.
> Perhaps it was meant to be "We ensured above" or "We bailed out above if divisor *was* constant".
Fixed, thanks.


================
Comment at: llvm/lib/Transforms/Utils/BypassSlowDivision.cpp:163
+  // together.
+  assert(!isa<ConstantInt>(Divisor));
+
----------------
tra wrote:
> Given that we do bail out on all constant divisors, this assert seems to be a bit redundant.
Well, yes, that's why the assertion is safe?  :)  The intent is to ensure that the behavior above doesn't change without the logic down here also changing.


https://reviews.llvm.org/D26699