[PATCH] D141355: [AMDGPU] Add NewPM support to AMDGPUUnifyDivergentExitNodes pass

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUUnifyDivergentExitNodes.cpp:305
+  auto &PDT = getAnalysis<PostDominatorTreeWrapperPass>().getPostDomTree();
+  LegacyDivergenceAnalysis &DA = getAnalysis<LegacyDivergenceAnalysis>();
+  TTI = &getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
----------------
As already noted, this legacy is not related to the pass manager. Just use the same DA in both.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUUnifyDivergentExitNodes.cpp:118-119
 /// XXX - Is there a more efficient way to find this?
-static bool isUniformlyReached(const LegacyDivergenceAnalysis &DA,
-                               BasicBlock &BB) {
+static bool isUniformlyReached(BasicBlock &BB,
+                               std::function<bool(Value *)> IsUniform) {
   SmallVector<BasicBlock *, 8> Stack(predecessors(&BB));
----------------
gandhi21299 wrote:
> arsenm wrote:
> > gandhi21299 wrote:
> > > arsenm wrote:
> > > > gandhi21299 wrote:
> > > > > arsenm wrote:
> > > > > > Not sure why you really need to change this, but the function should probably be a template argument. Is DA just missing a layer to present the PM independent analysis?
> > > > > The LegacyPM version depends on `LegacyDivergenceAnalysis` info whereas the NewPM version of this pass depends on `DivergenceInfo`.
> > > > There should be a common logic DivergenceInfo that both pass versions export
> > > How do these changes look @arsenm ?
> > I'm still confused. LegacyDivergenceAnalysis and DivergenceAnalysis/DivergenceInfo are two *different* analyses. LegacyDivergenceAnalysis is *not* the Legacy pass manager version of DivergenceAnalysis. New pass manager shouldn't invisibly switch the analysis used. I think LegacyDivergenceAnalysis needs to be ported to new PM first
> Ahh I guess I got confused with the names as well. Sure, I will port LegacyDivergenceAnalysis to newPM first.
No! Don't even try. It was considered and discarded long ago.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141355