[PATCH] D105723: [LSR] Do not hoist IV if it is not post increment case. PR43678

Wed Jul 14 22:50:09 PDT 2021

skatkov added a comment.

In D105723#2878473 <https://reviews.llvm.org/D105723#2878473>, @reames wrote:

> I spent way too much time looking at this on Tuesday.  Let me summarize my findings.
>
> The basic issue we're running into is that while attempting to expand an LSR formula corresponding to an operand of an IV increment, SCEVExpander goes looking for a profitable expansion in terms of existing IVs.  While doing that, it tries to optimize the IVs it finds by hoisting expressions involved in those IVs (to the IVIncInsertPt, but which point doesn't really matter).  This movement is itself entirely legal, but it creates a very surprising effect.  In this case, the IV being hoisted is exactly the one that LSR was in the middle of expanding a formula for an operand of.
>
> This results in the very surprising result that calling LSRInstance::Expand on an operand of some instruction can result in a Value being returned which does not dominate that instruction.  It dominates the location that instruction *used to be at*, but the instruction itself may have itself been hoisted.  LSR clearly does not expect this.
>
> SCEVExpander has a very delicate interlock with LSR.  LSR puts the expander into both a "literal mode", and a special "LSR mode".  LSR expects to be able to cache information about SCEVs and instructions, and have only it's own rewrites invalidate them.  This seems like a case where SCEVExpander is violating the implicit contract.
>
> Note that nothing in the description above is specific to post-increment IVs.  I think the fact we're happening to see this with post-inc is an artifact.  Or at least, I don't currently understand why pre-incs couldn't see the same issue.
>
> In this case, simply disabling hoisting entirely in LSRMode appears to work, and doesn't appear to cause any regressions in the test suite.  On the surface, that seems like a reasonable fix.
>
> The point I got stuck was trying to reason about how having SCEVExpander return a Value for the operand of some instruction which no longer dominates said instruction doesn't break users other than LSR as well.  I'd just started thinking that through when I got distracted and haven't come to a conclusion on that question yet.
>
> In terms of incrementalism, I'd suggest revising this patch to simply disable hoisting in LSRMode.  Doing that appears to work, is at least slightly less narrow in fixing a symptom, and is a reasonable step forward.  I'd LGTM that.  I could also maybe be convinced to LGTM the current patch if you can make a good argument for why this is actually specific to post-inc IVs.

Hi Philip, first of all thank you to looking to this as well and spending time to git this issue.
I completely agree with your root cause analysis. Thank you for writing this in clear manner.

WRT post-increment - digging.... My feeling is that with post-inc we can get such case. I'' try to write a test with post-inc or prove that it is impossible.
What worries me in disabling hoisting in LSR mode in SCEV expander is that it hoist under check LSR mode. So this was done intentionally...

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

https://reviews.llvm.org/D105723