[PATCH] D84413: [DA][SDA] SyncDependenceAnalysis re-write

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

It's interesting that you end up using a modified post-order where loops are kept contiguous, I found the same thing helpful in my work on control flow lowering in AMDGPU.

I haven't looked into the actual divergence propagation, but some early comments on other stuff.



================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:141
+      SmallVector<BasicBlock *, 3> NestedExits;
+      NestedLoop->getUniqueExitBlocks(NestedExits);
+      bool PushedNodes = false;
----------------
This is relatively expensive, isn't it? And it gets executed once per loop exit instead of once per loop...


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:193
+                          VisitedSet &Finalized) {
+  /// \brief Call CallBack on all loop blocks in the modified CFG
+  std::vector<const BasicBlock *> Stack;
----------------
"\brief" comments should be above the function, not inside its body.

How is the CFG modified?


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:223
                                                const LoopInfo &LI)
-    : FuncRPOT(DT.getRoot()->getParent()), DT(DT), PDT(PDT), LI(LI) {}
+    : LoopRPO(), DT(DT), PDT(PDT), LI(LI) {
+  ComputeTopLevelPO(*DT.getRoot()->getParent(), LI, [&](const BasicBlock &BB) {
----------------
The explicit initializer for LoopRPO isn't needed.


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:224-227
+  ComputeTopLevelPO(*DT.getRoot()->getParent(), LI, [&](const BasicBlock &BB) {
+    // errs() << BB.getName() << "\n";
+    LoopRPO.appendBlock(BB);
+  });
----------------
This looks like it's computing a post-order, not a reverse post-order. I haven't looked into how this lines up with the rest of the algorithm, but perhaps you can rename LoopRPO to LoopPO?


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:232
 
 using FunctionRPOT = ReversePostOrderTraversal<const Function *>;
 
----------------
Can this be removed now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84413