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


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

https://reviews.llvm.org/D54713