[PATCH] D51718: Update MemorySSA in LoopRotate.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/trunk/lib/Transforms/Utils/LoopRotationUtils.cpp:682-687
+  LoopRotate LR(Threshold, LI, TTI, AC, DT, SE, MSSAU, SQ, RotationOnly,
+                IsUtilMode);
+  if (MSSAU && VerifyMemorySSA)
+    MSSAU->getMemorySSA()->verifyMemorySSA();
 
   return LR.processLoop(L);
----------------
asbirlea wrote:
> fhahn wrote:
> > lebedev.ri wrote:
> > > @asbirlea @fhahn 
> > > 
> > > Is this actually correct?
> > > Don't you want to run verification after actually doing the changes?
> > > I.e. shouldn't this be
> > > ```
> > > {
> > >   if (MSSAU && VerifyMemorySSA)
> > >     MSSAU->getMemorySSA()->verifyMemorySSA();
> > >   LoopRotate LR(Threshold, LI, TTI, AC, DT, SE, MSSAU, SQ, RotationOnly,
> > >                 IsUtilMode);
> > >   bool Changed = LR.processLoop(L);
> > >   if (MSSAU && VerifyMemorySSA)
> > >     MSSAU->getMemorySSA()->verifyMemorySSA();
> > >   return Changed;
> > > }
> > > ```
> > Yeah the code in the patch doesn't look right, given that the actual modifications happen in processLoop.
> It's correct, but unnecessary. Thank you for noticing!
> That verification can be removed, since there are additional verificatons inside `LoopRotate::rotateLoop` that should cover the need to verify after the `LR.processLoop(L);` call.
> 
I dropped the verify calls there in 9ea0d8c38fc5


Repository:
  rL LLVM

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

https://reviews.llvm.org/D51718