[PATCH] D84763: DomTree: Make PostDomTree immune to block successors swap

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar requested changes to this revision.
kuhar added a comment.
This revision now requires changes to proceed.

Thanks for pushing on this.

While this makes root finding insensitive to successors' order, I think it still may be possible to run into tree instability by changing the order of tree children (also plain domtree), and in turn changing DFS numbers used by other analyses. In the case of a full tree construction, we could populate an ordering like this before tree construction to solve the tree children order issue, but I don't think this will work in case of incremental updates. AFAIK tree updates would still be based on some Cfg/Graph traits, which may give you unstable order.

Given that you want to fix InstCombine not to swap successors and move this to SimplifyCFG, does this patch improve anything in the current pass pipeline?

Overall, looks like a nice improvement to me, if there is no noticeable impact on compilation times. Have you benchmarked this patch?



================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:239
+      if (SuccOrder && Successors.size() > 1)
+        std::sort(Successors.begin(), Successors.end(),
+                  [=](NodePtr A, NodePtr B) {
----------------
nit: use `llvm::sort`


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:241
+                  [=](NodePtr A, NodePtr B) {
+                    return (*SuccOrder)[A] < (*SuccOrder)[B];
+                  });
----------------
The if checks that the size is at least 2, but this assumes something stronger: that both A and B are in the map. Since the map is mutable, this would insert missing elements making debugging hard IMO.


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:443
+      // successors transformation.
+      std::unique_ptr<DenseMap<NodePtr, unsigned>> SuccOrder;
+
----------------
nit: consider using `llvm::Optional` to stack-allocate the map.


================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:474-476
+            SuccOrder.reset(new DenseMap<NodePtr, unsigned>());
+            for (const auto NodePtr : nodes(DT.Parent))
+              SuccOrder->try_emplace(NodePtr, SuccOrder->size());
----------------
If I understand correctly, this will go over the whole function and populate the `SuccOrder` map with `O(|F|)` pairs.
Can we instead only populate it with nodes in the same SCC? This will still include some unnecessary nodes (reverse-reachable loop entries), but should be much smaller.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84763/new/

https://reviews.llvm.org/D84763