[PATCH] D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 2 inline comments as done.
chandlerc added a comment.

In https://reviews.llvm.org/D32740#748894, @davide wrote:

> The algorithm you have, I think, it's correct to start (although as pointed out by Sanjoy doesn't really compute the dominance frontier).
>  Have you considered computing the iterated dominance frontier every time instead? If there are technical difficulties (or it's just slower) I'd add a comment to the code explaining why we can't use it.


Added some comments about why the existing IDF calculator seems like a bad fit for what we want here.



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:117
+  SmallPtrSet<BasicBlock *, 4> DomSet;
+  auto AppendDomFrontier = [&](DomTreeNode *Node) {
+    assert(DomNodes.empty() && "Must start with no dominator nodes.");
----------------
sanjoy wrote:
> Minor stylistic thing: this lambda is large enough that I'd prefer pulling it out into a static helper.
> 
> I'd also call the parameter something more descriptive than `Node`.
I somewhat prefer it as a lambda because that lets the underlying (and persisting) data structures be very elegantly captured rather than having to clutter up the interface. But if you feel strongly, I'll hoist it out.

That said, a better parameter name is always welcome. Any suggestions though? I couldn't come up with anything...


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:135
+    // the DFS numbering.
+    for (DomTreeNode *InnerN : DomNodes)
+      DomSet.insert(InnerN->getBlock());
----------------
sanjoy wrote:
> I'd be tempted to sort and then binary search instead of building an intermediate set.
I have a lame reason to not do that: it makes the order of the worklist non-determinist because i have to sort by pointer order. At the point where I have to keep two lists, it seems like a set is just simpler code-wise.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:142
+      for (BasicBlock *SuccBB : successors(InnerN->getBlock()))
+        if (!DomSet.count(SuccBB))
+          Worklist.insert(SuccBB);
----------------
sanjoy wrote:
> You're not really computing the dominance frontier here -- the condition for that would be `!DomSet.count(SuccBB) || SuccBB == Node`.
> 
> I think the algorithm is correct overall, but this difference from a textbook dominance frontier needs to be documented.
> 
Added a comment. In fact, we could "append" itself without changing behavior because the worklist is deduplicated and this node came from the worklist. But it seems good to document that we're intentionally not including this aspect of the dominance frontier.


https://reviews.llvm.org/D32740