[PATCH] D87957: [LoopReroll][NewPM] Port -loop-reroll to NPM

Alina Sbirlea via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 23 12:17:01 PDT 2020


asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRerollPass.cpp:1726
+  return LoopReroll(&AR.AA, &AR.LI, &AR.SE, &AR.TLI, &AR.DT, true).runOnLoop(&L)
+             ? PreservedAnalyses::none()
+             : PreservedAnalyses::all();
----------------
aeubanks wrote:
> asbirlea wrote:
> > Isn't anything preserved? LCSSA and AA perhaps?
> > AFAICT, getLoopPassPreservedAnalyses() does not apply; DT, LI and ScEv are not preserved.
> Sorry, I didn't notice that the legacy PM's `getLoopAnalysisUsage()` preserved some analyses.
> 
> I'm not super familiar with loop passes, but `getLoopPassPreservedAnalyses()` says 
> ```
> /// Returns the minimum set of Analyses that all loop passes must preserve.
> ```
> so why wouldn't that apply here?
Good point, reading the code it looked like certain changes would affect the CFG and there were no DT or LI updates. Looking further, it looks like only instructions are added/updated, and not the BB, so it looks like we should use `getLoopPassPreservedAnalyses` here.

For the LPM, the above mustPreserveAnalysisID doesn't make sense to me if the pass already requires LCSSA in the `getLoopAnalysisUsage` call. It should always be true.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87957



More information about the llvm-commits mailing list