From 1aa844db221b7efb88f588677fcad99150d65cbd Mon Sep 17 00:00:00 2001 From: Michael Schimmel Date: Sat, 21 Feb 2026 22:51:53 +0100 Subject: [PATCH] feat: Add dead code elimination and improve optimization Integrates dead code elimination (DCE) into the optimizer. This phase removes expressions that have no side effects and are not the result of the block. Also includes several improvements to the existing optimization passes: - DCE now correctly handles assignments to variables that are never used or captured. - Explicitly tracks captured slots in the `SubstitutionMap` to prevent premature inlining. - Introduces a `is_side_effect_free` helper function for more robust purity checks. - Updates dependencies to include `insta` for snapshot testing. --- Cargo.lock | 38 +++++++++++ Cargo.toml | 3 + src/ast/compiler/optimizer.rs | 120 ++++++++++++++++++++++++++++++---- 3 files changed, 150 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 74bf3c0..f1e0fac 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -749,6 +749,18 @@ dependencies = [ "crossbeam-utils", ] +[[package]] +name = "console" +version = "0.15.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "054ccb5b10f9f2cbf51eb355ca1d05c2d279ce1804688d0db74b4733a5aeafd8" +dependencies = [ + "encode_unicode", + "libc", + "once_cell", + "windows-sys 0.59.0", +] + [[package]] name = "core-foundation" version = "0.9.4" @@ -1025,6 +1037,12 @@ dependencies = [ "bytemuck", ] +[[package]] +name = "encode_unicode" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "34aa73646ffb006b8f5147f3dc182bd4bcb190227ce861fc4a4844bf8e3cb2c0" + [[package]] name = "endi" version = "1.1.1" @@ -1660,6 +1678,19 @@ dependencies = [ "serde_core", ] +[[package]] +name = "insta" +version = "1.46.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e82db8c87c7f1ccecb34ce0c24399b8a73081427f3c7c50a5d597925356115e4" +dependencies = [ + "console", + "once_cell", + "serde", + "similar", + "tempfile", +] + [[package]] name = "is_terminal_polyfill" version = "1.70.2" @@ -1884,6 +1915,7 @@ dependencies = [ "chrono", "clap", "eframe", + "insta", "regex", ] @@ -2760,6 +2792,12 @@ version = "0.3.8" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e320a6c5ad31d271ad523dcf3ad13e2767ad8b1cb8f047f75a8aeaf8da139da2" +[[package]] +name = "similar" +version = "2.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbbb5d9659141646ae647b42fe094daf6c6192d1620870b449d9557f748b2daa" + [[package]] name = "slab" version = "0.4.12" diff --git a/Cargo.toml b/Cargo.toml index 7ef18b7..b2208c1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,3 +9,6 @@ eframe = "0.33.3" clap = { version = "4.5", features = ["derive"] } chrono = "0.4" regex = "1.10" + +[dev-dependencies] +insta = { version = "1.39", features = ["yaml"] } diff --git a/src/ast/compiler/optimizer.rs b/src/ast/compiler/optimizer.rs index 3b87b5f..68c8123 100644 --- a/src/ast/compiler/optimizer.rs +++ b/src/ast/compiler/optimizer.rs @@ -3,9 +3,9 @@ use crate::ast::compiler::bound_nodes::{BoundKind, TypedNode, Address}; use crate::ast::types::{Value, StaticType}; use crate::ast::vm::Closure; use crate::ast::nodes::Node; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; -/// The Optimizer performs Phase 2 (Cracking) and Phase 2.5 (Aggressive Collapsing). +/// The Optimizer performs Phase 2 (Cracking) and Phase 2.5/2.6 (Aggressive Collapsing & DCE). pub struct Optimizer { pub level: u32, max_passes: usize, @@ -45,6 +45,7 @@ impl Optimizer { kind: BoundKind::Constant(val.clone()), }; } + sub.used_slots.insert(slot); } Address::Upvalue(idx) => { if let Some(val) = sub.upvalues.get(&idx) { @@ -148,6 +149,33 @@ impl Optimizer { } if self.level >= 2 { + // Phase 2.6: Dead Code Elimination (DCE) + // Remove side-effect-free expressions from the block that are not the result. + if !new_exprs.is_empty() { + let last_idx = new_exprs.len() - 1; + let mut filtered = Vec::with_capacity(new_exprs.len()); + for (i, e) in new_exprs.into_iter().enumerate() { + if i == last_idx { + filtered.push(e); + continue; + } + + // A statement is removable if it's pure (no side effects) + // or if it's a local assignment to a variable that is never used or captured. + let removable = match &e.kind { + BoundKind::DefLocal { slot, .. } | BoundKind::Set { addr: Address::Local(slot), .. } => { + !sub.used_slots.contains(slot) && !sub.captured_slots.contains(slot) && self.is_side_effect_free(&e) + } + _ => self.is_side_effect_free(&e), + }; + + if !removable { + filtered.push(e); + } + } + new_exprs = filtered; + } + if new_exprs.is_empty() { return Node { identity: node.identity, kind: BoundKind::Nop, ty: StaticType::Void }; } else if new_exprs.len() == 1 { @@ -169,6 +197,8 @@ impl Optimizer { match capture_addr { Address::Local(slot) => { if let Some(val) = sub.locals.get(slot) { inlined_val = Some(val.clone()); } + // Mark slot as captured in parent scope + sub.captured_slots.insert(*slot); } Address::Upvalue(idx) => { if let Some(val) = sub.upvalues.get(idx) { inlined_val = Some(val.clone()); } @@ -203,6 +233,9 @@ impl Optimizer { }, BoundKind::DefLocal { name, slot, value, captured_by } => { + if !captured_by.is_empty() { + sub.captured_slots.insert(slot); + } let value = Box::new(self.visit_node(*value, sub)); if let BoundKind::Constant(val) = &value.kind { sub.add_local(slot, val.clone()); @@ -244,6 +277,21 @@ impl Optimizer { Node { identity: node.identity, kind: new_kind, ty: new_ty } } + fn is_side_effect_free(&self, node: &TypedNode) -> bool { + match &node.kind { + BoundKind::Constant(_) | BoundKind::Get { .. } | BoundKind::Parameter { .. } | BoundKind::Nop | BoundKind::Lambda { .. } => true, + BoundKind::Tuple { elements } => elements.iter().all(|e| self.is_side_effect_free(e)), + BoundKind::Record { fields } => fields.iter().all(|(k, v)| self.is_side_effect_free(k) && self.is_side_effect_free(v)), + BoundKind::If { cond, then_br, else_br } => { + self.is_side_effect_free(cond) && self.is_side_effect_free(then_br) && else_br.as_ref().is_none_or(|e| self.is_side_effect_free(e)) + } + BoundKind::Block { exprs } => exprs.iter().all(|e| self.is_side_effect_free(e)), + BoundKind::Expansion { bound_expanded, .. } => self.is_side_effect_free(bound_expanded), + BoundKind::DefLocal { value, .. } | BoundKind::Set { value, .. } => self.is_side_effect_free(value), + _ => false, // Call and DefGlobal are considered impure + } + } + fn try_beta_reduce(&self, params: &TypedNode, args: &TypedNode, body: TypedNode) -> Option { if self.contains_def_local(&body) { return None; @@ -354,11 +402,20 @@ struct SubstitutionMap { locals: HashMap, /// Mapping Address::Upvalue(idx) -> Value upvalues: HashMap, + /// Tracking which local slots are actually used (not inlined) + used_slots: HashSet, + /// Tracking which local slots are captured by lambdas + captured_slots: HashSet, } impl SubstitutionMap { fn new() -> Self { - Self { locals: HashMap::new(), upvalues: HashMap::new() } + Self { + locals: HashMap::new(), + upvalues: HashMap::new(), + used_slots: HashSet::new(), + captured_slots: HashSet::new(), + } } fn add_local(&mut self, slot: u32, val: Value) { @@ -376,7 +433,7 @@ impl SubstitutionMap { if let Some(res) = mapping.get(idx as usize) { match res { Some(new_idx) => (BoundKind::Get { addr: Address::Upvalue(*new_idx), name }, node.ty), - None => (BoundKind::Get { addr: Address::Upvalue(idx), name }, node.ty), // Should have been inlined + None => (BoundKind::Get { addr: Address::Upvalue(idx), name }, node.ty), } } else { (BoundKind::Get { addr: Address::Upvalue(idx), name }, node.ty) @@ -384,8 +441,6 @@ impl SubstitutionMap { }, BoundKind::Lambda { params, upvalues, body, positional_count } => { - // IMPORTANT: If this nested lambda captures an upvalue from our current scope, - // we MUST re-index it in its own capture list! let mut next_upvalues = Vec::new(); for addr in upvalues { if let Address::Upvalue(idx) = addr @@ -393,18 +448,14 @@ impl SubstitutionMap { if let Some(new_idx) = res { next_upvalues.push(Address::Upvalue(*new_idx)); } - continue; // Inlined or re-indexed + continue; } next_upvalues.push(addr); } - - // Note: We don't recurse into the body with the SAME mapping, - // because nested Get(Upvalue) nodes refer to THIS lambda's capture list. (BoundKind::Lambda { params, upvalues: next_upvalues, body, positional_count }, node.ty) }, BoundKind::If { cond, then_br, else_br } => { - let cond = Box::new(self.reindex_upvalues(*cond, mapping)); let then_br = Box::new(self.reindex_upvalues(*then_br, mapping)); let else_br = else_br.map(|e| Box::new(self.reindex_upvalues(*e, mapping))); @@ -441,3 +492,50 @@ impl SubstitutionMap { Node { identity: node.identity, kind: new_kind, ty: new_ty } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::ast::environment::Environment; + + fn get_optimized_dump(source: &str, level: u32) -> String { + let mut env = Environment::new(); + env.optimization_level = level; + env.dump_ast(source).expect("Compilation failed during test") + } + + #[test] + fn test_opt_folding() { + let dump = get_optimized_dump("(+ 10 20)", 2); + insta::assert_snapshot!(dump); + } + + #[test] + fn test_opt_local_inlining() { + // Wrap in a do-block within a lambda to force DefLocal + let source = "(fn [] (do (def x 10) (+ x 5)))"; + let dump = get_optimized_dump(source, 2); + insta::assert_snapshot!(dump); + } + + #[test] + fn test_opt_dce_unused() { + let source = "(fn [] (do (def x 10) 42))"; + let dump = get_optimized_dump(source, 2); + insta::assert_snapshot!(dump); + } + + #[test] + fn test_opt_assignment_safety() { + let source = "(fn [] (do (def x 10) (assign x 20) x))"; + let dump = get_optimized_dump(source, 2); + insta::assert_snapshot!(dump); + } + + #[test] + fn test_opt_lambda_cracking() { + let source = "(fn [] (do (def x 10) (fn [] x)))"; + let dump = get_optimized_dump(source, 2); + insta::assert_snapshot!(dump); + } +}