[PATCH] D64285: [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y))

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D64285#1580922 <https://reviews.llvm.org/D64285#1580922>, @lebedev.ri wrote:

> 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.


I think my comment came across more negative than intended. I think this is a useful patch (e.g. I remember seeing this pattern reported in https://github.com/rust-lang/rust/issues/58083). What I disagree with is the "premature compiler optimization" that is sometimes happening. The original motivation for this patch is very likely fully addressed by just having the InstCombine portion of this patch. But when new combines are implemented, it often happens that their scope is extended beyond what we know to be necessary, into more speculative territory.

Often these extensions are beneficial, for example because it is better to handle a more general pattern than some specific cases, or because a more general pattern naturally falls out of the matching mechanisms we use (typical example: splat vectors). But in some cases these extensions seem to happen more "because we can" than out of a reasonable expectation of utility.

In this specific case, yes, technically we could get an additional improvement by having both the instsimplify and instcombine folds, but it comes at the cost of both duplicate code and duplicate matching overhead. In such cases, a value judgement has to be made that may differ case-by-case. Is the additional complexity and compile-time cost worth the additional optimization opportunities? In this specific case, I don't think so, as it is rather unlikely the inststimplify fold will trigger in a context where the instcombine fold would not. But I understand that this is something two reasonable people may disagree on ;)

So to answer your question, it's about both code duplication and redundant matching. And while the death by a thousand cuts may be unavoidable, we should still try to not hasten it along unduly...


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

https://reviews.llvm.org/D64285