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

[Sam Parker via Phabricator via Openmp-commits]

samparker added inline comments.


================
Comment at: llvm/lib/CodeGen/HardwareLoops.cpp:284
+  PA.preserve<DominatorTreeAnalysis>();
+  PA.preserve<LoopAnalysis>();
+  return PA;
----------------
shchenz wrote:
> 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?
Ah, sorry, for some reason I've always thought we can only 'preserve' anything that we directly use!

As for the LPM, I guess my ignorance continues... I don't think I'm using it? I //thought// this is just a port of a function pass into another function pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140982