[PATCH] D140850: [Patch 2/4]: Add optimizations for icmp eq/ne (mul(X, Y), 0)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Proofs:

Both odd: https://alive2.llvm.org/ce/z/9qgwMo
One odd: https://alive2.llvm.org/ce/z/vRqUQO
Non-zero nuw: https://alive2.llvm.org/ce/z/3Bqx2-
Non-zero nsw: https://alive2.llvm.org/ce/z/AybG6r

>From the test diffs, I don't see any cases where we actually hit the true/false case. Presumably, this get's reliably handled by the "icmp eq isKnownNonZero, 0" fold in InstSimplify. If that's the case, we can omit handling for that.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1314
+    KnownBits YKnown = computeKnownBits(Y, 0, &Cmp);
+    bool XExtra = XKnown.countMaxTrailingZeros() == 0;
+    bool YExtra = YKnown.countMaxTrailingZeros() == 0;
----------------
YExtra -> YOdd if I understand the intention correctly.

Edit: Hm, or is the naming like this because it's odd in one case and non-zero in the other?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1319
+      // transformation using overflow information and zero/non-status
+      BinaryOperator *BO0 = dyn_cast<BinaryOperator>(Cmp.getOperand(0));
+      if (BO0 && (BO0->hasNoUnsignedWrap() || BO0->hasNoSignedWrap())) {
----------------
`cast<OverflowingBinaryOperator>` would always work (including for mul constant expressions).


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1323
+          XExtra = llvm::isKnownNonZero(X, DL, 0, Q.AC, Q.CxtI, Q.DT);
+          YExtra = llvm::isKnownNonZero(Y, DL, 0, Q.AC, Q.CxtI, Q.DT);
+       }
----------------
`llvm::` shouldn't be needed.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:1332
+                 : replaceInstUsesWith(Cmp,
+                                       ConstantInt::getTrue(Cmp.getType()));
+
----------------
Can use `getBool(Cmp.Type(), Pred == ICmpInst::ICMP_NE)` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140850