[PATCH] D110813: [LoopRotate] Forget SCEV values in RewriteUsesOfClonedInstructions

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

>> When rotating a loop it isn't enough to just forget SCEV for that loop nest.

I think this analysis is correct; the expression for phi that's now outside the loop won't be cleared by forgetting the loop

> Btw, I'm not sure if this is enough to keep ScalarEvolution valid throughout LoopRotate. There is for example a comment on line 769 that says "I don't believe this invalidates SCEV." that looks a bit fearsome.

There are probably still other issue lurking unfortunately, same as in other parts of the codebase.



================
Comment at: llvm/lib/Transforms/Utils/LoopRotationUtils.cpp:837
                         AssumptionCache *AC, DominatorTree *DT,
                         ScalarEvolution *SE, MemorySSAUpdater *MSSAU,
                         const SimplifyQuery &SQ, bool RotationOnly = true,
----------------
bjope wrote:
> I think that at least SE could be made a reference instead of a pointer here. At least loop-rotate always fetches ScalarEvolution nowadays (but probably also DominatorTree).
> So we could get rid of the conditional checks if SE is non-null.
> 
> Maybe I'll fix that in a separate patch later. Unless someone beats me to it.
Sounds like a good cleanup!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110813