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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a subscriber: NutshellySima.
kuhar added a comment.

One thing that is not clear to me is how you decide whether to apply the updates on top of the current CFG or to reverse-apply them. DomTreeConstruction needs the latter, but I don't know what makes sense in your use-case.

I think that this should eventually replace the diffing functionality in the dominator tree construction.  This should be also perhaps useful if we decide to go with CFG snapshots for easier incremental updates, like in @NutshellySima 's prototypes.

In https://reviews.llvm.org/D50479#1193105, @asbirlea wrote:

> Looking for feedback on this before moving forward. Some questions:
>
> 1. Namespace naming (cfg) and placement of Update struct.


Fine with me.

> 2. Move WrappedPairNodeDataIterator somewhere in ADT, to reuse in LoopIterator.h?

Not familiar enough to suggest anything :/.

> 3. The part to pre-process the Updates is not part of this patch. I would like to move GenericDomTreeConstruction::LegalizeUpdates somewhere in Support outside DomTreeBuilder namespace, in order to reuse the functionality. Where is a good place to have this, or are there objections to moving it?

Fine with migrating it somewhere. I would keep it close to the update struct.

> Should these follow-up patches be part of this one?
> 
> 1. Reference cfg::Update in DomTree, remove Update from DomTreeBuilder namespace.

Fine with either now or a separate patch.

> 2. Teach IDF to use children based on a GraphDiff, given the new GraphTraits in this patch.

Where does IDF calculation need the diffing functionality?



================
Comment at: include/llvm/IR/CFGDiff.h:41
+  SmallDenseMap<NodePtr, SmallVector<NodePtr, 2>> PredDelete;
+  SmallVector<NodePtr, 1> Empty;
+
----------------
It wan't obvious for me what this is for until I grepped for the field name and saw how it's used -- I'd add a comment saying why you need it.


================
Comment at: include/llvm/IR/CFGDiff.h:71
+
+  const SmallVectorImpl<NodePtr> *getAddedChildren(const NodePtr BB,
+                                                   bool Inverse) const {
----------------
Why is returning a pointer better than reference? Can this ever return nullptr?


================
Comment at: include/llvm/IR/CFGDiff.h:79
+  }
+};
+
----------------
I think it would be *extremely* useful to add a dump method here.


Repository:
  rL LLVM

https://reviews.llvm.org/D50479