[PATCH] D67970: [DDG] Data Dependence Graph - Root Node

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Do I understand it correctly, that this node is added to avoid having to deal with forests?

Introducing a `createAndConnectRootNode` step creates the problem that if a node is added afterwards, it will not be connected. Could you add an assertion that checks that no nodes are added once the root node is created?

[suggestion] We could create a root node at the graph's construction and connect it when nodes are created. Instead of redundantly storing a list in `DirectedGraph`, we could use that root node's connections to iterate over all nodes.

[suggestion] Another special node could point from ever node in the graph. I have used this to represent dependencies from code before the analyzed region respectively to code after the region. Could it be useful to create such a node es well? (I called them \top and \bottom).



================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:60
+    for (auto I : depth_first_ext(N, Visited))
+      if (I == N)
+        createDefUseEdge(RootNode, *N);
----------------
The algorithm here seems to add redundant edges depending on the iteration order over the node list (for a graph "A -> B", an edge from the root node is added to both if B is visited before A).

Please add some comment lines about how this algorithm works. The way it is written is somewhat confusing. `depth_first_ext` will always visit `N` first if not yet in the visited list. This makes `I = N` is a test for whether `N` has been visited yet.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:61
+      if (I == N)
+        createDefUseEdge(RootNode, *N);
+  }
----------------
[serious] It's not a def-use chain based dependency.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67970