After optimising an instruction, rewrite all of its operands.

Previously we only picked up results from the analyser if we performed
other optimisations. So imagine we had this:

```
%3: i8 = ...
%4: i1 = eq %3, 7
...
call f(%3)
```

then previously the call wouldn't become `f(7i8)` but would stay as
`f(%3)`. This commit rewrites *every* instruction so that we pick up all
the results of the analyser. We could do this more efficiently (e.g.
it's not worth rewriting instructions we've already optimised), but this
is good enough for now, and it leads to a meaningful difference e.g.
about a 4% improvement on big_loop.

ykrt/src/compile/jitc_yk/opt/analyse.rs

@@ -4,15 +4,18 @@ use super::{
     super::jit_ir::{GuardInst, Inst, InstIdx, Module, Operand, Predicate},
     Value,
 };
+use std::cell::RefCell;
 
 /// Ongoing analysis of a trace: what value can a given instruction in the past produce?
 ///
 /// Note that the analysis is forward-looking: just because an instruction's `Value` is (say) a
 /// `Const` now does not mean it would be valid to assume that at earlier points it is safe to
 /// assume it was also a `Const`.
 pub(super) struct Analyse {
-    /// For each instruction, what have we learnt about its [Value] so far?
-    values: Vec<Value>,
+    /// For each instruction, what have we learnt about its [Value] so far? This is a `RefCell` as
+    /// it allows [Analyse::op_map] to take `&self`: changing that to `&mut self` upsets a lot of
+    /// other parts of the system w.r.t. the borrow checker.
+    values: RefCell<Vec<Value>>,
 }
 
 impl Analyse {
@@ -22,15 +25,15 @@ impl Analyse {
             // point. What we do know is that it is at most two times the size (though since we
             // don't copy over [Tombstone]s and [Copy]s it will be slightly less than that.
             // FIXME: Can we calculate this more accurately?
-            values: vec![Value::Unknown; m.insts_len() * 2],
+            values: RefCell::new(vec![Value::Unknown; m.insts_len() * 2]),
         }
     }
 
     /// Map `op` based on our analysis so far. In some cases this will return `op` unchanged, but
     /// in others it may be able to turn what looks like a variable reference into a constant.
-    pub(super) fn op_map(&mut self, m: &Module, op: Operand) -> Operand {
+    pub(super) fn op_map(&self, m: &Module, op: Operand) -> Operand {
         match op {
-            Operand::Var(iidx) => match self.values[usize::from(iidx)] {
+            Operand::Var(iidx) => match self.values.borrow()[usize::from(iidx)] {
                 Value::Unknown => {
                     // Since we last saw an `ICmp` instruction, we may have gathered new knowledge
                     // that allows us to turn it into a constant.
@@ -42,7 +45,8 @@ impl Analyse {
                         {
                             if pred == Predicate::Equal && m.const_(lhs_cidx) == m.const_(rhs_cidx)
                             {
-                                self.values[usize::from(iidx)] = Value::Const(m.true_constidx());
+                                self.values.borrow_mut()[usize::from(iidx)] =
+                                    Value::Const(m.true_constidx());
                                 return Operand::Const(m.true_constidx());
                             }
                         }
@@ -56,12 +60,12 @@ impl Analyse {
     }
 
     /// Update our idea of what value the instruction at `iidx` can produce.
-    pub(super) fn set_value(&mut self, iidx: InstIdx, v: Value) {
-        self.values[usize::from(iidx)] = v;
+    pub(super) fn set_value(&self, iidx: InstIdx, v: Value) {
+        self.values.borrow_mut()[usize::from(iidx)] = v;
     }
 
     /// Use the guard `inst` to update our knowledge about the variable used as its condition.
-    pub(super) fn guard(&mut self, m: &Module, g_inst: GuardInst) {
+    pub(super) fn guard(&self, m: &Module, g_inst: GuardInst) {
         if let Operand::Var(iidx) = g_inst.cond(m) {
             if let Inst::ICmp(ic_inst) = m.inst(iidx) {
                 let lhs = self.op_map(m, ic_inst.lhs(m));

ykrt/src/compile/jitc_yk/opt/mod.rs

@@ -55,7 +55,6 @@ impl Opt {
         for (iidx, inst) in skipping.into_iter() {
             match inst {
                 Inst::TraceHeaderStart => (),
-                Inst::TraceHeaderEnd => (),
                 _ => {
                     self.opt_inst(iidx)?;
                     self.cse(&mut instll, iidx);
@@ -130,7 +129,7 @@ impl Opt {
         for (iidx, inst) in skipping.into_iter() {
             match inst {
                 Inst::TraceHeaderStart | Inst::TraceHeaderEnd => panic!(),
-                Inst::TraceBodyStart | Inst::TraceBodyEnd => (),
+                Inst::TraceBodyStart => (),
                 _ => {
                     self.opt_inst(iidx)?;
                     self.cse(&mut instll, iidx);
@@ -145,14 +144,17 @@ impl Opt {
 
     /// Optimise instruction `iidx`.
     fn opt_inst(&mut self, iidx: InstIdx) -> Result<(), CompilationError> {
+        // First rewrite the instruction so that all changes from the analyser are reflected
+        // straight away. Note: we deliberately do this before some of the changes below. Most
+        // notably we need to call `rewrite` before telling the analyser about a `Guard`: if we
+        // swap that order, the guard will pick up the wrong value for operand(s) related to
+        // whether the guard succeeds!
+        self.rewrite(iidx)?;
+
         match self.m.inst(iidx) {
             #[cfg(test)]
             Inst::BlackBox(_) => (),
-            Inst::Const(_)
-            | Inst::Copy(_)
-            | Inst::Tombstone
-            | Inst::TraceHeaderStart
-            | Inst::TraceHeaderEnd => {
+            Inst::Const(_) | Inst::Copy(_) | Inst::Tombstone | Inst::TraceHeaderStart => {
                 unreachable!()
             }
             Inst::BinOp(x) => match x.binop() {
@@ -478,9 +480,30 @@ impl Opt {
             }
             _ => (),
         };
+
         Ok(())
     }
 
+    /// Rewrite the instruction at `iidx`: duplicate it and remap its operands so that it reflects
+    /// everything learnt by the analyser.
+    fn rewrite(&mut self, iidx: InstIdx) -> Result<(), CompilationError> {
+        match self.m.inst_nocopy(iidx) {
+            None => Ok(()),
+            Some(Inst::Guard(_)) => {
+                // We can't safely rewrite guard operands as we pick up the result of the analysis
+                // on the guard itself!
+                Ok(())
+            }
+            Some(inst) => {
+                let r = inst.dup_and_remap_vars(&mut self.m, |m, op_iidx| {
+                    self.an.op_map(m, Operand::Var(op_iidx))
+                })?;
+                self.m.replace(iidx, r);
+                Ok(())
+            }
+        }
+    }
+
     /// Attempt common subexpression elimination on `iidx`, replacing it with a `Copy` or
     /// `Tombstone` if possible.
     fn cse(&mut self, instll: &mut InstLinkedList, iidx: InstIdx) {