[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 added inline comments.


================
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.");
----------------
chandlerc wrote:
> 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...
SGTM to the first.

I can't immediately think of something better than `Node` either, SGTM to the second too.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:135
+    // the DFS numbering.
+    for (DomTreeNode *InnerN : DomNodes)
+      DomSet.insert(InnerN->getBlock());
----------------
chandlerc wrote:
> 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.
I don't see why non-determinism in the worklist order matters here.


https://reviews.llvm.org/D32740