From a445b1ebeb423d39750259cb82c9f2c0677d08c7 Mon Sep 17 00:00:00 2001
From: Keno Fischer <keno@juliacomputing.com>
Date: Wed, 20 Feb 2019 21:00:26 -0500
Subject: [PATCH] Fix DFS tree construction

It's called a Depth-First tree after all, not a
dept-first-but-feel-free-to-record-any-of-the-parents tree. In
particular, in order for the semi-dominator condition to hold,
the parent in the DFS tree needs to be the predecessor with the
highest DFS number. Here we were accidentally doing the opposite
casuing us to look at the wrong node in case the sdom and the idom
are not the same. To understand #31121, consider the following
CFG (minimized from the bug report to show the issue), as well as
the corresponding DFS numbering and sdom assignment

```
     CFG     DFS     sdom

      A       1      0
      | \     | \    |\
      B C     2 5    1 1
     /|/     /|/    /|/
    | D     | 3    | 1
     \|      \|     \|
      E       4      2
```

This bug caused us to record the parent of `E` as `B`, when it should
have been `D` (the relevant invariant here is that the parent in the DFS
tree is the predecessor with the highest DFS number).
As a result, when computing idoms from the sdoms, we
were incorrectly looking at `B`, seeing that the sdom matched the
ancestor in the DFS tree and thus concluding that `E`'s idom was `B`
rather than `A`. As a result, we neglected to insert a phi node in
`E`.

Fixes #31121
---
 base/compiler/ssair/domtree.jl |  6 ++---
 test/compiler/ssair.jl         | 47 ++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/base/compiler/ssair/domtree.jl b/base/compiler/ssair/domtree.jl
index 27c9af8b44fd1..a967553b30e90 100644
--- a/base/compiler/ssair/domtree.jl
+++ b/base/compiler/ssair/domtree.jl
@@ -177,13 +177,11 @@ begin
         parent = 0
         while !isempty(worklist)
             (parent, current_node) = pop!(worklist)
+            dfs.reverse[current_node] != 0 && continue
             dfs.reverse[current_node] = dfs_num
             dfs.numbering[dfs_num] = current_node
             dfs.parents[dfs_num] = parent
             for succ in cfg.blocks[current_node].succs
-                dfs.reverse[succ] != 0 && continue
-                # Mark things that are currently in the worklist
-                dfs.reverse[succ] = 1
                 push!(worklist, (dfs_num, succ))
             end
             dfs_num += 1
@@ -239,7 +237,7 @@ begin
         # SLT would, but never simultaneously, so we could still
         # do this.
         ancestors = D.parents
-        for w ∈ reverse(_drop(preorder(D), 1))
+        for w::DFSNumber ∈ reverse(_drop(preorder(D), 1))
             # LLVM initializes this to the parent, the paper initializes this to
             # `w`, but it doesn't really matter (the parent is a predecessor,
             # so at worst we'll discover it below). Save a memory reference here.
diff --git a/test/compiler/ssair.jl b/test/compiler/ssair.jl
index f4da3979f34ab..cb25bf588b029 100644
--- a/test/compiler/ssair.jl
+++ b/test/compiler/ssair.jl
@@ -27,6 +27,53 @@ const Compiler = Core.Compiler
 #    # XXX: missing @test
 #end
 
+# Issue #31121
+using .Compiler: CFG, BasicBlock
+
+# We have the following CFG and corresponding DFS numbering:
+#
+#     CFG     DFS
+#
+#      A       1
+#      | \     | \
+#      B C     2 5
+#     /|/     /|/
+#    | D     | 3
+#     \|      \|
+#      E       4
+#
+# In the bug `E` got the wrong dominator (`B` instead of `A`), because the DFS
+# tree had the wrong parent (i.e. we recorded the parent of `4` as `2` rather
+# than `3`, so the idom search missed that `1` is `3`'s semi-dominator). Here
+# we manually construct that CFG and verify that the DFS records the correct
+# parent.
+make_bb(preds, succs) = BasicBlock(Compiler.StmtRange(0, 0), preds, succs)
+let cfg = CFG(BasicBlock[
+    make_bb([]     , [2, 3]),
+    make_bb([1]    , [4, 5]),
+    make_bb([1]    , [4]   ),
+    make_bb([2, 3] , [5]   ),
+    make_bb([2, 4] , []    ),
+], Int[])
+    dfs = Compiler.DFS(cfg, Compiler.BBNumber(1))
+    @test dfs.numbering[dfs.parents[dfs.reverse[5]]] == 4
+    let correct_idoms = Compiler.naive_idoms(cfg)
+        @test Compiler.SNCA(cfg) == correct_idoms
+        # For completeness, reverse the order of pred/succ in the CFG and verify
+        # the answer doesn't change (it does change the which node is chosen
+        # as the semi-dominator, since it changes the DFS numbering).
+        for (a, b, c, d) in Iterators.product(((true, false) for _ = 1:4)...)
+            let cfg′ = Compiler.copy(cfg)
+                a && reverse!(cfg′.blocks[1].succs)
+                b && reverse!(cfg′.blocks[2].succs)
+                c && reverse!(cfg′.blocks[4].preds)
+                d && reverse!(cfg′.blocks[5].preds)
+                @test Compiler.SNCA(cfg′) == correct_idoms
+            end
+        end
+    end
+end
+
 # test >:
 let
     f(a, b) = a >: b