[PATCH] D68930: [InstCombine] Shift amount reassociation in shifty sign bit test (PR43595)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D68930#1715144 <https://reviews.llvm.org/D68930#1715144>, @spatel wrote:

> In D68930#1714878 <https://reviews.llvm.org/D68930#1714878>, @lebedev.ri wrote:
>
> > In D68930#1714856 <https://reviews.llvm.org/D68930#1714856>, @xbolva00 wrote:
> >
> > > >> sadly i'm still seeing regressions (although smaller!) in target benchmark, and analysis of IR points to *this* missing pattern
> > >
> > > But does this or base pattern shows up somewhere else (test suite/clang/chromium) than in your benchmark?
> >
> >
> > It's not "'your benchmark'", that is a real code. I'm not pulling it out of a thin air.
>
>
> I don't think the intent was to disrespect whatever code this pattern showed up in,


Sorry, yes, i think i agree.

> but to ask if there are any stats for common clang benchmarks. That's the only way we can draw a line on these kinds of decisions. Every pattern is presumably showing up somewhere real for someone, so we can't distinguish where it belongs just based on existence.

This doesn't show up in test-suite with no externals, though we have already previously
established that test-suite plain has poor coverage for the code patterns in question..
To be noted, this 'benchmark' is proposed for addition as per rL345166 <https://reviews.llvm.org/rL345166>, so it isn't entirely outlandish to consider it.

To be noted, said library, //to my knowledge//, is only one of two in it's class (camera raw image decoding;
counting only open source libs), with other one using this one, so it's moderately widely used in-the-wild.
Not sure if that counts...

> My opinion (and others have disagreed with this) is that we always want the fold somewhere;
>  it's just a question of where does it go to minimize cost/maximize benefit.

Ignoring the -O3 issue, i'm not really confident AggressiceInstCombine is a great fit for these problems.
The thing is, these folds are not one-off "folds - yay, no fold - shrug".
There isn't much interest in this fold happening in itself.
The really important part is that it will pave the road for other folds to happen.
In particular, this fold will allow e.g. rL373964 <https://reviews.llvm.org/rL373964> to happen.
The situation is likely similar with all other "aggressive" folds i'we added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68930