[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:135
+    // the DFS numbering.
+    for (DomTreeNode *InnerN : DomNodes)
+      DomSet.insert(InnerN->getBlock());
----------------
chandlerc wrote:
> sanjoy wrote:
> > 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.
> That changes the order of children in the domtree, which in turn can change the output of anything that walks the domtree.
> 
> Typically, the domtree's order is fully determined by the order of the IR in the function. I'm not worried about having any particular order, but I'm worried about non-deterministic order as I suspect other passes transform code in ways influenced by the domtree visit order.
> 
> Does this seem unreasonable?
I missed that this would change the order of children.  Given that, what's here LGTM.


https://reviews.llvm.org/D32740