[PATCH] D147597: [InstCombine] Fold icmp(bin(X, Y) | LHS, RHS) --> icmp(bin(X, Y)) iff LHS > RHS s>= 0

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1959
+  // icmp(bin(X, Y) | LHS, C) --> icmp(bin(X, Y)) iff LHS > C s>= 0
+  if (match(Or, m_c_Or(m_BinOp(BO), m_APInt(LHS)))) {
+    if (LHS->isStrictlyPositive() && C.isNonNegative() && LHS->sgt(C)) {
----------------
junaire wrote:
> junaire wrote:
> > goldstein.w.n wrote:
> > > I think the BinOp aspect of the transform is meaningless:
> > > https://alive2.llvm.org/ce/z/cbWvv7
> > > 
> > > Also you're alive2 links still don't match the logic (you are only do `C == 0`).
> > > You can create the constraints you need with `@llvm.assume(i1)`
> > > I tried to implement here: https://alive2.llvm.org/ce/z/rmBJXx
> > > but it doesn't verify.
> > I agree we should remove the binop match, but the alive2 link you provided is not what the patch trying to transform. It should be https://alive2.llvm.org/ce/z/ybKpxy (in tgt we compare with 0, not RHS)
> Sorry, I think the binop match is necessary?
That wouldn't make sense, the binop includes things like `or %x, 0` which is just `%x`.

It verifies for:
`sge`: https://alive2.llvm.org/ce/z/w35F7q
but not for
`sle`: https://alive2.llvm.org/ce/z/2ocRSw


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147597