[PATCH] D54713: [SCEV] Guard movement of insertion point for loop-invariants

[Warren Ristow via Phabricator via llvm-commits]

wristow marked an inline comment as done.
wristow added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:1763
         // position. SCEVExpander must correct for this in all cases.
         InsertPt = &*L->getHeader()->getFirstInsertionPt();
       }
----------------
mkazantsev wrote:
> skatkov wrote:
> > wristow wrote:
> > > wristow wrote:
> > > > skatkov wrote:
> > > > > wristow wrote:
> > > > > > skatkov wrote:
> > > > > > > wristow wrote:
> > > > > > > > skatkov wrote:
> > > > > > > > > wristow wrote:
> > > > > > > > > > skatkov wrote:
> > > > > > > > > > > Should this part be fixed as well?
> > > > > > > > > > It does seem like it should.  But I couldn't come up with a C/C++ test-case that exercised that area.  That, along with the comment about "`even though it's an invalid position`" made me hesitant to touch this area.
> > > > > > > > > > 
> > > > > > > > > > Assuming this part should also be guarded, then since the `SafeToHoist` lambda is depends only on the SCEV `S` itself (rather than it being control-flow-sensitive, for example), we could move the guard up above the `Loop *L` loop that begins on line 1735.  Do you agree?  (Although if we ever intended for `SafeToHoist` to be more sophisticated and analyze specific test-conditions to potentially hoist over, then leaving the checks inside the loop leaves us in a better place for that potential improvement.  Personally, I lean toward moving the `SafeToHoist` guard outside the `Loop *L` loop.)
> > > > > > > > > I guess you cannot find a reproducer that case just because usually there is always a loop preheader.
> > > > > > > > > In other words to reproduce this case you need a pass which uses this utility function when there is no a loop pre-header for the loop. It might be a dead code right now.
> > > > > > > > > 
> > > > > > > > > You can add a logging print to catch a case when this "else" is executed and run bunch of tests. If you find a case when it is triggered you can write a case after that.
> > > > > > > > I think you're right about there always being a loop pre-header when starting from C/C++.  I'll do the experimenting you suggest with a logging-print to see whether that code is exercised in any tests I have.  I haven't tried, but I imagine I could start with a .ll test-case with a loop pre-header, and manually tweak it to not have one (and so then it would exercise that code).  I'll play with that a bit, too.
> > > > > > > It is a good idea to start with an ll file with pre-header and then eliminate it. Be careful with loop passes. They should build LCSSA form first and pre-header will be installed automatically. However loop vectorizer is a functional pass so you should be ok to play with it.
> > > > > > > 
> > > > > > > BTW you can even try the test from this CL. Just eliminate pre-header (do two predecessors for the current loop header) and check what happens.
> > > > > > > Assuming this part should also be guarded, then since the `SafeToHoist` lambda is depends only on the SCEV `S` itself (rather than it being control-flow-sensitive, for example), we could move the guard up above the `Loop *L` loop that begins on line 1735. Do you agree?
> > > > > > 
> > > > > > Actually, that isn't true.  Moving the `SafeToHoist` check above the `Loop *L`  loop as a whole is slightly different semantically.  So I'm abandoning that thought.
> > > > > > 
> > > > > What exactly do you mean by "slightly different semantically"?
> > > > I've experimented with this, and I'm unable to create a test-case that exercises that code when vectoriztion is enabled.
> > > > 
> > > > When I manually create a .ll file with a loop that doesn't have a preheader, and I enable vectorization (via `opt -loop-vectorize`), a preheader is always created in the resulting IR.  Looking into it, I see that the Loop Canonicalization Pass (in "LoopSimplify.cpp") creates the preheader (and other related) blocks.  (As an aside, if I tweak "LoopSimplify.cpp" to disable that pass to experiment with this, then assertions fail due to unexpected block structure (e.g., Entry blocks not dominating Exit blocks).)
> > > > 
> > > > As a couple of side points: If I disable the setting of `InsertPt` in that "loop Preheader is null" case, all the LLVM tests pass.  But on the other hand, that code is not 100% dead, in that I see it gets exercised for three LLVM tests:
> > > >     LLVM :: CodeGen/Hexagon/loop-prefetch.ll
> > > >     LLVM :: CodeGen/PowerPC/loop-data-prefetch-inner.ll
> > > >     LLVM :: CodeGen/PowerPC/ppc64-get-cache-line-size.ll
> > > > 
> > > > None of those tests have vectorizaiton enabled (they all run `llc`, and look for some cache/prefetch-related instruction).
> > > > 
> > > > Looking at the history of when that "loop Preheader is null" code was added, it was done back in revision 147439 (in early 2012).  That commit included adding a new test:
> > > >     llvm/test/Transforms/LoopStrengthReduce/2012-01-02-nopreheader.ll
> > > > 
> > > > but that test-case no longer exercises that block of code, and so of course it passes with it disabled.  (There are a couple other similar tests that were added later, and they also don't exercise that block of code.)
> > > > 
> > > > **The bottom line **is that other than the three prefetch-related tests (which don't enable vectorization), I don't have any test-cases that exercise that code at all, much less demonstrate how/whether it is needed (again, those three prefetch-related tests that do exercise that "loop Preheader is null" code all still pass, even if that code is disabled).  And in particular, it appears to be impossible to create a test with vectorization enabled that exercises that code.  So I'm hesitant to make a change to that code (by guarding it with `SafeToHoist`).  Someone with more experience in this area may be able to come up with a test-case (or justify also guarding it, without a test-case -- or explain why it should not be guarded).  But I think that additional step can be done separately from this patch.
> > > The `while` loop in the non-loop-invariant portion currently gets executed irrespective of whether `SafeToHoist` is true.  If we move the `SafeToHoist` guard above the entire `Loop *L` loop, that `while` loop will not be executed when `SafeToHoist` is false.
> > Well, may be I'm missing something....
> > 
> > SafeToHoist does not depend on L. We can hoist check for SafeToHoist from the for loop.
> > Let's compare the semantic if it returns false:
> > In the new case we will not do anything.
> > In the old case:
> > You will not be able to modify InsertPt if SafeToHoist==false and case when there is no pre-header should be fixed as well.
> > As soon as InsertPt is not changed it is an equal to &*Builder.GetInsertPoint();
> > So the first condition in a while loop you mentioned will be false and while loop will not be executed.
> > 
> > So I do not see any difference. Do I miss anything?
> > 
> > 
> I agree with Serguei that the case of a loop without preheader should also be fixed. I don't see any way how presence of preheader affects the bug being fixed. The simple example that comes to my mind is following:
> 
>   void foo(int x, int y) {
>       for (int i = 0; i < N; i++) {
>         call foo(y); // Executes system.exit if y == 0.
>         // Expansion point
>       }
>   }
> 
> We request SCEV Expander to expand x / y at the expansion point. Provided that both `x` and `y` are loop-invariant, this logic should pick loop header's first instruction as the insertion point. This is incorrect, because if `y == 0`, the execution should not reach the expansion point and we should never divide by zero. If we insert in loop header, this will happen.
> 
> I guess it is hard (or impossible) to write an opt test on a loop which will have no preheader, because in practice all loop passes forcefully create it. You can write a C++ unit test on that. Please find examples at `unittests/Analysis/ScalarEvolutionTest.cpp`.
First, thanks for all that feedback!

About the semantics of moving the guard to outside the loop:
> Well, may be I'm missing something....
> ...
> So the first condition in a while loop you mentioned will be false and while loop will not be executed.
>
> So I do not see any difference. Do I miss anything?

You're absolutely right, Serguei.  I missed the point that the first condition of the while loop is guaranteed to be false if `InsertPt` hasn't previously been modified.  With that, then assuming everyone is happy with also guarding the case without the loop preheader, then we can move the `SafeToHoist` guard out of the loop completely.

Regarding Max's comment:
> I agree with Serguei that the case of a loop without preheader should also be fixed. I don't see any way how presence of preheader affects the bug being fixed. The simple example that comes to my mind is following:
> ...

I definitely agree in principle that the lack of a preheader wouldn't make a dangerous hoist safe.  My concern is that I was unable to create a test-case that didn't have a preheader, and so I couldn't demonstrate what that code was doing.  And more directly, the comment in that block of code:
    // LSR sets the insertion point for AddRec start/step values to the
    // block start to simplify value reuse, even though it's an invalid
    // position. SCEVExpander must correct for this in all cases.

seems to be saying "//`InsertPt` is being set incorrectly here on purpuse, and SCEVExpander is going to fix it//".  That made me uncomfortable that there is some delicate balance happening here.  And since I couldn't come up with a test-case that demonstrates that delicate balance (and since I'm not experienced in this code), I was hesitant to disrupt what might be a delicate balance.  But since you two are experienced in this code, and are not concerned there is a delicate balance being disrupted by also guarding that code, I'm happy to take your advice on that.

To clarify my point of my difficulty of coming up with a test-case, if I take Max's test-case and compile it to a .ll file (at `-O1`), there will be a loop preheader.  It's straightforward to edit it to remove the preheader, but then if I run `opt -loop-vectorize` on it, the Loop Canonicalization Pass in "LoopSimplify.cpp" re-creates the preheader.  So frustratingly for me, that "no loop preheader" block of code //still //doesn't get exercised.

**In short**: I'm definitely fine re-doing this patch to also guard that non-loop-preheader portion, given that you two aren't concerned it's breaking some delicate balance.  And in re-doing it, I'll move the `SafeToHoist` guard outside the loop entirely.


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

https://reviews.llvm.org/D54713