[PATCH] D65350: [DDG] Data Dependence Graph Basics

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thanks for the docs! Could you clarify the difference between paper and implementation? The implementations looks fine, apart a few nits.



================
Comment at: llvm/docs/DependenceGraphs/DDG.rst:27-31
+As described in [1] the DDG is a hierarchical representation where nodes
+that are part of a strongly connected component of the graph are grouped
+into special nodes called pi-blocks. pi-blocks represent cycles of data
+dependency that prevent reordering transformations. For example, consider
+the following example:
----------------
[clarification needed] AFAICS, this implementation does not allow an arbitrary hierarchical depth; just one layer that allows group instructions into a single node (Could it be called a pi-node?). Could you clarify the difference between paper and implementation?


================
Comment at: llvm/docs/DependenceGraphs/DDG.rst:44
+
+.. image:: cycle.png
+
----------------
The patch currently does not apply with arcanist. I only get 0 byte files for these images.


================
Comment at: llvm/docs/DependenceGraphs/DDG.rst:110
+^^^^^^^^^^^^^^
+  - Builder may be perceived as over engineering at first.
+  - There are some similarities between DDG nodes and edges compared to PDG nodes and edges, but there is little reuse of the class definitions.
----------------
[nit] "[[ https://en.wikipedia.org/wiki/Overengineering | over-engineering ]]" (with dash or as single word)


================
Comment at: llvm/include/llvm/Analysis/DDG.h:113
+  /// Get the first/last instruction in the node.
+  Instruction *getFirstInstruction() const { return getInstructions()[0]; }
+  Instruction *getLastInstruction() const {
----------------
[suggestion] `getInstructions().front()`


================
Comment at: llvm/include/llvm/Analysis/DDG.h:115
+  Instruction *getLastInstruction() const {
+    return getInstructions()[InstList.size() - 1];
+  }
----------------
[suggestion] `getInstructions().back()`


================
Comment at: llvm/include/llvm/Analysis/DDG.h:256
+raw_ostream &operator<<(raw_ostream &OS, const DDGNode &N);
+raw_ostream &operator<<(raw_ostream &OS, const DDGNode::NodeKind K);
+raw_ostream &operator<<(raw_ostream &OS, const DDGEdge &E);
----------------
[nit] Passing a `NodeKind` by `const` has no effect on the declaration.


================
Comment at: llvm/lib/Analysis/DDG.cpp:49-51
+  default:
+    Out = "??";
+    break;
----------------
[suggestion] Explicitly handle the `NodeKind::Unknown` case so the compiler can warn that about a missing switch case if the `NodeKind` is extended.


================
Comment at: llvm/lib/Analysis/DDG.cpp:59
+  OS << "Node Address:" << &N << ":" << N.getKind() << "\n";
+  if (isa<SimpleDDGNode>(&N)) {
+    OS << " Instructions:\n";
----------------
[suggestion] `isa<SimpleDDGNode>(N)` since there is an overload `isa(const Y &Val)`.


================
Comment at: llvm/lib/Analysis/DDG.cpp:61
+    OS << " Instructions:\n";
+    for (auto *I : cast<const SimpleDDGNode>(&N)->getInstructions())
+      OS.indent(2) << *I << "\n";
----------------
[suggestion] `cast<const SimpleDDGNode>(N).getInstructions()`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65350