[Openmp-commits] [PATCH] D140982: [HardwareLoops] NewPM support

[ChenZheng via Phabricator via Openmp-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/CodeGen/HardwareLoops.cpp:284
+  PA.preserve<DominatorTreeAnalysis>();
+  PA.preserve<LoopAnalysis>();
+  return PA;
----------------
samparker wrote:
> shchenz wrote:
> > Seems this is an issue from LPM, but hardware loop insertion pass should preserve some other analysis as well, like `ScalarEvolutionAnalysis`, `BranchProbabilityAnalysis`, `DependenceAnalysis` and `MemorySSAAnalysis` (although there is no handling for it now, we may need pass it to `InsertPreheaderForLoop` if want to preserve this analysis)? I saw some other pass that calls `InsertPreheaderForLoop` but declare to preserve these analysis.
> I can't say I understand the LPM, but we're not using it here, nor are we using MemorySSA or BranchProbability. So could you please explain why/what you'd like to see change here?
I think the pass should try its best to declare the preservation of analysis passes to avoid the analysis passes are run again unnecessarily. If we don't change BranchProbability or some other analysis pass results, we should better to declare it as preserved, otherwise later pass that depends on this analysis will have to rerun the analysis pass again? I think this is the reason why we return `PreservedAnalyses::all()` if the pass does not do any transformation.

I said the LPM because in the legacy pass implementation of hardware loop pass, it also preserves these two analysis `DominatorTreeAnalysis` and `LoopAnalysis`, so I thought you were implementing the same thing in NPM with the LPM?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140982