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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DivergenceAnalysis.h:134
   // Recognized divergent loops
-  DenseSet<const Loop *> DivergentLoops;
+  DenseSet<const Loop *>
+      DivergentLoops; // FIXME Deprecated. For statistics only.
----------------
The comment could be placed on the preceding line to avoid breaking up the declaration.


================
Comment at: llvm/lib/Analysis/DivergenceAnalysis.cpp:178
+
+      if (markDivergent(*Phi))
+        pushUsers(*Phi);
----------------
It looks very strange to call a function on the whole Phi inside a loop that is iterating over its incoming values. This is actually a predicate loop that needs to be converted into a predicate function. Something like the following. Like the coding style says, this forces you to pick a name which documents what is happening.

```
if (!Phi)
  return;
if (!hasInterestingOperands(Phi))
  return;
if (markDivergent(*Phi))
  pushUsers(*Phi);
return;
```


================
Comment at: llvm/lib/Analysis/DivergenceAnalysis.cpp:187
 
-static bool usesLiveOut(const Instruction &I, const Loop *DivLoop) {
-  for (auto &Op : I.operands()) {
----------------
Same as previous comment. The intention of introducing usesLiveOut() as a separate function was to document the loop's intention in the name of the function itself.


================
Comment at: llvm/lib/Analysis/DivergenceAnalysis.cpp:267
+    DivLoop = DivLoop->getParentLoop();
+    if (!DivLoop || DivLoop->contains(&DivExit))
+      break;
----------------
Loop::Contains is called with the same argument in each iteration. If LoopInfo is available at this point, the function could call LoopInfo::getLoopFor() just once outside the current while-loop and then compare pointers at this point.


================
Comment at: llvm/lib/Analysis/SyncDependenceAnalysis.cpp:101
 //===----------------------------------------------------------------------===//
+#include "llvm/Analysis/SyncDependenceAnalysis.h"
 #include "llvm/ADT/PostOrderIterator.h"
----------------
This got reordered. Might be a good idea to clang-format the whole patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84413