[PATCH] D50479: Expose CFG Update struct. Define GraphTraits to get children given a snapshot CFG.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked 5 inline comments as done.
asbirlea added inline comments.


================
Comment at: include/llvm/IR/CFGDiff.h:60
+  GraphDiff() {}
+  GraphDiff(ArrayRef<cfg::Update<NodePtr>> Updates, bool InverseGraph = false) {
+    SmallVector<cfg::Update<NodePtr>, 4> LegalizedUpdates;
----------------
kuhar wrote:
> timshen wrote:
> > kuhar wrote:
> > > It's not clear to me what InverseGraph means here.
> > I imagine that if InverseGraph, then To is a predecessor of From. If not InverseGraph, To is a successor of From. Maybe you want to document that?
> What confuses me is it's already possible to Pass Inverse<BB*> as the template arguments, so it's not clear if InverseGraph is for the graph, the updates, or if it means to swap insertions and deletions of edges.
> 
> It would be really helpful to add a comment about it above.
Let me try to clarify what I had in mind and please let me know if this makes sense in terms of generality. There is also a comment about this below which I'd be happy to move around and expand to give clarity.

There are 2 boolean values used consistently throughout this file: InverseGraph and InverseEdge. InverseGraph refers to having a normal graph, or viewing the graph in reverse, which is analogous with reverse-applying updates.
InverseEdge refers to the direction of the edges you're looking for. For a non-inversed graph, that's naturally successors when InverseEdge is false and predecessors when InverseEdge is true.

Below there are two classes that call into GraphDiff:
CFGViewSuccessors and CFGViewPredecessors, both can be parametrized to consider the graph inverted or not (i.e. InverseGraph). Successors implicitly has InverseEdge = false and Predecessors implicitly has InverseEdge = true (see calls to GraphDiff methods in there).

In the GraphTraits instantiations:
- `GraphDiff<BasicBlock *>`  is equivalent to `InverseGraph = false`
- `GraphDiff<Inverse<BasicBlock *>>` is equivalent to `InverseGraph = true`
- second pair item is `BasicBlock *`, then `InverseEdge = false`
- second pair item is `Inverse<BasicBlock *>`, then `InverseEdge = true`

The 4 GraphTraits are as follows:
1. `std::pair<const GraphDiff<BasicBlock *> *, BasicBlock *>> : CFGViewSuccessors<false>`
Regular CFG, children means successors, InverseGraph = false, InverseEdge = false.
2. `std::pair<const GraphDiff<Inverse<BasicBlock *>> *, BasicBlock *>> : CFGViewSuccessors<true>`
Reverse the graph, get successors but reverse-apply updates, InverseGraph = true, InverseEdge = false.
3. `std::pair<const GraphDiff<BasicBlock *> *, Inverse<BasicBlock *>>> : CFGViewPredecessors<false>`
Regular CFG, reverse edges, so children mean predecessors, InverseGraph = false, InverseEdge = true.
4. `std::pair<const GraphDiff<Inverse<BasicBlock *>> *, Inverse<BasicBlock *>> : CFGViewPredecessors<true>` 
Reverse the graph and the edges, InverseGraph = true, InverseEdge = true.

If this makes sense, I'll make this explanation into a comment before GraphDiff, rather than before CFGViewSuccessors.


================
Comment at: include/llvm/Support/CFGUpdate.h:72
+    if (InverseGraph)
+      std::swap(From, To); // Reverse edge for postdominators.
+
----------------
kuhar wrote:
> timshen wrote:
> > There is no need to swap?
> DomTree and PostDomTree allow you to pass the same update vector to both, but since PostDomTree operates on inverse CFG, you have to swap somewhere to make it work.
> 
> I believe this was just copied from the domtree code.
The swap is intentional, for reverse-applying updates. It refers to the first bool: InverseGraph and should be used for PostDomTree.
Yes, the method is copied from the domtree code. I have a dependent patch to clean that up (remove Update struct and legalizeUpdates).


Repository:
  rL LLVM

https://reviews.llvm.org/D50479