[PATCH] D45945: [LoopRotate] Fix incorrect SCEV invalidation in loop rotation

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

In https://reviews.llvm.org/D45945#1075786, @chandlerc wrote:

> Or will we lazily rebuild it?


We will lazily rebuild SCEV expressions as needed.  Not great for compile time, but we should be functionally correct.

@mkazantsev:  IMO this illustrates an architectural problem with SCEV's cache -- ideally the cache would parallel the hierarchy of the loop tree itself so that changing an inner loop does not require us to throw away all of the information computed on the outer loop.  Do you have some ideas on how we can fix this architectural issue?



================
Comment at: llvm/trunk/lib/Transforms/Utils/LoopRotationUtils.cpp:262
   // Anything ScalarEvolution may know about this loop or the PHI nodes
-  // in its header will soon be invalidated.
+  // in its header will soon be invalidated, and it can also affect its parent
+  // loops as well.
----------------
Can you please add a comment on *how* we can affect parent loops?  Rotating an inner loop should not change the trip count of outer loops so right now the statement sounds a bit confusing.  Perhaps:  "Loop rotation can change the dominance relationships between blocks in the inner loop and blocks in the outer loop.  It may also delete blocks from the inner loop.  Both of these may end up violating invariants in backedge taken infos cached by SCEV about outer loops."


Repository:
  rL LLVM

https://reviews.llvm.org/D45945