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

[Yevgeny Rouban via Phabricator via llvm-commits]

yrouban added a comment.

In D84763#2179638 <https://reviews.llvm.org/D84763#2179638>, @kuhar wrote:

> Taking a few steps back, I'd love to see a cfg preservation check like D81558 <https://reviews.llvm.org/D81558> ...

I'm about to update the patches.

In D84763#2179475 <https://reviews.llvm.org/D84763#2179475>, @kuhar wrote:

> 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),...

I feel I do not understand. Any example?

> and in turn changing DFS numbers used by other analyses. ...

I agree that DFS numbers are changed by successors swap, but can we make an exclusion for DFS numbers? If not, then this patch is useless because successors swap change CFG by definition (DFS numbers will be changes if we recalculate them).

> AFAIK tree updates would still be based on some Cfg/Graph traits, which may give you unstable order.

As I see successors swap cannot be encoded for DomTreeUpdater. So this kind of change are not tracked.



================
Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:241
+                  [=](NodePtr A, NodePtr B) {
+                    return (*SuccOrder)[A] < (*SuccOrder)[B];
+                  });
----------------
kuhar wrote:
> 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.
there should not be missing elements. All blocks of the parent are mappend. I will mark SuccOrder const.


================
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());
----------------
kuhar wrote:
> 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.
> 
I agree that adding only needed elements to the map would take less memory. Though it could complicate implementation and take more CPU cycles. Every time we need to add a bunch of new mapped nodes we need to iterate through the whole DT.Parent just to get its number in the DT.Parent iteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84763