cranelift: Minimize ways to manipulate instruction results (#8293)

* cranelift: Minimize ways to manipulate instruction results

In particular, remove support for detaching/attaching/appending
instruction results.

The AliasAnalysis pass used detach_results, but leaked the detached
ValueList; using clear_results instead is better.

The verifier's `test_printing_contextual_errors` needed to get the
verifier to produce an error containing a pretty-printed instruction,
and did so by appending too many results. Instead, failing to append any
results gets a similar error out of the verifier, without requiring that
we expose the easy-to-misuse append_result method. However, `iconst` is
not a suitable instruction for this version of the test because its
result type is its controlling type, so failing to create any results
caused assertion failures rather than the desired verifier error. I
switched to `f64const` which has a non-polymorphic type.

The DFG's `aliases` test cleared both results of an instruction and then
reattached one of them. Since we have access to DFG internals in these
tests, it's easier to directly manipulate the relevant ValueList than to
use these unsafe methods.

The only other use of attach/append was in `make_inst_results_reusing`
which decided which to use based on whether a particular result was
supposed to reuse an existing value. Inlining both methods there
revealed that they were nearly identical and could have most of their
code factored out.

While I was looking at uses of `DataFlowGraph::results`, I also
simplified replace_with_aliases a little bit.

* Review comments

cranelift/codegen/src/alias_analysis.rs

@@ -385,7 +385,7 @@ impl<'a> AliasAnalysis<'a> {
             while let Some(inst) = pos.next_inst() {
                 if let Some(replaced_result) = self.process_inst(pos.func, &mut state, inst) {
                     let result = pos.func.dfg.inst_results(inst)[0];
-                    pos.func.dfg.detach_results(inst);
+                    pos.func.dfg.clear_results(inst);
                     pos.func.dfg.change_to_alias(result, replaced_result);
                     pos.remove_inst_and_step_back();
                 }

cranelift/codegen/src/ir/dfg.rs

@@ -525,33 +525,32 @@ impl DataFlowGraph {
     /// After calling this instruction, `dest_inst` will have had its results
     /// cleared, so it likely needs to be removed from the graph.
     ///
-    pub fn replace_with_aliases(&mut self, dest_inst: Inst, src_inst: Inst) {
+    pub fn replace_with_aliases(&mut self, dest_inst: Inst, original_inst: Inst) {
         debug_assert_ne!(
-            dest_inst, src_inst,
+            dest_inst, original_inst,
             "Replacing {} with itself would create a loop",
             dest_inst
         );
+
+        let dest_results = self.results[dest_inst].as_slice(&self.value_lists);
+        let original_results = self.results[original_inst].as_slice(&self.value_lists);
+
         debug_assert_eq!(
-            self.results[dest_inst].len(&self.value_lists),
-            self.results[src_inst].len(&self.value_lists),
+            dest_results.len(),
+            original_results.len(),
             "Replacing {} with {} would produce a different number of results.",
             dest_inst,
-            src_inst
+            original_inst
         );
 
-        for (&dest, &src) in self.results[dest_inst]
-            .as_slice(&self.value_lists)
-            .iter()
-            .zip(self.results[src_inst].as_slice(&self.value_lists))
-        {
-            let original = src;
+        for (&dest, &original) in dest_results.iter().zip(original_results) {
             let ty = self.value_type(original);
             debug_assert_eq!(
                 self.value_type(dest),
                 ty,
                 "Aliasing {} to {} would change its type {} to {}",
                 dest,
-                src,
+                original,
                 self.value_type(dest),
                 ty
             );
@@ -926,22 +925,27 @@ impl DataFlowGraph {
     where
         I: Iterator<Item = Option<Value>>,
     {
-        self.results[inst].clear(&mut self.value_lists);
+        self.clear_results(inst);
 
         let mut reuse = reuse.fuse();
         let result_tys: SmallVec<[_; 16]> = self.inst_result_types(inst, ctrl_typevar).collect();
-        let num_results = result_tys.len();
 
-        for ty in result_tys {
-            if let Some(Some(v)) = reuse.next() {
+        for (expected, &ty) in result_tys.iter().enumerate() {
+            let num = u16::try_from(expected).expect("Result value index should fit in u16");
+            let value_data = ValueData::Inst { ty, num, inst };
+            let v = if let Some(Some(v)) = reuse.next() {
                 debug_assert_eq!(self.value_type(v), ty, "Reused {} is wrong type", ty);
-                self.attach_result(inst, v);
+                debug_assert!(!self.value_is_attached(v));
+                self.values[v] = value_data.into();
+                v
             } else {
-                self.append_result(inst, ty);
-            }
+                self.make_value(value_data)
+            };
+            let actual = self.results[inst].push(v, &mut self.value_lists);
+            debug_assert_eq!(expected, actual);
         }
 
-        num_results
+        result_tys.len()
     }
 
     /// Create a `ReplaceBuilder` that will replace `inst` with a new instruction in place.
@@ -949,14 +953,6 @@ impl DataFlowGraph {
         ReplaceBuilder::new(self, inst)
     }
 
-    /// Detach the list of result values from `inst` and return it.
-    ///
-    /// This leaves `inst` without any result values. New result values can be created by calling
-    /// `make_inst_results` or by using a `replace(inst)` builder.
-    pub fn detach_results(&mut self, inst: Inst) -> ValueList {
-        self.results[inst].take()
-    }
-
     /// Clear the list of result values from `inst`.
     ///
     /// This leaves `inst` without any result values. New result values can be created by calling
@@ -965,25 +961,6 @@ impl DataFlowGraph {
         self.results[inst].clear(&mut self.value_lists)
     }
 
-    /// Attach an existing value to the result value list for `inst`.
-    ///
-    /// The `res` value is appended to the end of the result list.
-    ///
-    /// This is a very low-level operation. Usually, instruction results with the correct types are
-    /// created automatically. The `res` value must not be attached to anything else.
-    pub fn attach_result(&mut self, inst: Inst, res: Value) {
-        debug_assert!(!self.value_is_attached(res));
-        let num = self.results[inst].push(res, &mut self.value_lists);
-        debug_assert!(num <= u16::MAX as usize, "Too many result values");
-        let ty = self.value_type(res);
-        self.values[res] = ValueData::Inst {
-            ty,
-            num: num as u16,
-            inst,
-        }
-        .into();
-    }
-
     /// Replace an instruction result with a new value of type `new_type`.
     ///
     /// The `old_value` must be an attached instruction result.
@@ -1018,18 +995,6 @@ impl DataFlowGraph {
         new_value
     }
 
-    /// Append a new instruction result value to `inst`.
-    pub fn append_result(&mut self, inst: Inst, ty: Type) -> Value {
-        let res = self.values.next_key();
-        let num = self.results[inst].push(res, &mut self.value_lists);
-        debug_assert!(num <= u16::MAX as usize, "Too many result values");
-        self.make_value(ValueData::Inst {
-            ty,
-            inst,
-            num: num as u16,
-        })
-    }
-
     /// Clone an instruction, attaching new result `Value`s and
     /// returning them.
     pub fn clone_inst(&mut self, inst: Inst) -> Inst {
@@ -1778,8 +1743,7 @@ mod tests {
         };
 
         // Remove `c` from the result list.
-        pos.func.dfg.clear_results(iadd);
-        pos.func.dfg.attach_result(iadd, s);
+        pos.func.stencil.dfg.results[iadd].remove(1, &mut pos.func.stencil.dfg.value_lists);
 
         // Replace `uadd_overflow` with a normal `iadd` and an `icmp`.
         pos.func.dfg.replace(iadd).iadd(v1, arg0);

cranelift/codegen/src/verifier/mod.rs

@@ -1834,7 +1834,7 @@ mod tests {
             args: Default::default(),
         });
 
-        func.dfg.append_result(test_inst, ctrl_typevar);
+        func.dfg.make_inst_results(test_inst, ctrl_typevar);
         func.layout.append_inst(test_inst, block0);
         func.layout.append_inst(end_inst, block0);
 
@@ -1920,13 +1920,11 @@ mod tests {
         let block0 = func.dfg.make_block();
         func.layout.append_block(block0);
 
-        // Build instruction: v0, v1 = iconst 42
-        let inst = func.dfg.make_inst(InstructionData::UnaryImm {
-            opcode: Opcode::Iconst,
-            imm: 42.into(),
+        // Build instruction "f64const 0.0" (missing one required result)
+        let inst = func.dfg.make_inst(InstructionData::UnaryIeee64 {
+            opcode: Opcode::F64const,
+            imm: 0.into(),
         });
-        func.dfg.append_result(inst, types::I32);
-        func.dfg.append_result(inst, types::I32);
         func.layout.append_inst(inst, block0);
 
         // Setup verifier.
@@ -1935,11 +1933,11 @@ mod tests {
         let verifier = Verifier::new(&func, flags.into());
 
         // Now the error message, when printed, should contain the instruction sequence causing the
-        // error (i.e. v0, v1 = iconst.i32 42) and not only its entity value (i.e. inst0)
+        // error (i.e. f64const 0.0) and not only its entity value (i.e. inst0)
         let _ = verifier.typecheck_results(inst, types::I32, &mut errors);
         assert_eq!(
             format!("{}", errors.0[0]),
-            "inst0 (v0, v1 = iconst.i32 42): has more result values than expected"
+            "inst0 (f64const 0.0): has fewer result values than expected"
         )
     }