[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 09:48:29 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.
In D64285#1580392 <https://reviews.llvm.org/D64285#1580392>, @lebedev.ri wrote:
> In D64285#1580359 <https://reviews.llvm.org/D64285#1580359>, @nikic wrote:
>
> > In D64285#1576787 <https://reviews.llvm.org/D64285#1576787>, @lebedev.ri wrote:
> >
> > > Ugly, yeah, but powerful instsimplify is nice ^^
> >
> >
> > I feel pretty strongly that this is the not the right tradeoff to make here.
>
>
> Unless there's some specific evidence that we benefit from having this in instsimplify,
> reducing code duplication is imho much more valuable than making sure that
> one out of thousands of ad-hoc folds runs in a few more cases than it otherwise would...
>
> Feel free to bring this up as RFC on llvm-dev.
To elaborate, i don't disagree that this is truly a death of a thousand cuts,
*all* these folds are ad-hoc. Some will do wonders for some code patterns,
some will be useless drag for most of the code.
While theoretically we could teach the peep-hole pass all about
every possible pattern, it is infeasible in reality.
(at least until instcombine-SMT is here, will that be a glorious day!).
So what **is** feasible is teaching it about patterns that *were* observed
in real code. This is what i'm trying to do with my patches,
and i'd guess this may be the case with this patch, @xbolva00?
As to the question in the context of this patch, whether to only have the InstCombine part,
or duplicate it in InstSimplify thus not prohibiting other passes from being smarter,
is this a question of code duplication, or redundant matching?
The former can be workarounded with some refactoring to have a single function,
former is still the same death of a thousand cuts and can't be reasonably addressed
within existing framework.
================
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:
> lebedev.ri wrote:
> > 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..
> m_Exact(m_Instruction()) - does not work too.
>
> a) use m_Exact(m_Instruction(DummyInst))
> b) m_Exact(m_BinOp())
>
> ?
Blargh, sorry for leading this in circles.
Then let's go sideways and just do
```
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)) &&
(!cast<Instruction>(FalseVal)->isExact() ||
cast<Instruction>(TrueVal)->isExact()))
```
as this appears to least mad solution *here*.
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