[PATCH] D72350: [DDG] Data Dependence Graph - Graph Simplification

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:394
+  /// the consecutive instructions after merging belong to the same basic block.
+  bool nodesAreMergeable(const DDGNode &Src,
+                         const DDGNode &Tgt) const final override;
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | Function names should start with the verb ]]: `areMergableNodes`


================
Comment at: llvm/lib/Analysis/DDG.cpp:278-281
+  if (SimpleSrc->getLastInstruction()->getParent() !=
+      SimpleTgt->getFirstInstruction()->getParent())
+    return false;
+  return true;
----------------
[nit] Looks line an [[ https://knowthecode.io/smelly-code-ifreturn-truefalse-code-pattern |  anti-pattern ]]. Did you consider
```
return SimpleSrc->getLastInstruction()->getParent() == SimpleTgt->getFirstInstruction()->getParent();
```
or add an empty line before `return true` to make it distinct as the "all tests passed" action?


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:426
+      for (NodeType *N : Graph)
+        if (N->hasEdgeTo(EC.first->back().getTargetNode()))
+          ++EC.second;
----------------
[serious] This is an O(V^2*max(E)) worst case algorithm (`hasEdgeTo` is a linear search) reinvoked over each fixpoint round, i.e. potentially quadratic (maybe less because of the `EC.second > 1` bailout and fixpoint probably not linear over node count). To count the number of incoming edges, iterate over all edges, lookup the target node in `CandidateSrcNodes` and increase its counter. That's just O(E). To further reduce computation time, `CandidateSrcNodes` could be updated when nodes are merged instead of recomputing it.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:455
+    ++TotalSimplificationRounds;
+  } while (DidSomething);
+  LLVM_DEBUG(dbgs() << "=== End of Graph Simplification ===\n");
----------------
I wonder whether a fixpoint is even necessary. In what cases would we need a second round?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72350