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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour marked 2 inline comments as done.
bmahjour added a comment.
Herald added a reviewer: ppc-slack.

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

Kind of. The graph prior to creation of pi-blocks is not a DAG and may have disjoint components. The root node is needed to turn it into a "rooted digraph" making it easier to compute all the strongly connected components. Prior to creating the root, the graph is similar to a forest, but technically speaking it's not a forest because the components may not be trees.

> 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?

In general it would be unsafe to add new nodes after the root is established, but there is an exception. In the next patch I'll be adding pi-blocks which are hierarchical nodes representing strongly-connected components of the graph. Since these components are already reachable by root they are safe to be added after the root is established. Note also that the root node needs to be established **before** pi-blocks can be identified. I'll add an assert for now and a todo to exclude pi-blocks later.

> [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.

If we connect the root node as we add nodes we have two choices: 1) blindly connect the root to every new node 2) compute reachability and avoid creating an edge when the root can already reach the new node.  The first approach obviously creates too many edges, and the second approach is less efficient than the algorithm used here (because we keep track of and ignore visited nodes in this patch). Also the list of nodes will be needed for topological sort of the graph anyway, so it's desirable to keep it.

> [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).

I cannot think of a utility for the bottom node. Even the root node has limited uses as the DDG is mainly used to reason about properties of dependence within a given set of program elements and ignores dependencies outside of that range.



================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:60
+    for (auto I : depth_first_ext(N, Visited))
+      if (I == N)
+        createDefUseEdge(RootNode, *N);
----------------
Meinersbur wrote:
> 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.
Yes it's possible to add redundant edges depending on the order. This algorithm does not result in minimal edges from the root node but it does try to reduce redundant edges to some extent. An algorithm that would result in truly minimal number of edges from the root would be more complicated requiring more cpu cycles. I used this algorithm because it seems to be a reasonable compromise between compute cost and number of edges created. Please let me know if you don't agree with this approach, otherwise I'll add a comment to clarify.





================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:61
+      if (I == N)
+        createDefUseEdge(RootNode, *N);
+  }
----------------
Meinersbur wrote:
> [serious] It's not a def-use chain based dependency.
I will create a dedicated type of edge and add it to the builder interface.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67970