Auto merge of rust-lang#111596 - cjgillot:dominator-bucket, r=Mark-Si…

…mulacrum

Process current bucket instead of parent's bucket when starting loop for dominators.

The linked paper by Georgiadis suggests in §2.2.3 to process `bucket[w]` when beginning the loop, instead of `bucket[parent[w]]` when finishing it.

In the test case, we correctly computed `idom[2] = 0` and `sdom[3] = 1`, but the algorithm returned `idom[3] = 1`, instead of the correct value 0, because of the path 0-7-2-3.

This provoked LLVM ICE in rust-lang#111061 (comment). LLVM checks that SSA assignments dominate uses using its own implementation of Lengauer-Tarjan, and saw case where rustc was breaking the dominance property.

r? `@Mark-Simulacrum`

compiler/rustc_data_structures/src/graph/dominators/mod.rs

@@ -109,28 +109,27 @@ pub fn dominators<G: ControlFlowGraph>(graph: G) -> Dominators<G::Node> {
         // they have been placed in the bucket.
         //
         // We compute a partial set of immediate dominators here.
-        let z = parent[w];
-        for &v in bucket[z].iter() {
+        for &v in bucket[w].iter() {
             // This uses the result of Lemma 5 from section 2 from the original
             // 1979 paper, to compute either the immediate or relative dominator
             // for a given vertex v.
             //
             // eval returns a vertex y, for which semi[y] is minimum among
-            // vertices semi[v] +> y *> v. Note that semi[v] = z as we're in the
-            // z bucket.
+            // vertices semi[v] +> y *> v. Note that semi[v] = w as we're in the
+            // w bucket.
             //
             // Given such a vertex y, semi[y] <= semi[v] and idom[y] = idom[v].
             // If semi[y] = semi[v], though, idom[v] = semi[v].
             //
             // Using this, we can either set idom[v] to be:
-            //  * semi[v] (i.e. z), if semi[y] is z
+            //  * semi[v] (i.e. w), if semi[y] is w
             //  * idom[y], otherwise
             //
             // We don't directly set to idom[y] though as it's not necessarily
             // known yet. The second preorder traversal will cleanup by updating
             // the idom for any that were missed in this pass.
             let y = eval(&mut parent, lastlinked, &semi, &mut label, v);
-            idom[v] = if semi[y] < z { y } else { z };
+            idom[v] = if semi[y] < w { y } else { w };
         }
 
         // This loop computes the semi[w] for w.
@@ -213,10 +212,11 @@ pub fn dominators<G: ControlFlowGraph>(graph: G) -> Dominators<G::Node> {
         // If we don't yet know the idom directly, then push this vertex into
         // our semidominator's bucket, where it will get processed at a later
         // stage to compute its immediate dominator.
-        if parent[w] != semi[w] {
+        let z = parent[w];
+        if z != semi[w] {
             bucket[semi[w]].push(w);
         } else {
-            idom[w] = parent[w];
+            idom[w] = z;
         }
 
         // Optimization: We share the parent array between processed and not

compiler/rustc_data_structures/src/graph/dominators/tests.rs

@@ -53,3 +53,30 @@ fn immediate_dominator() {
     assert_eq!(dominators.immediate_dominator(2), Some(1));
     assert_eq!(dominators.immediate_dominator(3), Some(2));
 }
+
+#[test]
+fn transitive_dominator() {
+    let graph = TestGraph::new(
+        0,
+        &[
+            // First tree branch.
+            (0, 1),
+            (1, 2),
+            (2, 3),
+            (3, 4),
+            // Second tree branch.
+            (1, 5),
+            (5, 6),
+            // Third tree branch.
+            (0, 7),
+            // These links make 0 the dominator for 2 and 3.
+            (7, 2),
+            (5, 3),
+        ],
+    );
+
+    let dom_tree = dominators(&graph);
+    let immediate_dominators = &dom_tree.immediate_dominators;
+    assert_eq!(immediate_dominators[2], Some(0));
+    assert_eq!(immediate_dominators[3], Some(0)); // This used to return Some(1).
+}