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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
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.");
----------------
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`.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:127
+    for (int i = 0; i < (int)DomNodes.size(); ++i)
+      for (DomTreeNode *ChildN : *DomNodes[i])
+        DomNodes.push_back(ChildN);
----------------
This could be `DomNodes.insert(DomNodes.end(), DomNodes[i]->begin(), DomNodes[i]->end());`


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:135
+    // the DFS numbering.
+    for (DomTreeNode *InnerN : DomNodes)
+      DomSet.insert(InnerN->getBlock());
----------------
I'd be tempted to sort and then binary search instead of building an intermediate set.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:142
+      for (BasicBlock *SuccBB : successors(InnerN->getBlock()))
+        if (!DomSet.count(SuccBB))
+          Worklist.insert(SuccBB);
----------------
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.



https://reviews.llvm.org/D32740