[PATCH] D154126: [InstCombine] Transform (A > 0) | (A < 0) -> zext (A != 0) fold

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D154126#4461039 <https://reviews.llvm.org/D154126#4461039>, @goldstein.w.n wrote:

> Thank you for the patch and welcome!
>
> We actually seem to handle this as long as its `(zext (or slt, sgt))`: https://alive2.llvm.org/ce/z/Fpyw7G
> Its only if we have `(or (zext slt), (zext sgt))` that we fail.
>
> I think the best approach we actually be to fold:`(bitwise_logic (zext A), (zext B))` -> `(zext (bitwise_logic A, B))`. That will cover this case any may help other cases.
>
> Edit: Ah I see, we already do that (https://alive2.llvm.org/ce/z/nXUetC), we just mess up the pattern here.

It might also be easier to just update the fold for `(or slt, sgt)` to look through `zext`.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1720
+  ConstantInt *B;
+  ICmpInst::Predicate pred;
+  if (LogicOpc == Instruction::Or &&
----------------
Style `pred` -> `Pred`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1722
+  if (LogicOpc == Instruction::Or &&
+      ((match(Op0, m_LShr(m_Value(A), m_ConstantInt(B))) &&
+        match(Op1, m_ZExt(m_ICmp(pred, m_Specific(A), m_SpecificInt(0))))) ||
----------------
`m_ConstantInt(B)` -> `m_APInt(B)` (and make `B` a `const APInt *`).


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1723
+      ((match(Op0, m_LShr(m_Value(A), m_ConstantInt(B))) &&
+        match(Op1, m_ZExt(m_ICmp(pred, m_Specific(A), m_SpecificInt(0))))) ||
+       (match(Op1, m_LShr(m_Value(A), m_ConstantInt(B))) &&
----------------
`m_SpecificInt(0)` -> `m_Zero()`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1725
+       (match(Op1, m_LShr(m_Value(A), m_ConstantInt(B))) &&
+        match(Op0, m_ZExt(m_ICmp(pred, m_Specific(A), m_SpecificInt(0))))))) {
+    uint64_t X = A->getType()->getIntegerBitWidth();
----------------
Instead of duplicating the matching logic for both orders, can you create a lambda that tries the match on maybe `Lhs` and `Rhs` then just call it with `(Op0, Op1)` and `(Op1, Op0)`.
Another approach is to match `match(&I, m_c_ICmp(<your logic for one of the cases here>))`. (note the `m_c` prefix means commutative i.e try swapping the arg positions).


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1726
+        match(Op0, m_ZExt(m_ICmp(pred, m_Specific(A), m_SpecificInt(0))))))) {
+    uint64_t X = A->getType()->getIntegerBitWidth();
+    if (pred == ICmpInst::ICMP_SGT && B->getValue() == X - 1 &&
----------------
`A->getType()->getScalarBitWidth`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1728
+    if (pred == ICmpInst::ICMP_SGT && B->getValue() == X - 1 &&
+        A->getType()->isIntegerTy()) {
+      Value *cmp = Builder.CreateICmp(
----------------
`A->getType()->isIntegerTy()` is unneeded, this is `ICmp` (IntegerCmp)... I think.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1729
+        A->getType()->isIntegerTy()) {
+      Value *cmp = Builder.CreateICmp(
+          ICmpInst::ICMP_NE, A,
----------------
Style `cmp` variable should be `Cmp`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1731
+          ICmpInst::ICMP_NE, A,
+          Builder.getInt(APInt::getZero(A->getType()->getIntegerBitWidth())));
+      return new ZExtInst(cmp, A->getType());
----------------
`Builder.getInt(APInt::getZero(A->getType()->getIntegerBitWidth()))` -> `Constant::getNullValue(A->getType())`


================
Comment at: llvm/test/Transforms/InstCombine/and-or-icmps.ll:2595
 ; CHECK-NEXT:    ret i64 [[E]]
 ;
   %A = icmp slt i64 %x, 0
----------------
Can you add:

1) A vector test.
2) A few negative tests (maybe `ashr` instead of `lshr`, `slt` instead of `sgt`, incorrect shift amt).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154126