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

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

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.

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

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

After all that, LGTM. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68930