From 0e023e97ea088ae2f6700b110139b2721fcf9467 Mon Sep 17 00:00:00 2001
From: Dan Gohman <sunfish@mozilla.com>
Date: Thu, 25 May 2017 16:37:31 -0700
Subject: [PATCH] Fix more GVN issues (#83)

* Fix GVN skipping the instruction after a deleted instruction.

* Teach GVN to resolve aliases as it proceeds.

* Clean up an obsolete reference to extended_values.
---
 cranelift/filetests/simple_gvn/basic.cton | 14 ++++++--
 lib/cretonne/src/ir/dfg.rs                | 42 +++++++++++++++++------
 lib/cretonne/src/ir/layout.rs             | 12 +++++++
 lib/cretonne/src/simple_gvn.rs            |  5 ++-
 4 files changed, 59 insertions(+), 14 deletions(-)

diff --git a/cranelift/filetests/simple_gvn/basic.cton b/cranelift/filetests/simple_gvn/basic.cton
index 20544666fc..06d0989d1e 100644
--- a/cranelift/filetests/simple_gvn/basic.cton
+++ b/cranelift/filetests/simple_gvn/basic.cton
@@ -4,8 +4,18 @@ function simple_redundancy(i32, i32) -> i32 {
 ebb0(v0: i32, v1: i32):
     v2 = iadd v0, v1
     v3 = iadd v0, v1
-; check: v3 -> v2
     v4 = imul v2, v3
-; check: v4 = imul $v2, $v3
+; check: v4 = imul $v2, $v2
     return v4
 }
+
+function cascading_redundancy(i32, i32) -> i32 {
+ebb0(v0: i32, v1: i32):
+    v2 = iadd v0, v1
+    v3 = iadd v0, v1
+    v4 = imul v2, v3
+    v5 = imul v2, v2
+    v6 = iadd v4, v5
+; check: v6 = iadd $v4, $v4
+    return v6
+}
diff --git a/lib/cretonne/src/ir/dfg.rs b/lib/cretonne/src/ir/dfg.rs
index 81c3142680..35d7a18fa4 100644
--- a/lib/cretonne/src/ir/dfg.rs
+++ b/lib/cretonne/src/ir/dfg.rs
@@ -106,6 +106,23 @@ impl DataFlowGraph {
     }
 }
 
+/// Resolve value aliases.
+///
+/// Find the original SSA value that `value` aliases.
+fn resolve_aliases(values: &EntityMap<Value, ValueData>, value: Value) -> Value {
+    let mut v = value;
+
+    // Note that values may be empty here.
+    for _ in 0..1 + values.len() {
+        if let ValueData::Alias { original, .. } = values[v] {
+            v = original;
+        } else {
+            return v;
+        }
+    }
+    panic!("Value alias loop detected for {}", value);
+}
+
 /// Handling values.
 ///
 /// Values are either EBB arguments or instruction results.
@@ -176,17 +193,7 @@ impl DataFlowGraph {
     ///
     /// Find the original SSA value that `value` aliases.
     pub fn resolve_aliases(&self, value: Value) -> Value {
-        let mut v = value;
-
-        // Note that extended_values may be empty here.
-        for _ in 0..1 + self.values.len() {
-            if let ValueData::Alias { original, .. } = self.values[v] {
-                v = original;
-            } else {
-                return v;
-            }
-        }
-        panic!("Value alias loop detected for {}", value);
+        resolve_aliases(&self.values, value)
     }
 
     /// Resolve value copies.
@@ -216,6 +223,19 @@ impl DataFlowGraph {
         panic!("Copy loop detected for {}", value);
     }
 
+    /// Resolve all aliases among inst's arguments.
+    ///
+    /// For each argument of inst which is defined by an alias, replace the
+    /// alias with the aliased value.
+    pub fn resolve_aliases_in_arguments(&mut self, inst: Inst) {
+        for arg in self.insts[inst].arguments_mut(&mut self.value_lists) {
+            let resolved = resolve_aliases(&self.values, *arg);
+            if resolved != *arg {
+                *arg = resolved;
+            }
+        }
+    }
+
     /// Turn a value into an alias of another.
     ///
     /// Change the `dest` value to behave as an alias of `src`. This means that all uses of `dest`
diff --git a/lib/cretonne/src/ir/layout.rs b/lib/cretonne/src/ir/layout.rs
index 4e6f738653..06ab92612b 100644
--- a/lib/cretonne/src/ir/layout.rs
+++ b/lib/cretonne/src/ir/layout.rs
@@ -923,6 +923,18 @@ impl<'f> Cursor<'f> {
         inst
     }
 
+    /// Remove the instruction under the cursor.
+    ///
+    /// The cursor is left pointing at the position preceding the current instruction.
+    ///
+    /// Return the instruction that was removed.
+    pub fn remove_inst_and_step_back(&mut self) -> Inst {
+        let inst = self.current_inst().expect("No instruction to remove");
+        self.prev_inst();
+        self.layout.remove_inst(inst);
+        inst
+    }
+
     /// Insert an EBB at the current position and switch to it.
     ///
     /// As far as possible, this method behaves as if the EBB header were an instruction inserted
diff --git a/lib/cretonne/src/simple_gvn.rs b/lib/cretonne/src/simple_gvn.rs
index cfd13128fe..a7665e965d 100644
--- a/lib/cretonne/src/simple_gvn.rs
+++ b/lib/cretonne/src/simple_gvn.rs
@@ -28,6 +28,9 @@ pub fn do_simple_gvn(func: &mut Function, cfg: &mut ControlFlowGraph) {
         while let Some(inst) = pos.next_inst() {
             let opcode = func.dfg[inst].opcode();
 
+            // Resolve aliases, particularly aliases we created earlier.
+            func.dfg.resolve_aliases_in_arguments(inst);
+
             if trivially_unsafe_for_gvn(opcode) {
                 continue;
             }
@@ -47,7 +50,7 @@ pub fn do_simple_gvn(func: &mut Function, cfg: &mut ControlFlowGraph) {
                 Occupied(mut entry) => {
                     if domtree.dominates(*entry.get(), inst, &pos.layout) {
                         func.dfg.replace_with_aliases(inst, *entry.get());
-                        pos.remove_inst();
+                        pos.remove_inst_and_step_back();
                     } else {
                         // The prior instruction doesn't dominate inst, so it
                         // won't dominate any subsequent instructions we'll