[PATCH] D64088: [DDG] DirectedGraph as a base class for various dependence graphs such as DDG and PDG.

[Bardia via Phabricator via llvm-commits]

bmahjour marked 15 inline comments as done.
bmahjour added inline comments.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:118-120
+    if (llvm::find_if(Edges, [&E](const EdgeType *Edge) {
+          return *Edge == E;
+        }) != Edges.end())
----------------
Meinersbur wrote:
> This makes me worry about scaling. Is this method used often? How large is the worst practical edge list? Did you think about using `SetVector`?
> 
> For the sake of avoiding premature optimization, we might worry about it when it becomes a problem.
This is a good question. The method is used fairly frequently when building the graph. I do not have comprehensive stats on the number of nodes and edges and their ratios when building large applications. The space complexity of the DDG depends on a number of factors such as:

1. The number of instructions being analyzed. For example if DDG is run as a function pass it is more likely to result in larger number of nodes (and consequently edges) than if it is run as a loop pass. Of course this also depends on the size of the functions and loop bodies.
2. The quality of the dependence analysis. If dependence analysis gives us pessimistic results we end up creating more edges between nodes.
3. How well we are able to simplify the graph. Sometimes it's possible to collapse an edge and merge two nodes together. The more we can simplify the less nodes and edges we will have.

Using SetVector will likely help with the compile-time performance, but it comes with a memory trade off. My preliminary tests suggest that time-complexity may be more of an issue than memory consumption, so it maybe a good trade off. 

I agree it's better not to do premature optimizations at this point, and instead consider such improvements when more comprehensive stats become available.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:129-134
+    auto IT =
+        llvm::find_if(Edges, [&E](const EdgeType *Edge) { return *Edge == E; });
+    if (IT == Edges.end())
+      return false;
+    Edges.erase(IT);
+    return true;
----------------
Meinersbur wrote:
> Use `std::remove`?
I'll use the erase-remove idiom.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:275-279
+    Src.findEdgesTo(Dst, EL);
+    if (llvm::find_if(EL, [&E](const EdgeType *Edge) { return *Edge == E; }) !=
+        EL.end())
+      return false;
+    Src.addEdge(E);
----------------
Meinersbur wrote:
> This iterates over the list of outgoing edges 3 times. The `find_if` is redundant since `addEdge` already does this.
Good point. I'll remove the find_if part.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:287
+    for (NodeType *N : Nodes)
+      N->clear();
+    Nodes.clear();
----------------
Meinersbur wrote:
> Is clearing each individual node necessary? I wouldn't expect them to be re-used.
Not sure if I understand your question...we need to clear each node so that their outgoing edges are removed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64088