diff --git a/ykrt/src/compile/jitc_yk/opt/analyse.rs b/ykrt/src/compile/jitc_yk/opt/analyse.rs index 05c453afb..f2d577eb5 100644 --- a/ykrt/src/compile/jitc_yk/opt/analyse.rs +++ b/ykrt/src/compile/jitc_yk/opt/analyse.rs @@ -4,6 +4,7 @@ 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? /// @@ -11,8 +12,10 @@ use super::{ /// `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, + /// 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>, } 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)); diff --git a/ykrt/src/compile/jitc_yk/opt/mod.rs b/ykrt/src/compile/jitc_yk/opt/mod.rs index f3f99b7c8..998222edd 100644 --- a/ykrt/src/compile/jitc_yk/opt/mod.rs +++ b/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) {