From 76937fe1d90862fdce00f7e5585fad7a209c3fbd Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Fri, 29 May 2026 21:34:30 +0200 Subject: [PATCH 1/3] =?UTF-8?q?perf(vm):=20add=20peephole=20pass=20to=20el?= =?UTF-8?q?ide=20dead=20Load;=20Pop=20pairs=20=E2=9C=82=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Runs to a fixed point over the IR, removing consecutive `Constant|GetGlobal|GetLocal` followed by `Pop` when neither is a jump target. Skipped on the REPL's resumable path so `halt_ip` stays stable across resume-from-halt. Co-Authored-By: Claude Opus 4.7 (1M context) --- Cargo.lock | 1 + ndc_vm/Cargo.toml | 7 ++- ndc_vm/src/chunk.rs | 45 ++++++++++++- ndc_vm/src/compiler.rs | 32 +++++++++- tests/compiler/tests/compiler.rs | 105 ++++++++++++------------------- 5 files changed, 119 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ee29880e..885b43ea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1270,6 +1270,7 @@ dependencies = [ name = "ndc_vm" version = "0.3.0" dependencies = [ + "ahash", "ndc_core", "ndc_lexer", "ndc_parser", diff --git a/ndc_vm/Cargo.toml b/ndc_vm/Cargo.toml index 99807be8..0bc05aa4 100644 --- a/ndc_vm/Cargo.toml +++ b/ndc_vm/Cargo.toml @@ -7,9 +7,10 @@ version.workspace = true trace = [] [dependencies] -thiserror.workspace = true -ndc_parser.workspace = true -ndc_lexer.workspace = true +ahash.workspace = true ndc_core.workspace = true +ndc_lexer.workspace = true +ndc_parser.workspace = true num.workspace = true ordered-float.workspace = true +thiserror.workspace = true diff --git a/ndc_vm/src/chunk.rs b/ndc_vm/src/chunk.rs index 9d8ea25b..8b5f05d8 100644 --- a/ndc_vm/src/chunk.rs +++ b/ndc_vm/src/chunk.rs @@ -1,4 +1,5 @@ use crate::Value; +use ahash::AHashSet; use ndc_core::hash_map::HashMap; use ndc_lexer::Span; use ndc_parser::CaptureSource; @@ -36,7 +37,6 @@ impl JumpTarget { /// Advance `ip` by this offset using wrapping signed arithmetic. #[inline] pub fn apply(self, ip: usize) -> usize { - // TODO: is this bad for perf? match self { JumpTarget::Offset(o) => ip.wrapping_add_signed(o), JumpTarget::Label(_) => panic!("cannot apply instruction pointer to unresolved label"), @@ -267,6 +267,49 @@ impl OptimizerIr { } } + pub(crate) fn peephole(&mut self) { + let targets: AHashSet = self + .code + .iter() + .filter_map(|(_, op, _)| match op { + OpCode::Jump(JumpTarget::Label(l)) + | OpCode::JumpIfFalse(JumpTarget::Label(l)) + | OpCode::JumpIfTrue(JumpTarget::Label(l)) + | OpCode::IterNext(JumpTarget::Label(l)) => Some(*l), + _ => None, + }) + .collect(); + + loop { + let mut out = Vec::with_capacity(self.code.len()); + let mut i = 0; + let mut removed = 0; + while i < self.code.len() { + let elide = i + 1 < self.code.len() + && matches!( + self.code[i].1, + OpCode::Constant(_) | OpCode::GetGlobal(_) | OpCode::GetLocal(_) + ) + && matches!(self.code[i + 1].1, OpCode::Pop) + && !targets.contains(&self.code[i].0) + && !targets.contains(&self.code[i + 1].0); + if elide { + i += 2; + removed += 2; + } else { + out.push(self.code[i].clone()); + i += 1; + } + } + + self.code = out; + + if removed == 0 { + break; + } + } + } + pub(crate) fn add_constant(&mut self, value: Value) -> usize { self.constants.push(value); self.constants.len() - 1 diff --git a/ndc_vm/src/compiler.rs b/ndc_vm/src/compiler.rs index dc1d853d..04763b65 100644 --- a/ndc_vm/src/compiler.rs +++ b/ndc_vm/src/compiler.rs @@ -9,29 +9,51 @@ use ndc_parser::{ }; use std::rc::Rc; -#[derive(Default, Clone)] +#[derive(Clone)] pub struct Compiler { ir: OptimizerIr, num_locals: usize, loop_stack: Vec, allow_return: bool, + optimize: bool, +} + +impl Default for Compiler { + fn default() -> Self { + Self { + ir: OptimizerIr::default(), + num_locals: 0, + loop_stack: Vec::new(), + allow_return: false, + optimize: true, + } + } } impl Compiler { pub fn compile( expressions: impl Iterator, ) -> Result { - Ok(Self::compile_resumable(expressions)?.0) + let mut compiler = Self::default(); + for expr_loc in expressions { + compiler.compile_expr(expr_loc)?; + } + Ok(compiler.finish()?.0) } /// Compile expressions and return both the finished function and a /// checkpoint that can be passed to `resume` to append more code later. /// The checkpoint is the compiler state *before* the `Halt` instruction, /// so `resume` can extend the bytecode without re-running old instructions. + /// + /// Optimization is disabled on this path: the REPL's resume-from- + /// halt machinery relies on `halt_ip` matching the position of `Halt` in + /// the emitted chunk, and shifting instructions invalidates that. pub fn compile_resumable( expressions: impl Iterator, ) -> Result<(CompiledFunction, Self), CompileError> { let mut compiler = Self::default(); + compiler.optimize = false; for expr_loc in expressions { compiler.compile_expr(expr_loc)?; } @@ -71,6 +93,10 @@ impl Compiler { fn finish(mut self) -> Result<(CompiledFunction, Self), CompileError> { let checkpoint = self.clone(); self.ir.write(OpCode::Halt, Span::synthetic()); + if self.optimize { + self.ir.peephole(); + } + let function = CompiledFunction { name: None, static_type: StaticType::Function { @@ -723,6 +749,8 @@ impl Compiler { fn_compiler.compile_expr(body)?; fn_compiler.ir.write(OpCode::Return, Span::synthetic()); + fn_compiler.ir.peephole(); + let compiled = CompiledFunction { name, static_type, diff --git a/tests/compiler/tests/compiler.rs b/tests/compiler/tests/compiler.rs index 52a26514..10bdf144 100644 --- a/tests/compiler/tests/compiler.rs +++ b/tests/compiler/tests/compiler.rs @@ -140,12 +140,11 @@ fn test_or() { // 5; // -// 0: Constant(0) push `5` -// 1: Pop discard value (it's a statement) -// 2: Halt +// Pre-peephole: `Constant(0), Pop, Halt`. The peephole pass elides the dead +// `Constant; Pop` pair, leaving just `Halt`. #[test] fn test_statement() { - assert_eq!(compile("5;"), [Constant(0), Pop, Halt]); + assert_eq!(compile("5;"), [Halt]); } // { 5 } @@ -159,40 +158,35 @@ fn test_block_with_expression() { // { 5; } // -// 0: Constant(0) push `5` -// 1: Pop discard (trailing semicolon) -// 2: Constant(1) push `()` (block result is unit) -// 3: Halt +// Pre-peephole: `Constant(0), Pop, Constant(1), Halt`. The peephole pass +// elides the dead `Constant(0); Pop`, leaving the unit result. #[test] fn test_block_with_trailing_statement() { - assert_eq!(compile("{ 5; }"), [Constant(0), Pop, Constant(1), Halt]); + assert_eq!(compile("{ 5; }"), [Constant(1), Halt]); } // { 5; 6 } // -// 0: Constant(0) push `5` -// 1: Pop discard intermediate statement -// 2: Constant(1) push `6` (block result) -// 3: Halt +// Pre-peephole: `Constant(0), Pop, Constant(1), Halt`. The peephole elides +// the intermediate `Constant(0); Pop`, leaving just the block result. #[test] fn test_block_multiple_statements() { - assert_eq!(compile("{ 5; 6 }"), [Constant(0), Pop, Constant(1), Halt]); + assert_eq!(compile("{ 5; 6 }"), [Constant(1), Halt]); } // if true { 3 } else { 3; } // -// true branch returns 3, false branch returns () +// true branch returns 3, false branch returns (). The peephole pass elides +// the false branch's `Constant; Pop` (pushing `3` only to discard it). // // 0: Constant(0) push `true` // 1: JumpIfFalse(3) jump to false branch (index 5) // 2: Pop pop condition (true path) // 3: Constant(1) push `3` -// 4: Jump(4) jump to Halt (index 9) +// 4: Jump(2) jump to Halt (index 7) // 5: Pop pop condition (false path) -// 6: Constant(2) push `3` (inner of `3;`) -// 7: Pop discard (trailing semicolon) -// 8: Constant(3) push `()` (block result) -// 9: Halt +// 6: Constant(3) push `()` (block result; constant 2 still in table but unreferenced) +// 7: Halt #[test] fn test_if_with_statement_else() { assert_eq!( @@ -202,9 +196,7 @@ fn test_if_with_statement_else() { JumpIfFalse(JumpTarget::Offset(3)), Pop, Constant(1), - Jump(JumpTarget::Offset(4)), - Pop, - Constant(2), + Jump(JumpTarget::Offset(2)), Pop, Constant(3), Halt @@ -214,34 +206,27 @@ fn test_if_with_statement_else() { // if true { 3; } else { 3; } // -// Both branches return () — result is unit regardless of condition +// Both branches return () — the peephole elides the `Constant; Pop` in each +// arm (each branch's `3;` becomes nothing). // // 0: Constant(0) push `true` -// 1: JumpIfFalse(5) jump to false branch (index 7) +// 1: JumpIfFalse(3) jump to false branch (index 5) // 2: Pop pop condition (true path) -// 3: Constant(1) push `3` -// 4: Pop discard -// 5: Constant(2) push `()` -// 6: Jump(4) jump to Halt (index 11) -// 7: Pop pop condition (false path) -// 8: Constant(3) push `3` -// 9: Pop discard -// 10: Constant(4) push `()` -// 11: Halt +// 3: Constant(2) push `()` (true-branch result; constant 1 unreferenced) +// 4: Jump(2) jump to Halt (index 7) +// 5: Pop pop condition (false path) +// 6: Constant(4) push `()` (false-branch result; constant 3 unreferenced) +// 7: Halt #[test] fn test_if_with_statement_branches() { assert_eq!( compile("if true { 3; } else { 3; }"), [ Constant(0), - JumpIfFalse(JumpTarget::Offset(5)), - Pop, - Constant(1), + JumpIfFalse(JumpTarget::Offset(3)), Pop, Constant(2), - Jump(JumpTarget::Offset(4)), - Pop, - Constant(3), + Jump(JumpTarget::Offset(2)), Pop, Constant(4), Halt @@ -251,25 +236,24 @@ fn test_if_with_statement_branches() { // while true { 1 } // +// The peephole elides the body's `Constant(1); Pop` (the loop body's value +// is discarded). The backward jump offset shrinks accordingly. +// // 0: Constant(0) push `true` ← loop_start -// 1: JumpIfFalse(4) if false, jump past body to exit Pop (index 6) +// 1: JumpIfFalse(2) if false, jump past body to exit Pop (index 4) // 2: Pop pop condition (true path) -// 3: Constant(1) body: push `1` -// 4: Pop discard body value (loops produce no value) -// 5: Jump(-6) jump back to loop_start (index 0) -// 6: Pop pop condition (false path, loop exit) -// 7: Halt +// 3: Jump(-4) jump back to loop_start (index 0) +// 4: Pop pop condition (false path, loop exit) +// 5: Halt #[test] fn test_while() { assert_eq!( compile("while true { 1 }"), [ Constant(0), - JumpIfFalse(JumpTarget::Offset(4)), - Pop, - Constant(1), + JumpIfFalse(JumpTarget::Offset(2)), Pop, - Jump(JumpTarget::Offset(-6)), + Jump(JumpTarget::Offset(-4)), Pop, Halt ] @@ -294,30 +278,21 @@ fn test_declaration() { // let a = 1; // a = 5; // -// Declaration stores 1 into pre-allocated slot 0. -// Assignment pushes new value, SetLocal overwrites, -// push unit as the expression result, Pop discards it. +// Declaration stores 1 into pre-allocated slot 0. Assignment pushes new +// value, SetLocal overwrites. The peephole elides the trailing +// `Constant; Pop` (the assignment expression's unit result, discarded as a +// statement). // // 0: Constant(0) push `1` // 1: SetLocal(0) store in slot 0 (declaration) // 2: Constant(1) push `5` // 3: SetLocal(0) overwrite slot 0 (assignment) -// 4: Constant(2) push `()` (assignment result) -// 5: Pop discard (statement) -// 6: Halt +// 4: Halt #[test] fn test_assignment() { assert_eq!( compile_with_analysis("let a = 1;\na = 5;"), - [ - Constant(0), - SetLocal(0), - Constant(1), - SetLocal(0), - Constant(2), - Pop, - Halt - ] + [Constant(0), SetLocal(0), Constant(1), SetLocal(0), Halt] ); } From cdd03bb1ad949b4219267e24c886dbe347b0df80 Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Fri, 29 May 2026 22:08:20 +0200 Subject: [PATCH 2/3] =?UTF-8?q?test(compiler):=20split=20raw=20and=20optim?= =?UTF-8?q?ized=20assertions=20into=20separate=20suites=20=F0=9F=94=AC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `Compiler::compile_unoptimized` and `Interpreter::compile_str_unoptimized` so the existing compiler-tests crate can document the raw compiler output again. Reverts the seven assertions that PR #163 updated for the post-peephole form, and introduces `tests/optimizer.rs` with focused smoke tests for the peephole pass itself. Co-Authored-By: Claude Opus 4.7 (1M context) --- ndc_interpreter/src/lib.rs | 11 +++ ndc_vm/src/compiler.rs | 14 ++++ tests/compiler/tests/compiler.rs | 113 +++++++++++++++++++----------- tests/compiler/tests/optimizer.rs | 88 +++++++++++++++++++++++ 4 files changed, 184 insertions(+), 42 deletions(-) create mode 100644 tests/compiler/tests/optimizer.rs diff --git a/ndc_interpreter/src/lib.rs b/ndc_interpreter/src/lib.rs index 213e1292..cc7dfda1 100644 --- a/ndc_interpreter/src/lib.rs +++ b/ndc_interpreter/src/lib.rs @@ -125,6 +125,17 @@ impl Interpreter { Ok(Compiler::compile(expressions.into_iter())?) } + /// Like [`Self::compile_str`] but skips the peephole optimizer. + /// Used by the compiler-tests crate to assert against raw bytecode. + pub fn compile_str_unoptimized( + &mut self, + input: &str, + ) -> Result { + let source_id = self.source_db.add("", input); + let (expressions, _) = self.parse_and_analyse(input, source_id)?; + Ok(Compiler::compile_unoptimized(expressions.into_iter())?) + } + pub fn disassemble_str(&mut self, input: &str) -> Result { let compiled = self.compile_str(input)?; let mut out = String::new(); diff --git a/ndc_vm/src/compiler.rs b/ndc_vm/src/compiler.rs index 04763b65..0e5ba549 100644 --- a/ndc_vm/src/compiler.rs +++ b/ndc_vm/src/compiler.rs @@ -41,6 +41,20 @@ impl Compiler { Ok(compiler.finish()?.0) } + /// Compile expressions without running the peephole optimizer. Useful + /// for tests that want to inspect the raw compiler output, and for + /// debugging tools (e.g. a future `--no-optimize` disassembler flag). + pub fn compile_unoptimized( + expressions: impl Iterator, + ) -> Result { + let mut compiler = Self::default(); + compiler.optimize = false; + for expr_loc in expressions { + compiler.compile_expr(expr_loc)?; + } + Ok(compiler.finish()?.0) + } + /// Compile expressions and return both the finished function and a /// checkpoint that can be passed to `resume` to append more code later. /// The checkpoint is the compiler state *before* the `Halt` instruction, diff --git a/tests/compiler/tests/compiler.rs b/tests/compiler/tests/compiler.rs index 10bdf144..3426d22d 100644 --- a/tests/compiler/tests/compiler.rs +++ b/tests/compiler/tests/compiler.rs @@ -5,12 +5,16 @@ use ndc_vm::chunk::OpCode; use ndc_vm::chunk::OpCode::*; use ndc_vm::compiler::Compiler; +// These helpers compile without the peephole optimizer so the tests +// document the raw compiler output as a specification. Optimizer behaviour +// is exercised in `tests/optimizer.rs`. + fn compile(input: &str) -> Vec { let tokens = Lexer::new(input, SourceId::SYNTHETIC) .collect::, _>>() .expect("lex failed"); let expressions = Parser::from_tokens(tokens).parse().expect("parse failed"); - Compiler::compile(expressions.into_iter()) + Compiler::compile_unoptimized(expressions.into_iter()) .expect("compile failed") .opcodes() .to_vec() @@ -19,7 +23,7 @@ fn compile(input: &str) -> Vec { fn compile_with_analysis(input: &str) -> Vec { let mut interp = ndc_interpreter::Interpreter::capturing(); interp - .compile_str(input) + .compile_str_unoptimized(input) .expect("compile failed") .opcodes() .to_vec() @@ -140,11 +144,12 @@ fn test_or() { // 5; // -// Pre-peephole: `Constant(0), Pop, Halt`. The peephole pass elides the dead -// `Constant; Pop` pair, leaving just `Halt`. +// 0: Constant(0) push `5` +// 1: Pop discard value (it's a statement) +// 2: Halt #[test] fn test_statement() { - assert_eq!(compile("5;"), [Halt]); + assert_eq!(compile("5;"), [Constant(0), Pop, Halt]); } // { 5 } @@ -158,35 +163,40 @@ fn test_block_with_expression() { // { 5; } // -// Pre-peephole: `Constant(0), Pop, Constant(1), Halt`. The peephole pass -// elides the dead `Constant(0); Pop`, leaving the unit result. +// 0: Constant(0) push `5` +// 1: Pop discard (trailing semicolon) +// 2: Constant(1) push `()` (block result is unit) +// 3: Halt #[test] fn test_block_with_trailing_statement() { - assert_eq!(compile("{ 5; }"), [Constant(1), Halt]); + assert_eq!(compile("{ 5; }"), [Constant(0), Pop, Constant(1), Halt]); } // { 5; 6 } // -// Pre-peephole: `Constant(0), Pop, Constant(1), Halt`. The peephole elides -// the intermediate `Constant(0); Pop`, leaving just the block result. +// 0: Constant(0) push `5` +// 1: Pop discard intermediate statement +// 2: Constant(1) push `6` (block result) +// 3: Halt #[test] fn test_block_multiple_statements() { - assert_eq!(compile("{ 5; 6 }"), [Constant(1), Halt]); + assert_eq!(compile("{ 5; 6 }"), [Constant(0), Pop, Constant(1), Halt]); } // if true { 3 } else { 3; } // -// true branch returns 3, false branch returns (). The peephole pass elides -// the false branch's `Constant; Pop` (pushing `3` only to discard it). +// true branch returns 3, false branch returns () // // 0: Constant(0) push `true` // 1: JumpIfFalse(3) jump to false branch (index 5) // 2: Pop pop condition (true path) // 3: Constant(1) push `3` -// 4: Jump(2) jump to Halt (index 7) +// 4: Jump(4) jump to Halt (index 9) // 5: Pop pop condition (false path) -// 6: Constant(3) push `()` (block result; constant 2 still in table but unreferenced) -// 7: Halt +// 6: Constant(2) push `3` (inner of `3;`) +// 7: Pop discard (trailing semicolon) +// 8: Constant(3) push `()` (block result) +// 9: Halt #[test] fn test_if_with_statement_else() { assert_eq!( @@ -196,7 +206,9 @@ fn test_if_with_statement_else() { JumpIfFalse(JumpTarget::Offset(3)), Pop, Constant(1), - Jump(JumpTarget::Offset(2)), + Jump(JumpTarget::Offset(4)), + Pop, + Constant(2), Pop, Constant(3), Halt @@ -206,27 +218,34 @@ fn test_if_with_statement_else() { // if true { 3; } else { 3; } // -// Both branches return () — the peephole elides the `Constant; Pop` in each -// arm (each branch's `3;` becomes nothing). +// Both branches return () — result is unit regardless of condition // // 0: Constant(0) push `true` -// 1: JumpIfFalse(3) jump to false branch (index 5) +// 1: JumpIfFalse(5) jump to false branch (index 7) // 2: Pop pop condition (true path) -// 3: Constant(2) push `()` (true-branch result; constant 1 unreferenced) -// 4: Jump(2) jump to Halt (index 7) -// 5: Pop pop condition (false path) -// 6: Constant(4) push `()` (false-branch result; constant 3 unreferenced) -// 7: Halt +// 3: Constant(1) push `3` +// 4: Pop discard +// 5: Constant(2) push `()` +// 6: Jump(4) jump to Halt (index 11) +// 7: Pop pop condition (false path) +// 8: Constant(3) push `3` +// 9: Pop discard +// 10: Constant(4) push `()` +// 11: Halt #[test] fn test_if_with_statement_branches() { assert_eq!( compile("if true { 3; } else { 3; }"), [ Constant(0), - JumpIfFalse(JumpTarget::Offset(3)), + JumpIfFalse(JumpTarget::Offset(5)), + Pop, + Constant(1), Pop, Constant(2), - Jump(JumpTarget::Offset(2)), + Jump(JumpTarget::Offset(4)), + Pop, + Constant(3), Pop, Constant(4), Halt @@ -236,24 +255,25 @@ fn test_if_with_statement_branches() { // while true { 1 } // -// The peephole elides the body's `Constant(1); Pop` (the loop body's value -// is discarded). The backward jump offset shrinks accordingly. -// // 0: Constant(0) push `true` ← loop_start -// 1: JumpIfFalse(2) if false, jump past body to exit Pop (index 4) +// 1: JumpIfFalse(4) if false, jump past body to exit Pop (index 6) // 2: Pop pop condition (true path) -// 3: Jump(-4) jump back to loop_start (index 0) -// 4: Pop pop condition (false path, loop exit) -// 5: Halt +// 3: Constant(1) body: push `1` +// 4: Pop discard body value (loops produce no value) +// 5: Jump(-6) jump back to loop_start (index 0) +// 6: Pop pop condition (false path, loop exit) +// 7: Halt #[test] fn test_while() { assert_eq!( compile("while true { 1 }"), [ Constant(0), - JumpIfFalse(JumpTarget::Offset(2)), + JumpIfFalse(JumpTarget::Offset(4)), Pop, - Jump(JumpTarget::Offset(-4)), + Constant(1), + Pop, + Jump(JumpTarget::Offset(-6)), Pop, Halt ] @@ -278,21 +298,30 @@ fn test_declaration() { // let a = 1; // a = 5; // -// Declaration stores 1 into pre-allocated slot 0. Assignment pushes new -// value, SetLocal overwrites. The peephole elides the trailing -// `Constant; Pop` (the assignment expression's unit result, discarded as a -// statement). +// Declaration stores 1 into pre-allocated slot 0. +// Assignment pushes new value, SetLocal overwrites, +// push unit as the expression result, Pop discards it. // // 0: Constant(0) push `1` // 1: SetLocal(0) store in slot 0 (declaration) // 2: Constant(1) push `5` // 3: SetLocal(0) overwrite slot 0 (assignment) -// 4: Halt +// 4: Constant(2) push `()` (assignment result) +// 5: Pop discard (statement) +// 6: Halt #[test] fn test_assignment() { assert_eq!( compile_with_analysis("let a = 1;\na = 5;"), - [Constant(0), SetLocal(0), Constant(1), SetLocal(0), Halt] + [ + Constant(0), + SetLocal(0), + Constant(1), + SetLocal(0), + Constant(2), + Pop, + Halt + ] ); } diff --git a/tests/compiler/tests/optimizer.rs b/tests/compiler/tests/optimizer.rs new file mode 100644 index 00000000..091cb600 --- /dev/null +++ b/tests/compiler/tests/optimizer.rs @@ -0,0 +1,88 @@ +use ndc_lexer::{Lexer, SourceId}; +use ndc_parser::Parser; +use ndc_vm::chunk::OpCode; +use ndc_vm::chunk::OpCode::*; +use ndc_vm::compiler::Compiler; + +fn parse(input: &str) -> Vec { + let tokens = Lexer::new(input, SourceId::SYNTHETIC) + .collect::, _>>() + .expect("lex failed"); + Parser::from_tokens(tokens).parse().expect("parse failed") +} + +fn unoptimized(input: &str) -> Vec { + Compiler::compile_unoptimized(parse(input).into_iter()) + .expect("compile failed") + .opcodes() + .to_vec() +} + +fn optimized(input: &str) -> Vec { + Compiler::compile(parse(input).into_iter()) + .expect("compile failed") + .opcodes() + .to_vec() +} + +// `5;` — a discarded constant. The peephole eliminates the `Constant; Pop` +// pair entirely, leaving just `Halt`. +#[test] +fn elides_constant_pop_statement() { + assert_eq!(unoptimized("5;"), [Constant(0), Pop, Halt]); + assert_eq!(optimized("5;"), [Halt]); +} + +// A `Pop` that's the target of a conditional jump must not be elided even +// when the preceding instruction is a `Load` — the jumper relies on it to +// drop the condition value off the stack. Here, `JumpIfFalse` targets the +// false-branch `Pop`, and that `Pop` survives optimization. +#[test] +fn preserves_pop_as_jump_target() { + let raw = unoptimized("if true { 3 } else { 3; }"); + let opt = optimized("if true { 3 } else { 3; }"); + + // Optimization removes the `3; → Constant; Pop` in the else branch. + assert!( + opt.len() < raw.len(), + "optimizer should shrink the chunk; raw={raw:?} opt={opt:?}" + ); + + // Both branch-condition Pops survive — one per arm. Raw has a third Pop + // (the discarded statement in the else branch); optimized does not. + let pop_count = |ops: &[OpCode]| ops.iter().filter(|op| matches!(op, Pop)).count(); + assert_eq!(pop_count(&raw), 3, "raw should have 3 Pops"); + assert_eq!(pop_count(&opt), 2, "optimized should have 2 Pops"); + + // The optimized chunk still has the conditional jump and the + // unconditional skip — control flow is intact. + assert!(opt.iter().any(|op| matches!(op, JumpIfFalse(_)))); + assert!(opt.iter().any(|op| matches!(op, Jump(_)))); +} + +// `{ 1; 2; 3; }` is three discarded statements followed by the block's +// unit result. Every `Load; Pop` pair is elided; only the trailing block +// result remains. Exercises the peephole loop catching multiple pairs. +#[test] +fn elides_multiple_load_pop_pairs() { + let raw = unoptimized("{ 1; 2; 3; }"); + let opt = optimized("{ 1; 2; 3; }"); + + // Raw: three `Constant; Pop` pairs plus the final unit `Constant` plus Halt. + assert_eq!( + raw, + [ + Constant(0), + Pop, + Constant(1), + Pop, + Constant(2), + Pop, + Constant(3), + Halt, + ] + ); + + // Optimized: just the final unit `Constant` plus Halt. + assert_eq!(opt, [Constant(3), Halt]); +} From 02b1a3a5d92c6d97b459893af5c9f5009688ebd6 Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Fri, 29 May 2026 22:17:39 +0200 Subject: [PATCH 3/3] =?UTF-8?q?fix(vm):=20propagate=20optimize=20flag=20to?= =?UTF-8?q?=20nested=20function=20compilers=20=F0=9F=AA=86?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nested `fn_compiler` was constructed with `Self::default()` and called `peephole()` unconditionally, so function bodies were optimized even when the caller used `compile_unoptimized` or `compile_str_unoptimized` for the top-level. Inherit `self.optimize` and gate the call. Adds `CompiledFunction::body()` as a public accessor so the new test can walk the top-level constants table to inspect a nested function's raw vs optimized bytecode. Spotted by Codex on PR #163. Co-Authored-By: Claude Opus 4.7 (1M context) --- ndc_vm/src/compiler.rs | 17 +++++++++---- ndc_vm/src/value/function.rs | 4 +++ tests/compiler/tests/optimizer.rs | 42 +++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/ndc_vm/src/compiler.rs b/ndc_vm/src/compiler.rs index 0e5ba549..dd85a17f 100644 --- a/ndc_vm/src/compiler.rs +++ b/ndc_vm/src/compiler.rs @@ -47,8 +47,10 @@ impl Compiler { pub fn compile_unoptimized( expressions: impl Iterator, ) -> Result { - let mut compiler = Self::default(); - compiler.optimize = false; + let mut compiler = Self { + optimize: false, + ..Default::default() + }; for expr_loc in expressions { compiler.compile_expr(expr_loc)?; } @@ -66,8 +68,10 @@ impl Compiler { pub fn compile_resumable( expressions: impl Iterator, ) -> Result<(CompiledFunction, Self), CompileError> { - let mut compiler = Self::default(); - compiler.optimize = false; + let mut compiler = Self { + optimize: false, + ..Default::default() + }; for expr_loc in expressions { compiler.compile_expr(expr_loc)?; } @@ -758,12 +762,15 @@ impl Compiler { let mut fn_compiler = Self { num_locals: num_params, allow_return: true, + optimize: self.optimize, ..Default::default() }; fn_compiler.compile_expr(body)?; fn_compiler.ir.write(OpCode::Return, Span::synthetic()); - fn_compiler.ir.peephole(); + if fn_compiler.optimize { + fn_compiler.ir.peephole(); + } let compiled = CompiledFunction { name, diff --git a/ndc_vm/src/value/function.rs b/ndc_vm/src/value/function.rs index 5d4a7ebe..fa036a9e 100644 --- a/ndc_vm/src/value/function.rs +++ b/ndc_vm/src/value/function.rs @@ -64,6 +64,10 @@ impl CompiledFunction { pub fn opcodes(&self) -> &[OpCode] { self.body.opcodes() } + + pub fn body(&self) -> &Chunk { + &self.body + } } impl Function { pub fn prototype(&self) -> Option<&Rc> { diff --git a/tests/compiler/tests/optimizer.rs b/tests/compiler/tests/optimizer.rs index 091cb600..0470a90a 100644 --- a/tests/compiler/tests/optimizer.rs +++ b/tests/compiler/tests/optimizer.rs @@ -1,5 +1,6 @@ use ndc_lexer::{Lexer, SourceId}; use ndc_parser::Parser; +use ndc_stdlib as _; use ndc_vm::chunk::OpCode; use ndc_vm::chunk::OpCode::*; use ndc_vm::compiler::Compiler; @@ -60,6 +61,47 @@ fn preserves_pop_as_jump_target() { assert!(opt.iter().any(|op| matches!(op, Jump(_)))); } +// When the caller asks for unoptimized output, nested function bodies must +// also be raw — otherwise debug/inspection callers would silently see +// optimized bytecode inside the constants table. +fn first_nested_function_opcodes(compiled: &ndc_vm::value::CompiledFunction) -> Vec { + for (_, _, val) in compiled.body().iter() { + let Some(ndc_vm::Value::Object(obj)) = val else { + continue; + }; + if let ndc_vm::Object::Function(ndc_vm::value::Function::Compiled(f)) = obj.as_ref() { + return f.opcodes().to_vec(); + } + } + panic!("expected a compiled function constant in the top-level chunk"); +} + +#[test] +fn nested_function_bodies_respect_unoptimized_flag() { + let mut interp = ndc_interpreter::Interpreter::capturing(); + let raw = interp + .compile_str_unoptimized("fn f() { 1; }") + .expect("compile failed"); + + let mut interp = ndc_interpreter::Interpreter::capturing(); + let opt = interp.compile_str("fn f() { 1; }").expect("compile failed"); + + // Raw body for `1;` followed by an implicit unit Return: + // `Constant; Pop; Constant; Return`. + assert_eq!( + first_nested_function_opcodes(&raw), + [Constant(0), Pop, Constant(1), Return], + "nested function body should be raw under compile_str_unoptimized", + ); + + // Optimized body: peephole drops the leading `Constant; Pop`. + assert_eq!( + first_nested_function_opcodes(&opt), + [Constant(1), Return], + "nested function body should be optimized under compile_str", + ); +} + // `{ 1; 2; 3; }` is three discarded statements followed by the block's // unit result. Every `Load; Pop` pair is elided; only the trailing block // result remains. Exercises the peephole loop catching multiple pairs.