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

[Serguei Katkov via Phabricator via llvm-commits]

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:
> 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.


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

https://reviews.llvm.org/D54713