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

[Bardia Mahjour via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:426
+      for (NodeType *N : Graph)
+        if (N->hasEdgeTo(EC.first->back().getTargetNode()))
+          ++EC.second;
----------------
Meinersbur wrote:
> [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.
The DirectedGraph implementation does not hold a list of all edges (to save space), so we first need to compute it. Note that in the worst case (where the graph is complete) walking over all edges has O(V^2) complexity, so the whole algorithm will have O(V^2 + V) complexity. 

Note also that the CandidateSrcNodes map is a map from the candidate source nodes (not the target nodes), so we would need another map to store the incoming edge count of all nodes and then cross reference it as we walk CandidateSrcNodes and update it. 

We would need to introduce a map and a list with worst-case space complexities O(V) and O(V^2) respectively. Having said that in the case of complete graphs, we have `max(E) == V` so in comparison with the above algorithm, it would still be an improvement in terms of computation.

If you still think the additional memory complexity increase is justified, I can change it as described.


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

`{(a)->(b), (b)->(c), (c)->(d), (e)->(d)}`

the goal is to turn it into:

`{(a,b,c)->(d), (e)->(d)}`

I think I can get rid of the fixpoint algorithm by making use of the CandidateSrcNodes as a worklist. I'll post an update soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72350