diff --git a/examples/bitwise_ops.nes b/examples/bitwise_ops.nes index 94e82bb..61032e6 100644 Binary files a/examples/bitwise_ops.nes and b/examples/bitwise_ops.nes differ diff --git a/examples/logic_ops.nes b/examples/logic_ops.nes index 511a9ef..1f2ebe9 100644 Binary files a/examples/logic_ops.nes and b/examples/logic_ops.nes differ diff --git a/examples/match_demo.nes b/examples/match_demo.nes index 772ecae..65f247f 100644 Binary files a/examples/match_demo.nes and b/examples/match_demo.nes differ diff --git a/examples/mmc3_per_state_split.nes b/examples/mmc3_per_state_split.nes index f80e039..857af30 100644 Binary files a/examples/mmc3_per_state_split.nes and b/examples/mmc3_per_state_split.nes differ diff --git a/examples/platformer.nes b/examples/platformer.nes index 0ed5185..819ffcc 100644 Binary files a/examples/platformer.nes and b/examples/platformer.nes differ diff --git a/examples/two_player.nes b/examples/two_player.nes index 2611272..4e5c36e 100644 Binary files a/examples/two_player.nes and b/examples/two_player.nes differ diff --git a/src/optimizer/mod.rs b/src/optimizer/mod.rs index 9a1ad18..81219a6 100644 --- a/src/optimizer/mod.rs +++ b/src/optimizer/mod.rs @@ -218,13 +218,22 @@ fn strength_reduce_block(block: &mut IrBasicBlock) { /// destination temps are no longer referenced anywhere in the block. fn const_fold(program: &mut IrProgram) { for func in &mut program.functions { + // Pre-compute function-wide source-operand usage: a LoadImm's + // destination may be read by an op in a sibling block or + // consumed by a branch/jump terminator in another block, so + // the per-block DCE below can't decide liveness by looking + // at its own block alone. Cf. the `and` / `or` short-circuit + // lowering: the false path writes `LoadImm(result, 0)` but + // `result` is read by the merge block's branch, not in the + // false block itself. + let func_used = collect_used_temps(func); for block in &mut func.blocks { - const_fold_block(block); + const_fold_block(block, &func_used); } } } -fn const_fold_block(block: &mut IrBasicBlock) { +fn const_fold_block(block: &mut IrBasicBlock, func_used: &HashSet) { let mut constants: HashMap = HashMap::new(); // First pass: fold arithmetic / comparison ops into LoadImm where possible. @@ -349,12 +358,21 @@ fn const_fold_block(block: &mut IrBasicBlock) { } } - // Second pass: remove LoadImm ops whose dest temps are no longer referenced - // as source operands by anything else in the block (ops + terminator). - let used = collect_used_temps_in_block(block); + // Second pass: remove LoadImm ops whose dest temps are no longer + // referenced locally AND aren't referenced function-wide. The + // function-wide check is what makes this pass correct in the + // presence of control-flow merges — a LoadImm written in one + // block and consumed in another (for example the `and`/`or` + // short-circuit false path, whose `LoadImm(result, 0)` is only + // read by the downstream merge block's branch terminator) must + // not be dropped. The previous implementation only consulted + // block-local usage and silently dropped these cross-block + // LoadImms, leaving the zero-page result slot to carry whatever + // value the *previous* AND/OR had written into it. + let used_local = collect_used_temps_in_block(block); block.ops.retain(|op| { if let IrOp::LoadImm(t, _) = op { - used.contains(t) + used_local.contains(t) || func_used.contains(t) } else { true } @@ -397,7 +415,10 @@ fn collect_used_temps(func: &IrFunction) -> HashSet { used } -/// Collect temps used as source operands within a single block (ops + terminator). +/// Collect temps used as source operands within a single block +/// (ops + terminator). Used by the per-block `LoadImm` DCE so we +/// can cheaply find local uses before falling back on the +/// function-wide liveness set. fn collect_used_temps_in_block(block: &IrBasicBlock) -> HashSet { let mut used = HashSet::new(); collect_used_from_block(block, &mut used); diff --git a/src/optimizer/tests.rs b/src/optimizer/tests.rs index f868466..4492a68 100644 --- a/src/optimizer/tests.rs +++ b/src/optimizer/tests.rs @@ -436,3 +436,85 @@ fn inline_removes_trivial() { .any(|op| matches!(op, IrOp::Call(..))); assert!(!has_call, "call to trivial function should be removed"); } + +// --------------------------------------------------------------------------- +// Cross-block LoadImm liveness (regression for stale-and-result bug) +// --------------------------------------------------------------------------- + +/// A `LoadImm(result, 0)` in a predecessor block whose only consumer +/// is a branch terminator in a *successor* block must not be dropped +/// by the const-folding pass. This is the shape the logical-`and` +/// short-circuit false path lowers to, and dropping the `LoadImm` +/// leaves the zero-page slot holding whichever value the previous +/// `and` evaluation wrote — which was observable in the platformer +/// example as spurious enemy-stomp bounces firing whenever coin 2 +/// happened to be in its pickup window. +#[test] +fn const_fold_preserves_loadimm_used_by_sibling_branch() { + let cond = IrTemp(0); + let result = IrTemp(1); + let zero = IrTemp(2); + + // Shape mirrors `lower_logical_and`: an entry block that branches + // on `cond`, a right-side block that writes `result` via an + // `Or(result, , zero)`, a false-side block that only + // writes `LoadImm(result, 0)`, and a merge block whose terminator + // reads `result`. + let entry = IrBasicBlock { + label: "entry".to_string(), + ops: vec![IrOp::LoadImm(cond, 1)], + terminator: IrTerminator::Branch(cond, "right".to_string(), "false_path".to_string()), + }; + let right = IrBasicBlock { + label: "right".to_string(), + ops: vec![IrOp::LoadImm(zero, 0), IrOp::Or(result, cond, zero)], + terminator: IrTerminator::Jump("end".to_string()), + }; + // This is the block that previously tripped the bug: its + // `LoadImm(result, 0)` is the only op, and `result` is not used + // as a source operand anywhere *within* this block — but it is + // read by the `end` block's branch terminator. + let false_path = IrBasicBlock { + label: "false_path".to_string(), + ops: vec![IrOp::LoadImm(result, 0)], + terminator: IrTerminator::Jump("end".to_string()), + }; + let end = IrBasicBlock { + label: "end".to_string(), + ops: vec![], + terminator: IrTerminator::Return(Some(result)), + }; + + let mut prog = IrProgram { + functions: vec![IrFunction { + name: "and_like".to_string(), + blocks: vec![entry, right, false_path, end], + locals: vec![], + param_count: 0, + has_return: true, + source_span: Span::new(0, 0, 0), + }], + globals: vec![], + rom_data: vec![], + states: vec![], + start_state: String::new(), + }; + + optimize(&mut prog); + + // Find the false_path block post-optimization and verify that + // its LoadImm(result, 0) is still there. + let func = &prog.functions[0]; + let fp = func + .blocks + .iter() + .find(|b| b.label == "false_path") + .expect("false_path block should survive as reachable"); + assert!( + fp.ops + .iter() + .any(|op| matches!(op, IrOp::LoadImm(t, 0) if *t == result)), + "false-path LoadImm(result, 0) must survive optimization; block ops were {:?}", + fp.ops + ); +} diff --git a/tests/emulator/goldens/platformer.audio.hash b/tests/emulator/goldens/platformer.audio.hash index 03072e1..bf575e5 100644 --- a/tests/emulator/goldens/platformer.audio.hash +++ b/tests/emulator/goldens/platformer.audio.hash @@ -1 +1 @@ -7c8e60e4 132084 +a00949db 132084