[PATCH] D55232: [SCEV] Suppress hoisting insertion point of binops when unsafe

[Warren Ristow via Phabricator via llvm-commits]

wristow added a subscriber: rnk.
wristow added a comment.

Since the callers of `SCEVExpander::InsertBinop` "know" whether the binop being inserted is safe to hoist, this proposed solution of having the caller pass a parameter to indicate whether it's safe seems clean.  But a simpler approach is to special-case the opcode `Instruction::UDiv` in `InsertBinop`.

An even simpler approach is to simply never hoist the insertion-point in `InsertBinop`.  In fact, in the cases I've examined, `SCEVExpander::expand` always did the desired hoisting when it was safe, anyway.  So removing the loop that I'm guarding here with the new parameter `IsSafeToHoist` is equivalent for those test-cases (the check-all tests all passed on x86_64-linux if I simply removed the loop entirely).  I'd be happy to go with this approach of removing the hoisting loop, but I don't know for certain whether the hoisting is always redundant with other optimizations.

To try to understand (and in particular, to try to find a test-case that depended on that hoisting being done), I looked through the history of that hosting loop.  I see it was added more than 8 years ago (r97642):

  Make SCEVExpander and LSR more aggressive about hoisting expressions out
  of loops.

But that revision didn't include any test-cases.  So I'm at a loss to demonstrate whether there are any cases where the code-gen is improved by the hoisting.

In investigating this history, I noticed a couple of other interesting points.

First, the patch here, along with the recently committed patch of:
https://reviews.llvm.org/rL347934

is equivalent to a patch that was committed about a year ago:
https://reviews.llvm.org/rL289412

(As an aside, that year-old commit uses the simpler approach of special-casing the opcode `Instruction::UDiv` in `InsertBinop` to guard the loop, rather than having the caller pass an extra parameter to indicate when it's unsafe to hoist.)

Second, the reason this patch and the recently committed r347934 are needed (even though this was fixed a year ago by r289412), is because that year-old fix was reverted at r289453:

  Reverts r289412. It caused an OOB PHI operand access in instcombine when
  ASan is enabled. Reduction in progress.

@rnk: I wanted to give you a heads up that that problem may re-apear once this patch is reviewed and committed.


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

https://reviews.llvm.org/D55232