From c074f2230293a1690f2079579a5503cea39dd6e9 Mon Sep 17 00:00:00 2001 From: Laurence Tratt Date: Thu, 19 Dec 2024 11:10:29 +0000 Subject: [PATCH 1/2] Be clearer about when we will/won't peel. In particular, this change catches a case that slipped through before: if we end up with a non-testing trace that doesn't end with `TraceHeaderEnd` we have problems. This commit will make such an ill-formed trace cause a `panic`. --- ykrt/src/compile/jitc_yk/opt/mod.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/ykrt/src/compile/jitc_yk/opt/mod.rs b/ykrt/src/compile/jitc_yk/opt/mod.rs index a33526467..3e2e7d8eb 100644 --- a/ykrt/src/compile/jitc_yk/opt/mod.rs +++ b/ykrt/src/compile/jitc_yk/opt/mod.rs @@ -369,12 +369,18 @@ impl Opt { fn opt(mut self) -> Result { let base = self.m.insts_len(); - // The instruction offset after all `loadti` instructions. - let is_sidetrace = matches!(self.m.inst(self.m.last_inst_idx()), Inst::SidetraceEnd); - - // Disable loop peeling if there is no `header_end` and we are running tests. - #[cfg(test)] - let disable_peel = !matches!(self.m.inst(self.m.last_inst_idx()), Inst::TraceHeaderEnd); + let peel = match self.m.inst(self.m.last_inst_idx()) { + // If this is a sidetrace, we perform optimisations up to, but including, loop peeling. + Inst::SidetraceEnd => false, + Inst::TraceHeaderEnd => true, + #[cfg(test)] + // Not all tests create "fully valid" traces, in the sense that -- to keep things + // simple -- they don't end with `TraceHeaderEnd`. We don't want to peel such traces, + // but nor, in testing mode, do we consider them ill-formed. + _ => false, + #[cfg(not(test))] + _ => panic!(), + }; // Note that since we will apply loop peeling here, the list of instructions grows as this // loop runs. Each instruction we process is (after optimisations were applied), duplicated @@ -388,13 +394,7 @@ impl Opt { // explicitly perform dead code elimination and this function can be made `#[cfg(test)]` only. self.m.dead_code_elimination(); - #[cfg(test)] - if disable_peel { - return Ok(self.m); - } - - if is_sidetrace { - // Side-traces don't loop and thus cannot be peeled. + if !peel { return Ok(self.m); } From 3d222c0b5428df76c2a02cea01417c1fb6aa6e51 Mon Sep 17 00:00:00 2001 From: Laurence Tratt Date: Thu, 19 Dec 2024 11:18:53 +0000 Subject: [PATCH 2/2] Turn global mutation into a local variable. --- ykrt/src/compile/jitc_yk/codegen/x64/mod.rs | 22 +++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/ykrt/src/compile/jitc_yk/codegen/x64/mod.rs b/ykrt/src/compile/jitc_yk/codegen/x64/mod.rs index c78281593..7fc90d212 100644 --- a/ykrt/src/compile/jitc_yk/codegen/x64/mod.rs +++ b/ykrt/src/compile/jitc_yk/codegen/x64/mod.rs @@ -204,9 +204,6 @@ struct Assemble<'a> { /// The offset after the trace's prologue. This is the re-entry point when returning from /// side-traces. prologue_offset: AssemblyOffset, - /// Whether or not to skip processing [Param]s. We enable this once we've finished processing - /// the header, as we currently [Param]s in the trace body are only placeholders. - skip_params: bool, } impl<'a> Assemble<'a> { @@ -269,7 +266,6 @@ impl<'a> Assemble<'a> { used_insts, ptradds, prologue_offset: AssemblyOffset(0), - skip_params: false, })) } @@ -469,6 +465,7 @@ impl<'a> Assemble<'a> { fn cg_insts(&mut self) -> Result<(), CompilationError> { let mut iter = self.m.iter_skipping_insts(); let mut next = iter.next(); + let mut in_header = true; while let Some((iidx, inst)) = next { self.comment(self.asm.offset(), inst.display(iidx, self.m).to_string()); if !self.used_insts[usize::from(iidx)] { @@ -485,7 +482,13 @@ impl<'a> Assemble<'a> { } jit_ir::Inst::BinOp(i) => self.cg_binop(iidx, i), - jit_ir::Inst::Param(i) => self.cg_param(iidx, i), + jit_ir::Inst::Param(i) => { + // Right now, `Param`s in the body contain dummy values, and shouldn't be + // processed. + if in_header { + self.cg_param(iidx, i); + } + } jit_ir::Inst::Load(i) => self.cg_load(iidx, i), jit_ir::Inst::PtrAdd(pa_inst) => self.cg_ptradd(iidx, pa_inst), jit_ir::Inst::DynPtrAdd(i) => self.cg_dynptradd(iidx, i), @@ -515,7 +518,10 @@ impl<'a> Assemble<'a> { } jit_ir::Inst::Guard(i) => self.cg_guard(iidx, i), jit_ir::Inst::TraceHeaderStart => self.cg_header_start(), - jit_ir::Inst::TraceHeaderEnd => self.cg_header_end(), + jit_ir::Inst::TraceHeaderEnd => { + self.cg_header_end(); + in_header = false; + } jit_ir::Inst::TraceBodyStart => self.cg_body_start(), jit_ir::Inst::TraceBodyEnd => self.cg_body_end(iidx), jit_ir::Inst::SidetraceEnd => self.cg_sidetrace_end(iidx, self.m.root_jump_addr()), @@ -1072,9 +1078,6 @@ impl<'a> Assemble<'a> { /// Codegen a [jit_ir::ParamInst]. This only informs the register allocator about the /// locations of live variables without generating any actual machine code. fn cg_param(&mut self, iidx: jit_ir::InstIdx, inst: &jit_ir::ParamInst) { - if self.skip_params { - return; - } let m = VarLocation::from_yksmp_location(self.m, iidx, self.m.param(inst.paramidx())); debug_assert!(self.m.inst(iidx).def_byte_size(self.m) <= REG64_BYTESIZE); match m { @@ -1992,7 +1995,6 @@ impl<'a> Assemble<'a> { e => panic!("{:?}", e), } } - self.skip_params = true; } fn cg_body_start(&mut self) {