[PATCH] D154126: [InstCombine] Transform (A > 0) | (A < 0) -> zext (A != 0) fold
Noah Goldstein via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 29 11:53:47 PDT 2023
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
More information about the llvm-commits
mailing list