[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]

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