[PATCH] D50433: A New Divergence Analysis for LLVM

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

nhaehnle added a comment.

Please do a style pass to capitalize all variable names according to the LLVM coding style.

Also, maybe I missed it, but could you state clearly in a comment what the assumed preconditions are for correctness? And in particular, how do you propose we deal with unstructured loops? This looks like a regression to me at this point.



================
Comment at: include/llvm/Analysis/SyncDependenceAnalysis.h:55
+
+  // the set of blocks which are reachable by disjoin paths from the
+  // loop exits of @loop.
----------------
*disjoint


================
Comment at: include/llvm/Analysis/SyncDependenceAnalysis.h:69-70
+
+  std::map<const Loop *, ConstBlockSet *> cachedLoopExitJoins;
+  std::map<const TerminatorInst *, ConstBlockSet *> cachedBranchJoins;
+};
----------------
Use std::unique_ptr for the ConstBlockSets.


================
Comment at: lib/Analysis/DivergenceAnalysis.cpp:38
 //
-// The divergence analysis identifies the sources of divergence (e.g., special
-// variables that hold the thread ID), and recursively marks variables that are
-// data or sync dependent on a source of divergence as divergent.
+// The DivergenceAnalysis implementation is generic in the sense that it doe
+// not itself identify original sources of divergence.
----------------
*does


================
Comment at: lib/Analysis/DivergenceAnalysis.cpp:145-150
+  const auto *observingLoop = LI.getLoopFor(&observingBlock);
+  for (const auto *loop = LI.getLoopFor(inst->getParent());
+       loop != observingLoop; loop = loop->getParentLoop()) {
+    if (divergentLoops.count(loop))
+      return true;
+  }
----------------
What if observingLoop is a sibling of inst's loop? E.g.:
```
loop(divergent) {
   loop(uniform) {
      val = inst;
   }
   loop {
      use(val);
   }
}
```
As is, the function will incorrectly claim that the use is divergent. If the outer loop is uniform, it'll even crash.

This may not be a problem due to how the function is currently used (and it's private), but then please at least add an assert that observingLoop is an ancestor.


================
Comment at: lib/Analysis/DivergenceAnalysis.cpp:219-222
+      // all PHI nodes of @userBlock become divergent
+      for (auto &blockInst : *userBlock) {
+        if (!isa<PHINode>(blockInst))
+          break;
----------------
Loop over `userBlock->phis()` instead. (Debug values can be mixed with PHIs.)


================
Comment at: lib/Analysis/DivergenceAnalysis.cpp:257-263
+  for (const auto &inst : block) {
+    if (!isa<PHINode>(inst))
+      continue;
+    if (isDivergent(inst))
+      continue;
+    worklist.push_back(&inst);
   }
----------------
Use a loop over `block.phis()` instead, to avoid iterating over the entire block.


================
Comment at: lib/Analysis/DivergenceAnalysis.cpp:299
+
+  // disjoint-paths divergente at @joinBlock
+  markBlockJoinDivergent(joinBlock);
----------------
*divergent


================
Comment at: lib/Analysis/LegacyDivergenceAnalysis.cpp:87
+    cl::desc(
+        "turn the KernelDivergenceAnalysis into a wrapper for GPUDivergenceAnalysis"));
+
----------------
*LegacyDivergenceAnalysis


Repository:
  rL LLVM

https://reviews.llvm.org/D50433