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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds accepted this revision.
sameerds added a comment.
This revision is now accepted and ready to land.

LGTM, to the extent of my high-level understanding of the analysis. I would recommend waiting a few days and give others a chance to finish the review. I do have some nitpicks, but feel free to decide which ones warrant a change.



================
Comment at: llvm/lib/Analysis/DivergenceAnalysis.cpp:220
 
-    assert(!DivLoop->contains(UserBlock) &&
+    assert(!OuterDivLoop.contains(UserBlock) &&
            "irreducible control flow detected");
----------------
Not necessarily binding on this change, but could this be an error instead? Or perhaps an early exit with a conservative approximation?


================
Comment at: llvm/lib/Analysis/DivergenceAnalysis.cpp:271
+void DivergenceAnalysis::taintAndPushPhiNodes(const BasicBlock &JoinBlock) {
+  LLVM_DEBUG(dbgs() << "taintAndPhiNodes in " << JoinBlock.getName() << "\n");
 
----------------
typo in the debug output: "taintAndPushPhiNodes"


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:116-120
+// The SDA algorithm operates on a modified CFG - we modify the edges leaving
+// loop headers as follows:
+//
+// * We remove all edges leaving all loop headers.
+// * We add additional edges from the loop headers to their exit blocks.
----------------
Is this intended to treat each child loop as a single node when traversing a parent loop? Could we just say that here?


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:132
+// * We want a loop-compact block enumeration, that is the numbers assigned by
+// the traveral to the blocks of a loop are an interval.
+using POCB = std::function<void(const BasicBlock &)>;
----------------
typo: line needs to be indented.


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:143
+static void computeStackPO(BlockStack &Stack, const LoopInfo &LI,
+                           const BasicBlock *LoopHead, Loop *Loop,
+                           POCB CallBack, VisitedSet &Finalized) {
----------------
Is LoopHead always the same as Loop->getHeader()? Also, the complete name LoopHeader is preferred.


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:238
+  computeTopLevelPO(*DT.getRoot()->getParent(), LI, [&](const BasicBlock &BB) {
+    // errs() << BB.getName() << "\n";
+    LoopPO.appendBlock(BB);
----------------
This should either be removed or put in a debug macro


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:253
+
+  // if BlockLabels[IndexOf(B)] == C then C is the dominating definition at
+  // block B if BlockLabels[IndexOf(B)] ~ undef then we haven't seen B yet if
----------------
Did you mean to start each "if" on a new line? Missing markup for an unordered list?


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:284
+  // Push a definition (\p PushedLabel) to \p SuccBlock and return whether this
+  // raises the definition to '\top'
+  bool computeJoin(const BasicBlock &SuccBlock, const BasicBlock &PushedLabel) {
----------------
The final return seems to only update the label. How is \top defined?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84413