[PATCH] D37575: [BasicBlock] add new function removeEdge()

[Brian Rzycki via Phabricator via llvm-commits]

brzycki marked 5 inline comments as done.
brzycki added a comment.

In https://reviews.llvm.org/D37575#864676, @kuhar wrote:

> This actually seems a bit orthogonal to what we wanted to do with @dberlin. The idea was to see if it is possible to always preserve dominators, and to do it completely automatically. With this high-level goal in mind, I tried to hack removePredecessor (and others) to grab DominatorTree (if available) and update it internally. That would probably also deprecate the batch updater, as every update DominatorTree update would happen in lockstep with CFG update calls. The problem was however that the existing BasicBlock API doesn't really care about CFG edges and it wasn't really possible to place the DT update calls anywhere.


@sebpop and I tried the exact same thing early on in the https://reviews.llvm.org/D37528 JumpThreading work. We ran into the exact same problem: too many places deal with the minutiae of the IR and we found several CFG cases where dominance couldn't be easily updated.  This removeEdge() patch actually came out of that work as yet another attempt to fix the problem. Unfortunately the call sites for removePredecessor() almost always use for (auto Successor: Successors(BB))... and needed more fixup to work with removeEdge().

> It looks like this patch could be the one of the first steps towards completely automatic DominatorTree updates. Another interesting thing would be to see if it would be possible to do something equivalent for addEdge -- with these 2 pieces of API in place, it would be much easier to fuzz and benchmarks the incremental DominatorTree updater.

I'd be happy to help move in this direction: it would greatly simplify middle-end work for everyone if we could think about BB chains in the context of CFG nodes and edges instead of constantly programming terminator instruction edge-case logic.



================
Comment at: llvm/lib/IR/BasicBlock.cpp:302
+        Updates.push_back({DominatorTree::Delete, this, NormalDestBB});
+      if (this != UnwindDestBB && NormalDestBB != UnwindDestBB)
+        Updates.push_back({DominatorTree::Delete, this, UnwindDestBB});
----------------
Fixed in rev 2, needed to check NormalDestBB != UnwindDestBB in the corner-case where Normal and Unwind are the same basic block.


https://reviews.llvm.org/D37575