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

Tue Nov 27 23:38:17 PST 2018

skatkov added inline comments.

================
Comment at: llvm/lib/Analysis/ScalarEvolutionExpander.cpp:1763
         // position. SCEVExpander must correct for this in all cases.
         InsertPt = &*L->getHeader()->getFirstInsertionPt();
       }
----------------
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?

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

https://reviews.llvm.org/D54713