mirror of
https://github.com/imjasonh/nescript
synced 2026-07-17 06:02:01 +00:00
review: tighten PRNG / void-intrinsic / FCEUX path handling
Follow-up cleanup on the cc65 parity batch. Addresses issues found during a post-commit code review. **Correctness fixes:** - `rand8()` / `rand16()` at statement position (result discarded) were being eliminated by DCE because `op_dest` returned `Some(dest)` for Rand8/Rand16 even though the ops have a visible side effect — advancing the PRNG state. Now `op_dest` returns `None` for both, keeping the JSR regardless of liveness. New regression test `rand8_statement_survives_dce`. - Void-only intrinsics (`poke`, `seed_rand`, `set_palette_brightness`) used in expression position (e.g. `var x = seed_rand(42)`) were panicking the linker with an unresolved `__ir_fn_X` label. The analyzer now emits E0203 with a clear message; new `void_intrinsic_in_expression_position_errors` test covers all three names. - Statement-position `rand8()` / `rand16()` weren't lowered at all (they fell through to the default Call path). Now both lower to their IR op with a fresh temp that nothing reads; the JSR still runs so the PRNG state advances. - `--fceux-labels foo.nes` was producing `foo.0.nl` because `PathBuf::with_extension` replaces instead of appends. Rewritten to literally append `.<bank>.nl` / `.ram.nl` to the OsString, so users get the FCEUX-expected `foo.nes.<bank>.nl` naming. - Linker now asserts CNROM / AxROM don't accept user-declared switchable PRG banks — their page sizes don't fit the 16 KB per bank model, and silently producing a mis-sized ROM is worse than a loud panic. **PRNG cleanup:** - Removed the stream-of-consciousness comment block in `gen_prng` that described three abandoned algorithms before landing on the actual Galois LFSR. - Simplified `__rand16` to a single JSR + LDX instead of two JSRs + TAY/TYA round-trip — a single shift already produces 16 fresh bits, the doubled call just burned ~40 cycles. The golden PNG for `prng_demo` was regenerated to reflect the new sequence. - Rewrote the `gen_prng` doc comment to accurately describe the algorithm as a Galois LFSR (it was mislabelled as xorshift). - Rewrote the `gen_palette_brightness` doc comment with a proper table of level→mask mappings — the prior prose description didn't match the actual table values. **Tests:** - Three new unit tests in `linker::debug_symbols` covering the FCEUX `.nl` renderer: user-facing labels only, empty output when no user labels exist, and deterministic sorting in `.ram.nl`. - Extended `nes2_mapper_high_nibble_in_byte_8_is_zero_for_small_mappers` to cover AxROM + CNROM. - Renumbered priority list in future-work.md after removing the shipped sections (J, K, N, parts of V and Y). All 737 tests + 40/40 emulator goldens still green.
This commit is contained in:
parent
a924dcc59c
commit
f4968256f4
13 changed files with 337 additions and 166 deletions
|
|
@ -3072,6 +3072,53 @@ start Main
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rand8_statement_survives_dce() {
|
||||
// `rand8()` at statement position (result discarded) has a
|
||||
// side effect — advancing the PRNG state — so DCE must not
|
||||
// eliminate the JSR. Regression test: earlier the op_dest
|
||||
// helper returned Some(dest) for Rand8, which let DCE drop
|
||||
// statement-level draws where the temp was unused.
|
||||
let source = r#"
|
||||
game "StmtRand" { mapper: NROM }
|
||||
on frame {
|
||||
rand8()
|
||||
rand16()
|
||||
}
|
||||
start Main
|
||||
"#;
|
||||
let (program, _) = nescript::parser::parse(source);
|
||||
let program = program.unwrap();
|
||||
let analysis = analyzer::analyze(&program);
|
||||
let mut ir_program = ir::lower(&program, &analysis);
|
||||
optimizer::optimize(&mut ir_program);
|
||||
let sprites = assets::resolve_sprites(&program, Path::new(".")).unwrap();
|
||||
let sfx = assets::resolve_sfx(&program).unwrap();
|
||||
let music = assets::resolve_music(&program).unwrap();
|
||||
let mut codegen = IrCodeGen::new(&analysis.var_allocations, &ir_program)
|
||||
.with_sprites(&sprites)
|
||||
.with_audio(&sfx, &music);
|
||||
let mut instructions = codegen.generate(&ir_program);
|
||||
nescript::codegen::peephole::optimize(&mut instructions);
|
||||
let linked = Linker::new(program.game.mirroring).link_banked_with_ppu_detailed(
|
||||
&instructions,
|
||||
&sprites,
|
||||
&sfx,
|
||||
&music,
|
||||
&[],
|
||||
&[],
|
||||
&[],
|
||||
);
|
||||
let rand8_addr = *linked
|
||||
.labels
|
||||
.get("__rand8")
|
||||
.expect("statement-level rand8() should still link in the PRNG routine");
|
||||
assert!(
|
||||
contains_jsr_to(&linked.rom, rand8_addr),
|
||||
"rand8() statement should keep its JSR through the optimizer"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn set_palette_brightness_lowers_to_jsr() {
|
||||
let source = r#"
|
||||
|
|
@ -3216,6 +3263,40 @@ start Main
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn void_intrinsic_in_expression_position_errors() {
|
||||
// `seed_rand(...)`, `set_palette_brightness(...)`, and `poke(...)`
|
||||
// don't produce values, so using them in expression position
|
||||
// should be a compile-time error (not a linker panic later).
|
||||
let cases = [
|
||||
"var x: u8 = seed_rand(42)",
|
||||
"var x: u8 = set_palette_brightness(4)",
|
||||
"var x: u8 = poke(0x0200, 1)",
|
||||
];
|
||||
for frag in cases {
|
||||
let source = format!(
|
||||
r#"
|
||||
game "BadVoid" {{ mapper: NROM }}
|
||||
on frame {{
|
||||
{frag}
|
||||
}}
|
||||
start Main
|
||||
"#
|
||||
);
|
||||
let (program, _) = nescript::parser::parse(&source);
|
||||
let program = program.expect("parse should succeed");
|
||||
let analysis = analyzer::analyze(&program);
|
||||
assert!(
|
||||
analysis
|
||||
.diagnostics
|
||||
.iter()
|
||||
.any(|d| d.is_error() && d.message.contains("does not return a value")),
|
||||
"void-intrinsic in expr position should be an error; fragment was `{frag}`, got {:?}",
|
||||
analysis.diagnostics
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rand8_no_args_mismatch_errors() {
|
||||
let source = r#"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue