[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#1715721 <https://reviews.llvm.org/D68930#1715721>, @spatel wrote:

> In D68930#1715680 <https://reviews.llvm.org/D68930#1715680>, @lebedev.ri wrote:
>
> > In D68930#1715645 <https://reviews.llvm.org/D68930#1715645>, @spatel wrote:
> >
> > > In D68930#1715209 <https://reviews.llvm.org/D68930#1715209>, @lebedev.ri wrote:
> > >
> > > > 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.
> > >
> > >
> > > Yes, I understand this patch alone doesn't look like a big change.
> >
> >
> > To recap, the main problem is not that they use InstSimplify,
> >  but that `SimplifyAssociativeBinOp()` uses recursive reasoning, correct?
>
>
> Any time we call into InstSimplify, we may be incurring expensive analysis either by recursion directly or via ValueTracking, so that's part of it. The other part is that we add a few lines in every patch, and then end up with 100+ lines of code to deal with some rare patterns.


Right.

>> In this case before we ever call `SimplifyAddInst()` we need to match 3 instructions - `!= 0` + 2 right shifts.
>>  `reassociateShiftAmtsOfTwoSameDirectionShifts()` for example only requires just 2 instructions,
>>  `foldShiftIntoShiftInAnotherHandOfAndInICmp()` - 4 instructions,
>>  `dropRedundantMaskingOfLeftShiftInput()` - 3/4 instructions.
>> 
>> So *this* case is certainly not worst,
>>  i`m honestly not sure what's re-triggered this disscussion yet again...
> 
> It's the accumulating effect

Certainly true, okay.

> , and I agree it's out-of-scope to beat up on this patch alone, so I won't hold it up. But we should take a step back, do some measurements, and decide how things can be split up.

Any thoughts on the approach i suggested:

> All that being said, if it's really this much of a concern to warrant immediate action,
>  can't we simply expose `"Depth"` in the `llvm::SimplifyAddInst()`
>  and call it with such a depth that either disallows recursive reasoning,
>  or severely limits it? Some fastpaths will likely needed to be added then most likely.
>  That may be the best outcome considering the options..

?

>>> (We'd move the whole reassociateShiftAmtsOfTwoSameDirectionShifts() if we decide to move anything?) 
>>>  But it should be ok. We can assume that regular and aggressive will work together,
>>>  and we can make sure that doesn't break by adding a test within PhaseOrdering.
>>>  Obviously, it's more work to do it that way, so that's probably why we haven't done it often.
>> 
>> To be perfectly honest i don't really see/don't look forward that happening,
>>  because i don't really believe just a single pass of AIC followed by IC will be enough to still handle everything.
>>  i suspect it would need to be IC-AIC-IC-AIC-IC, and right now AIC only runs once.
> 
> Hmm...I don't remember it being that way, but you're right. I thought it was running interleaved with instcombine at some point.

Ah, i see why it was suggested then.
Good that i believe we reached understanding on **why** such move might be troubling..

> After all that, LGTM. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68930