[PATCH] D64285: [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y))
Roman Lebedev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 11 07:11:16 PDT 2019
- Previous message: [PATCH] D64285: [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y))
- Next message: [PATCH] D64285: [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y))
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
lebedev.ri added a comment.
It'd be really great to have at just least one positive non-splat-vector test for instsimplify and instcombine for each predicate (i.e. 4 more tests?)
================
Comment at: lib/Analysis/InstructionSimplify.cpp:92-93
+ match(CmpLHS, m_Specific(X))) {
+ if (!cast<BinaryOperator>(TrueVal)->isExact() &&
+ cast<BinaryOperator>(FalseVal)->isExact())
+ return nullptr;
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > lebedev.ri wrote:
> > > lebedev.ri wrote:
> > > > ```
> > > > /// If either the `ashr` is non-`exact`, or `lshr` is exact too, then we can just return the `ashr` instruction.
> > > > ```
> > > > i think it may be less confusing to flip the `if` accordingly.
> > > Hmm, also, as i've recently seen, `cast<BinaryOperator>` is going to break for constant expressions.
> > > Let's just do the most obvious thing instead:
> > > ```
> > > if (match(TrueVal, m_LShr(m_Value(X), m_Value(Y))) &&
> > > match(FalseVal, m_AShr(m_Specific(X), m_Specific(Y))) &&
> > > match(CmpLHS, m_Specific(X)) &&
> > > (!match(FalseVal, m_Exact()) || match(TrueVal, m_Exact())))
> > > ```
> > m_Exact() not exists?
> m_Exact(m_BinOp()).. okay
Ugh, right, sorry.
Though maybe `m_Instruction()`, not `m_BinOp()`; this really asks for `m_Anything()` matcher..
================
Comment at: lib/Analysis/InstructionSimplify.cpp:82-84
+ if ((Pred != ICmpInst::ICMP_SGT || !match(CmpRHS, m_APInt(C)) ||
+ !C->sge(-1)) &&
+ (Pred != ICmpInst::ICMP_SLT || !match(CmpRHS, m_APInt(C)) || !C->sge(0)))
----------------
This is not going to work with non-splat vector constants, although it is trivial to make it,
i'd recommend to do this instead:
```
unsigned Bitwidth = CmpRHS->getType()->getScalarSizeInBits();
if ((Pred != ICmpInst::ICMP_SGT ||
!match(CmpRHS,
m_SpecificInt_ICMP(ICmpInst::ICMP_SGE, APInt(Bitwidth, -1)))) &&
(Pred != ICmpInst::ICMP_SLT ||
!match(CmpRHS,
m_SpecificInt_ICMP(ICmpInst::ICMP_SGE, APInt(Bitwidth, 0))))) {
```
(`m_SpecificInt_ICMP()` is really fresh)
================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:548-553
+ const APInt *C;
+ if ((Pred != ICmpInst::ICMP_SGT || !match(CmpRHS, m_APInt(C)) ||
+ !C->sge(-1)) &&
+ (Pred != ICmpInst::ICMP_SLT || !match(CmpRHS, m_APInt(C)) || !C->sge(0)))
+ return nullptr;
+
----------------
Same remark about constant
================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:561
+ match(CmpLHS, m_Specific(X))) {
+ const auto *Ashr = cast<BinaryOperator>(FalseVal);
+ // This new ashr must not be exact.
----------------
Likewise, i think it's sufficient to cast to `Instruction`..
================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:564
+ return Builder.CreateAShr(Ashr->getOperand(0), Ashr->getOperand(1),
+ Ashr->getName(), /*isExact=*/false);
+ }
----------------
nice!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64285/new/
https://reviews.llvm.org/D64285
- Previous message: [PATCH] D64285: [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y))
- Next message: [PATCH] D64285: [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y))
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
More information about the llvm-commits
mailing list