Merge pull request #1535 from ltratt/dead_load_store

Dead load store analysis / optimisation

ykrt/src/compile/jitc_yk/codegen/x64/rev_analyse.rs

@@ -138,8 +138,9 @@ impl<'a> RevAnalyse<'a> {
 
     fn analyse(&mut self, iidx: InstIdx, inst: Inst) {
         if self.used_insts.get(usize::from(iidx)).unwrap()
+            || inst.is_internal_inst()
+            || inst.is_guard()
             || inst.has_store_effect(self.m)
-            || inst.is_barrier(self.m)
         {
             self.used_insts.set(usize::from(iidx), true);
 

ykrt/src/compile/jitc_yk/jit_ir/dead_code.rs

@@ -13,8 +13,9 @@ impl Module {
         let mut tombstone = Vob::from_elem(false, usize::from(self.last_inst_idx()) + 1);
         for (iidx, inst) in self.iter_skipping_insts().rev() {
             if used.get(usize::from(iidx)).unwrap()
+                || inst.is_internal_inst()
+                || inst.is_guard()
                 || inst.has_store_effect(self)
-                || inst.is_barrier(self)
             {
                 used.set(usize::from(iidx), true);
                 inst.map_operand_vars(self, &mut |x| {

ykrt/src/compile/jitc_yk/jit_ir/jit_ir.l

@@ -7,7 +7,7 @@ double "DOUBLE_TYPE"
 -?[0-9]+i[0-9]+ "CONST_INT"
 -?[0-9]+(\.[0-9]+)?float "CONST_FLOAT"
 -?[0-9]+(\.[0-9]+)?double "CONST_DOUBLE"
-[0-9]+ "UINT"
+-?[0-9]+ "INT"
 0[xX][0-9a-fA-F]+ "CONST_PTR"
 add "ADD"
 and "AND"

ykrt/src/compile/jitc_yk/jit_ir/jit_ir.y

@@ -80,7 +80,7 @@ Inst -> Result<ASTInst, Box<dyn Error>>:
   | "GUARD" "FALSE" "," Operand "," "[" OperandsList "]" {
       Ok(ASTInst::Guard{cond: $4?, is_true: false, operands: $7?})
     }
-  | "LOCAL_OPERAND" ":" Type "=" "PARAM" "UINT" {
+  | "LOCAL_OPERAND" ":" Type "=" "PARAM" "INT" {
       Ok(ASTInst::Param{assign: $1?.span(), type_: $3?, tiidx: $6?.span()})
     }
   | "LOCAL_OPERAND" ":" Type "=" BinOp Operand "," Operand  {
@@ -110,10 +110,10 @@ Inst -> Result<ASTInst, Box<dyn Error>>:
   | "LOCAL_OPERAND" ":" Type "=" "LOAD" Operand "," "VOLATILE" {
       Ok(ASTInst::Load{assign: $1?.span(), type_: $3?, val: $6?, volatile: true})
     }
-  | "LOCAL_OPERAND" ":" Type "=" "PTR_ADD" Operand "," "UINT" {
+  | "LOCAL_OPERAND" ":" Type "=" "PTR_ADD" Operand "," "INT" {
       Ok(ASTInst::PtrAdd{assign: $1?.span(), type_: $3?, ptr: $6?, off: $8?.span()})
     }
-  | "LOCAL_OPERAND" ":" Type "=" "DYN_PTR_ADD" Operand "," Operand "," "UINT" {
+  | "LOCAL_OPERAND" ":" Type "=" "DYN_PTR_ADD" Operand "," Operand "," "INT" {
       Ok(ASTInst::DynPtrAdd{assign: $1?.span(), type_: $3?, ptr: $6?, num_elems: $8?, elem_size: $10?.span()})
     }
   | "LOCAL_OPERAND" ":" Type "=" "SEXT" Operand {

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

@@ -400,11 +400,11 @@ impl Module {
     pub(crate) fn iter_skipping_insts(
         &self,
     ) -> impl DoubleEndedIterator<Item = (InstIdx, Inst)> + '_ {
-        // The `unchecked_from` is safe because we know from `Self::push` that we can't have
-        // exceeded `InstIdx`'s bounds.
         (0..self.insts.len()).filter_map(|i| {
             let inst = &self.insts[i];
             if !matches!(inst, Inst::Const(_) | Inst::Copy(_) | Inst::Tombstone) {
+                // The `unchecked_from` is safe because we know from `Self::push` that we can't have
+                // exceeded `InstIdx`'s bounds.
                 Some((InstIdx::unchecked_from(i), *inst))
             } else {
                 None
@@ -1530,30 +1530,34 @@ impl Inst {
     /// Does this instruction have a store side effect?
     pub(crate) fn has_store_effect(&self, m: &Module) -> bool {
         match self {
+            #[cfg(test)]
+            Inst::BlackBox(_) => true,
             Inst::Copy(x) => m.inst_raw(*x).has_store_effect(m),
             Inst::Store(_) => true,
-            Inst::Call(_) | Inst::IndirectCall(_) => false,
+            Inst::Call(_) | Inst::IndirectCall(_) => true,
             _ => false,
         }
     }
 
-    /// Is this instruction a compiler barrier / fence? Our simple semantics are that barriers
-    /// cannot be reordered at all. Note that load/store effects are not considered
-    /// barriers.
-    pub(crate) fn is_barrier(&self, m: &Module) -> bool {
-        match self {
-            #[cfg(test)]
-            Inst::BlackBox(_) => true,
-            Inst::Copy(x) => m.inst_raw(*x).is_barrier(m),
-            Inst::Guard(_) => true,
-            Inst::Call(_) | Inst::IndirectCall(_) => true,
+    /// Is this instruction an "internal" trace instruction (e.g. `TraceHeaderStart` /
+    /// `TraceHeaderEnd`)? Such instructions must not be removed, and should be thought of as
+    /// similar to a barrier.
+    pub(crate) fn is_internal_inst(&self) -> bool {
+        matches!(
+            self,
             Inst::TraceHeaderStart
-            | Inst::TraceHeaderEnd
-            | Inst::TraceBodyStart
-            | Inst::TraceBodyEnd
-            | Inst::SidetraceEnd => true,
-            _ => false,
-        }
+                | Inst::TraceHeaderEnd
+                | Inst::TraceBodyStart
+                | Inst::TraceBodyEnd
+                | Inst::SidetraceEnd
+        )
+    }
+
+    /// Is this instruction a guard?
+    ///
+    /// This is a convenience function to allow an easy mirror of `is_internal_inst`.
+    pub(crate) fn is_guard(&self) -> bool {
+        matches!(self, Inst::Guard(_))
     }
 
     /// Apply the function `f` to each of this instruction's [Operand]s iff they are of type
@@ -2270,6 +2274,12 @@ impl LoadInst {
     pub(crate) fn tyidx(&self) -> TyIdx {
         self.tyidx
     }
+
+    /// Is this a volatile load?
+    pub(crate) fn is_volatile(&self) -> bool {
+        // FIXME: We don't yet record this in AOT?!
+        false
+    }
 }
 
 /// The `Param` instruction.
@@ -2532,6 +2542,10 @@ impl StoreInst {
     pub(crate) fn tgt(&self, m: &Module) -> Operand {
         self.tgt.unpack(m)
     }
+
+    pub(crate) fn is_volatile(&self) -> bool {
+        self.volatile
+    }
 }
 
 /// The operands for a [Inst::Select]

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

@@ -1,11 +1,26 @@
 //! Analyse a trace and gradually refine what values we know a previous instruction can produce.
 
 use super::{
-    super::jit_ir::{GuardInst, Inst, InstIdx, Module, Operand, Predicate},
-    Value,
+    super::jit_ir::{ConstIdx, GuardInst, Inst, InstIdx, Module, Operand, Predicate},
+    heapvalues::{Address, HeapValues},
 };
 use std::cell::RefCell;
 
+#[derive(Clone, Debug)]
+pub(super) enum Value {
+    Unknown,
+    Const(ConstIdx),
+}
+
+impl Value {
+    fn to_operand(&self) -> Operand {
+        match self {
+            Value::Unknown => todo!(),
+            Value::Const(cidx) => Operand::Const(*cidx),
+        }
+    }
+}
+
 /// 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
@@ -16,6 +31,7 @@ pub(super) struct Analyse {
     /// 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>>,
+    heapvalues: RefCell<HeapValues>,
 }
 
 impl Analyse {
@@ -26,6 +42,7 @@ impl Analyse {
             // don't copy over [Tombstone]s and [Copy]s it will be slightly less than that.
             // FIXME: Can we calculate this more accurately?
             values: RefCell::new(vec![Value::Unknown; m.insts_len() * 2]),
+            heapvalues: RefCell::new(HeapValues::new()),
         }
     }
 
@@ -60,8 +77,32 @@ impl Analyse {
     }
 
     /// Update our idea of what value the instruction at `iidx` can produce.
-    pub(super) fn set_value(&self, iidx: InstIdx, v: Value) {
-        self.values.borrow_mut()[usize::from(iidx)] = v;
+    pub(super) fn set_value(&self, m: &Module, iidx: InstIdx, v: Value) {
+        self.values.borrow_mut()[usize::from(iidx)] = v.clone();
+        if let Some(Inst::Load(linst)) = m.inst_nocopy(iidx) {
+            let addr = Address::from_operand(m, linst.operand(m));
+            self.heapvalues.borrow_mut().load(m, addr, v.to_operand());
+        }
+    }
+
+    /// What, if any, is the currently known value of `bytesize` bytes stored at `addr`?
+    pub(super) fn heapvalue(&self, m: &Module, addr: Address, bytesize: usize) -> Option<Operand> {
+        self.heapvalues.borrow_mut().get(m, addr, bytesize)
+    }
+
+    /// Associate the value derived from a load in `op` with the address `addr`.
+    pub(super) fn push_heap_load(&self, m: &Module, addr: Address, op: Operand) {
+        self.heapvalues.borrow_mut().load(m, addr, op);
+    }
+
+    /// Associate the value derived from a store in `op` with the address `addr`.
+    pub(super) fn push_heap_store(&self, m: &Module, addr: Address, op: Operand) {
+        self.heapvalues.borrow_mut().store(m, addr, op);
+    }
+
+    /// Clear all known heap values.
+    pub(super) fn heap_barrier(&self) {
+        self.heapvalues.borrow_mut().barrier();
     }
 
     /// Use the guard `inst` to update our knowledge about the variable used as its condition.
@@ -81,7 +122,7 @@ impl Analyse {
                         if (g_inst.expect && pred == Predicate::Equal)
                             || (!g_inst.expect && pred == Predicate::NotEqual)
                         {
-                            self.set_value(iidx, Value::Const(cidx));
+                            self.set_value(m, iidx, Value::Const(cidx));
                         }
                     }
                     (&Operand::Var(_), &Operand::Var(_)) => (),