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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

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

An alternative could be to guard this code (and possibly many more) with:

  if (ExpensiveCombines)

?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68930