[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();
       }
----------------
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.



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

https://reviews.llvm.org/D54713