[PATCH] D130039: [InstCombine] Improve folding of mul + icmp

[Sanjay Patel via Phabricator via llvm-commits]

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

LGTM - see inline for a couple of minor points.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2032
+    }
+    if (Pred == ICmpInst::ICMP_SLT || Pred == ICmpInst::ICMP_SGE)
+      NewC = ConstantInt::get(
----------------
alexander-shaposhnikov wrote:
> spatel wrote:
> > With icmp predicate canonicalization, I don't think we need to handle SGE/SLE/ULE/UGE. You could assert that those predicates are not present at this point.
> Adding an assert makes bootstrapped Clang crash (with the newly added assertion failure).
> The crash doesn't happen immediately but on a few inputs, so it looks like they are not fully canonicalized at the moment. 
> What would you say  to adding a TODO/FIXME here (to remove  SGE/SLE/ULE/UGE + add assert) ?
> (we can file a separate issue)
> 
Ok - that's surprising to me. It would be good to reduce a test from clang to see how we end up with an icmp with *-or-equal pred and a constant operand 1.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-mul.ll:859
 
 ; Negative test - the 33rd bit could be set.
 
----------------
Update comment - something like:
; The 33rd bit can only be set when MSB of x is set.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130039/new/

https://reviews.llvm.org/D130039