[PATCH] D141355: [AMDGPUUnifyDivergentExitNodes] Add NewPM support

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUUnifyDivergentExitNodes.cpp:162
 
-bool AMDGPUUnifyDivergentExitNodes::runOnFunction(Function &F) {
-  DominatorTree *DT = nullptr;
-  if (RequireAndPreserveDomTree)
-    DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
-
-  auto &PDT = getAnalysis<PostDominatorTreeWrapperPass>().getPostDomTree();
-  if (PDT.root_size() == 0 ||
-      (PDT.root_size() == 1 &&
-       !isa<BranchInst>(PDT.getRoot()->getTerminator())))
+bool AMDGPUUnifyDivergentExitNodesImpl::run(Function &F, DominatorTree *DT,
+                                            PostDominatorTree *PDT,
----------------
Either declare parameters as references, or implement checks for nullptr in the function body. Also, can any of these be const?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUUnifyDivergentExitNodes.cpp:307-308
+    DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
+  auto *PDT = &getAnalysis<PostDominatorTreeWrapperPass>().getPostDomTree();
+  auto *UA = &getAnalysis<UniformityInfoWrapperPass>().getUniformityInfo();
+  auto *TranformInfo = &getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
----------------
If you know that a variable cannot be nullptr, it is preferable to declare it as a reference rather than a pointer. It can be converted into a pointer at the point where some function expects a pointer. For example, the call below can be "whatever.run(&DT, &PDT, UA)".


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUUnifyDivergentExitNodes.cpp:190-192
-  if (PDT.root_size() == 0 ||
-      (PDT.root_size() == 1 &&
-       !isa<BranchInst>(PDT.getRoot()->getTerminator())))
----------------
ruiling wrote:
> I think you just removed this accidentally.
Was this comment addressed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141355