mirror of
https://github.com/imjasonh/nescript
synced 2026-07-18 14:45:58 +00:00
fix(optimizer): preserve cross-block LoadImm uses in const_fold DCE
`const_fold_block`'s per-block dead-code pass was collecting temp usage from only the block it was folding, so a `LoadImm` whose destination is consumed by a *sibling* block (for example via the merge block's branch terminator) was incorrectly treated as dead and dropped. The `and` / `or` short-circuit lowering emits exactly that shape: the false path writes `LoadImm(result, 0)` and joins with the right path at an `and_end` / `or_end` block whose branch terminator reads `result`. After the DCE the false path's store was gone, leaving the zero-page result slot to carry whatever value the *previous* `and` / `or` evaluation had written there — stale data that bled into subsequent conditional branches. I found this while instrumenting `examples/platformer.ne` through a puppeteer-driven jsnes harness, stepping one frame at a time and snapshotting the full zero-page trace of each scenario (title-skip, hold-right, hold-left, jump-spam, coin-drift, enemy-stomp, long-run). In a clean idle run the enemy-1 stomp bounce (`rise_count = 6`, `fall_vy = 0`) fired at emulator frames 83 and 96 with `camera_x` = 61 and 74, i.e. with `e1_sx` = 39 and 26, nowhere near the intended `[72, 96)` pickup window. The trigger turned out to be the slot alias: every time `c2_sx` landed in its pickup window (so the coin-2 `and` stored 1 into ZP(130)) and the player was mid-fall at or past `player_y = 152`, the enemy-1 stomp `and` short-circuited to its false path, left ZP(130) at 1, and the stomp `if` fired on stale data. The fix is to compute function-wide source-operand usage once before folding each function's blocks and OR it into the per-block liveness check, so a LoadImm is only dropped if nobody — neither its own block nor any other block in the function — reads the temp. Added a regression test (`const_fold_preserves_loadimm_used_by_sibling_branch`) that builds the exact CFG shape the `and` lowering emits and verifies the false-path `LoadImm(result, 0)` survives optimization. Impact on the example ROMs: - `examples/platformer.nes`: enemy-1 stomp now fires only when `e1_sx ∈ [72, 96)`, as the source intends. The pixel golden is unchanged (`player_y` converges back to the ground line before frame 180), but the audio hash flips because the spurious `play hit` sfx calls during coin-2 passage are gone. Committing the new `tests/emulator/goldens/platformer.audio.hash`. - `examples/logic_ops.nes`, `examples/bitwise_ops.nes`, `examples/match_demo.nes`, `examples/mmc3_per_state_split.nes`, `examples/two_player.nes`: byte-different but observably unchanged — their pixel + audio goldens still match to the byte. They exercise `and` / `or` in the source and now compile through the corrected DCE. All other example ROMs are byte-identical to pre-fix. `cargo fmt`, `cargo clippy --all-targets`, `cargo test --release` (498 tests), and `tests/emulator/run_examples.mjs` (22/22 goldens) are clean. https://claude.ai/code/session_013Bi4H4YQ5or5HtMB4doUFi
This commit is contained in:
parent
59905147b4
commit
629fdcfce0
9 changed files with 111 additions and 8 deletions
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
|
|
@ -218,13 +218,22 @@ fn strength_reduce_block(block: &mut IrBasicBlock) {
|
||||||
/// destination temps are no longer referenced anywhere in the block.
|
/// destination temps are no longer referenced anywhere in the block.
|
||||||
fn const_fold(program: &mut IrProgram) {
|
fn const_fold(program: &mut IrProgram) {
|
||||||
for func in &mut program.functions {
|
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 {
|
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<IrTemp>) {
|
||||||
let mut constants: HashMap<IrTemp, u8> = HashMap::new();
|
let mut constants: HashMap<IrTemp, u8> = HashMap::new();
|
||||||
|
|
||||||
// First pass: fold arithmetic / comparison ops into LoadImm where possible.
|
// 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
|
// Second pass: remove LoadImm ops whose dest temps are no longer
|
||||||
// as source operands by anything else in the block (ops + terminator).
|
// referenced locally AND aren't referenced function-wide. The
|
||||||
let used = collect_used_temps_in_block(block);
|
// 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| {
|
block.ops.retain(|op| {
|
||||||
if let IrOp::LoadImm(t, _) = op {
|
if let IrOp::LoadImm(t, _) = op {
|
||||||
used.contains(t)
|
used_local.contains(t) || func_used.contains(t)
|
||||||
} else {
|
} else {
|
||||||
true
|
true
|
||||||
}
|
}
|
||||||
|
|
@ -397,7 +415,10 @@ fn collect_used_temps(func: &IrFunction) -> HashSet<IrTemp> {
|
||||||
used
|
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<IrTemp> {
|
fn collect_used_temps_in_block(block: &IrBasicBlock) -> HashSet<IrTemp> {
|
||||||
let mut used = HashSet::new();
|
let mut used = HashSet::new();
|
||||||
collect_used_from_block(block, &mut used);
|
collect_used_from_block(block, &mut used);
|
||||||
|
|
|
||||||
|
|
@ -436,3 +436,85 @@ fn inline_removes_trivial() {
|
||||||
.any(|op| matches!(op, IrOp::Call(..)));
|
.any(|op| matches!(op, IrOp::Call(..)));
|
||||||
assert!(!has_call, "call to trivial function should be removed");
|
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, <true-case>, 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
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -1 +1 @@
|
||||||
7c8e60e4 132084
|
a00949db 132084
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue