Recover array RANK and fix the logical-shift miscompile by macabeus · Pull Request #9 · macabeus/asmlift

macabeus · 2026-08-02T15:39:28Z

Targets kleod:CopyBGScrollTiles, one of the two rows on the board asmlift could not compile at all. Two capabilities, both closing silent-wrong classes. No levers — the row ends diff:32, not a match (see Not done below).

Board

asmlift  343 match / 168 nonmatch / 162 declined / 2 noncompile   (main)
     ->  345 match / 168 nonmatch / 162 declined / 0 noncompile
m2c      340 match, unchanged

0 lost, 2 gained. pnpm bench regression: 0 lost, 0 missing, 2 gained. test:offline 546, test:matching 277, typecheck + lint clean.

kleod:CopyBGScrollTiles:agbcc     noncompile(1) -> diff:32     m2c=noncompile(1)
kleod:UpdateHUDCounterDisplay     noncompile(1) -> diff:127    m2c=noncompile(1)

These were asmlift's only two noncompile rows on the entire board. m2c produces nothing usable on either, so both conversions are pure gain against the comparison.

The two gains were not targeted: pokeemerald:Random and FeebasRandom, PRNGs whose >> over a seed was silently compiling to asr.

Capability 1 — array rank

asmlift could not represent how many subscripts an array global takes, so it spelled every one with a single subscript. On the project's own extern u16 gBgTilemapBufs[4][0x400], gBgTilemapBufs[i] is a row: four incompatible types in assignment, and where a row address reaches an integer context, only a warning and a different address than the asm computed.

Not a one-row shape — the vendored contexts declare 576 multi-dim arrays in kleod alone, and the re-vendored maps carry 444 rank>1 array globals across the six projects.

SymbolInfo.dims ← @gba-kit/debug-info's new dims (Report an array's RANK, not just its flat element count gba-kit#7 — its arrayLength() multiplied the dimensions into one count).
arrayInnerExtents() is the single reading, shared by the access side and the declaration side so the two cannot disagree about an array's shape.
L3 index gains optional lead — the constant leading subscripts. The node still denotes one width-byte element, so its type, legalization and stride contract are unchanged; this is a spelling of the same address. g[0][i] is the idiom decomp sources use themselves.
declare.ts emits extern u16 g[][1024]; — inner extents exactly, outermost unsized as ever.
assertArrayDimsPresent gates it on key presence, same idiom as the cv-qualifier and pointee gates: a package that cannot report rank must not be read as "rank 1", because that is the silent case.

Capability 2 — shift direction

C spells the logical and the arithmetic right shift the same (>>) and picks from the left operand's type. engine.ts:199 documents this exact miscompile and fixes it only for widths 8 and 16, via the zext fold — which it can only do for widths C can name. Every other extract width, i.e. every bitfield read, was emitting C that computes a different value: *(u8 *)&gUnk_03005220 << 30 >> 30 promotes to int, so a 2-bit field holding 2 evaluates to -1.

BinOp gains '>>>' (logical right shift); shr_u lowers to it, shr_s to >>; backend/cfamily.ts synthesizes the operand cast that pins the choice, beside the deref-cast legalization that already worked this way. Pascal declines loudly on the operation.

Adversarial rounds

Two rounds, two reviewers each. Both fixes were right in output; one was wrong in place — and that was not visible from the diff or the board.

Round 1 found the placement two ways at once: a C spelling inside the language-neutral L3 tree broke the Pascal backend, and — independently — the structurer's variable environment holds params and locals only, so the rule typed gSym[i] from the access width and answered "signed" for every word load whatever the map said, leaving its own miscompile live on u32-element array globals (9,158 in the pokeemerald map). One move cured both, because the backend judges against declaredTypes(sfn), which includes shaped globals.

Round 2 found more than round 1: the fix had blinded the ranker (a wrong signedness pin became byte-equal to the right one — 31 signature flips, 20 on matching rows, quality.casts 424→522), and my first tie-break then inverted the named-over-raw preference (its regex skips pointer casts, so marioparty3:GWBoardRecordGet traded two named struct fields for anonymous byte offsets). Final ordering is one shared compareScored — score || group || castCount || order — used by both the CLI and the webapp drivers, which had silently diverged. 9 flips, none on a matching row.

Retraction

M2/M5 asserted since the divmod round that the sdiv-pow2 pattern moves the objdiff score. It does not — that movement was this bug: the raw lowering spelled the sign-bit shift as a bare x >> 31. Unfolded it now scores 0 on its own, so the pattern's payoff is readability. The tests say so in place rather than weakening the assertion quietly. M3's signedness pin moved from x >> 1 (no longer a discriminator) onto division, where __udivsi3 vs __divsi3 is a different relocation no spelling can mask.

Not done, deliberately

CopyBGScrollTiles needs three more things, verified by hand-editing the current output down to instruction-identical (50 → 8 → 0 differing instructions): D2 void-return inference, index-term association ((i<<1)+1) + (X<<5), and if-arm polarity. The last two are levers, and this repo's history says ungated levers cost matches — out of scope by explicit decision.

Dependency

@gba-kit/debug-info 0.5.0 is published (macabeus/gba-kit#7, merged and released); pnpm-lock.yaml is regenerated and CI is unblocked.

Development ran against a hand-linked local build, so two things had never been exercised for real and now have been:

The registry package reproduces the board byte-identically — re-vendoring gives the same maps (22,794 arrays, 444 rank>1, 0 missing dims) and a full bench run differs from the committed results only in temp-directory names inside recorded compiler diagnostics.
assertArrayDimsPresent was verified end to end against a real registry install of 0.4.0 (npm-packed, CLI temporarily pointed at it). loadSymbolMap refuses loudly with the upgrade advice rather than silently reading every array as rank 1. Until now only the unit test covered it, against a synthetic shape object.

🤖 Generated with Claude Code

…ount asmlift could not represent how many subscripts an array global takes, so it spelled every one with a single subscript. On `extern u16 gBgTilemapBufs[4][0x400]` — the project's own declaration — `gBgTilemapBufs[i]` is a ROW: four `incompatible types in assignment` errors, and, where a row address reaches an integer context, only a warning and a silently different address than the asm computed. kleod:CopyBGScrollTiles noncompile(1) -> diff:34 kleod:UpdateHUDCounterDisplay noncompile(1) -> diff:125 These were asmlift's only two noncompile rows on the board. m2c produces nothing usable on either (noncompile), so both conversions are pure gain against the comparison. Not a one-row shape: the vendored contexts declare 576 multidimensional arrays in kleod alone, and the re-vendored maps carry 444 rank>1 array globals across the six projects. The rank comes from @gba-kit/debug-info's new `dims` (its `arrayLength()` multiplied the DW_TAG_subrange dimensions into one count), REQUIRED by a capability gate on the same key-presence terms as the cv-qualifier and pointee gates: a package that cannot report rank must not be read as "rank 1", because that is the silent case. L3 `index` gains optional `lead` — the constant leading subscripts. The node still denotes ONE `width`-byte element, so its type, legalization and stride contract are unchanged; this is a spelling of the same address. Compared by exprEquals (it is part of the address), declined by the Pascal backend and by reindex, and excluded from the C++ receiver hook — three places that would otherwise DROP it silently. L2→L3 `bareArrayLead` is the one gate on the bare-name spelling, shared by both access paths. Rank > 1 pins the leading dimensions at 0 and puts the flat element index last (`g[0][i]`) — the same arithmetic, and what the reference source spells. decl `extern u16 g[][1024];` — inner extents exactly, outermost left unsized as ever. Access and declaration read the rank through ONE function (arrayInnerExtents), so an unspellable rank falls back to the `((T *)&g)[i]`/`extern T g[];` pair together. Requires @gba-kit/debug-info >= 0.5.0 (branch feat/array-rank-dims, unpublished).

C spells the logical and the arithmetic right shift the SAME — `>>` — and chooses between them from the LEFT OPERAND'S TYPE. asmlift rendered both `shr_u` and `shr_s` as `>>` and left the operand's C type to whatever the surrounding recovery happened to produce, so an `shr_u` over anything signed recompiled to `asr` where the target has `lsr` AND evaluated to a different value: `*(u8 *)&gUnk_03005220 << 30 >> 30` promotes to `int`, and a 2-bit field holding 2 comes out -1. This was already known for ONE width class. engine.ts's zext fold documents the same miscompile and fixes it by folding the whole shift PAIR to a cast op — which it can only do for widths C can name, so widths 8 and 16 were covered and every other extract width, which is to say every bitfield read, was left miscompiling. This closes the general case where the operand's C type is actually decided. kleod:CopyBGScrollTiles diff:34 -> diff:32 The operand type is settled by CONSTRUCTION, not inspection: exprCType is pointer-ness accurate and reports every integer as `s32` BY CONTRACT, so it cannot answer this. New `renderedIntSignedness` (l3/typing.ts, next to exprCType and documented as its complement) models the two rules that contract omits — integer promotion and the usual arithmetic conversions — and returns undefined wherever it does not reach. The structurer casts unless the answer is already the signedness the opcode needs, so undefined is safe: a redundant cast is codegen-identical, a missing one is a miscompile. Two goldens move, both because they were wrong: • ppc rlwinm extract — the rotate makes the shift logical; `(u32)a0 >> 5 & 255` • the return-trampoline loop — `shr_u` over an `s32`-declared v0; `(u32)v0 >> 1` Everything else is byte-identical: the promotion model is what keeps `~a0 >> 31` and `(a0 << 8 | 255) << 16 >> 16` cast-free, since both already render signed.

Vendored maps carry `dims` (444 rank>1 array globals across the six projects, 0 arrays without a rank). Full board, 675 rows: asmlift 343 match / 168 nonmatch / 162 declined / 2 noncompile -> 345 match / 168 nonmatch / 162 declined / 0 noncompile m2c 340 match, unchanged 0 lost, 2 gained (pokeemerald:Random, pokeemerald:FeebasRandom — both PRNGs whose `>>` over a seed was the arithmetic shift), 2 noncompile rows converted to scorable. Ten further rows narrowed (kleod:Decompress 10->9, pokeemerald:ModifyStatByNature 47->40, sa3:VramGetTotalAllocatedTiles 25->22, …); one widened, marioparty3:func_800600C0_60CC0 38->41, kept because no match is at stake and both capabilities are soundness fixes.

The toolchain matching suite had six failures, all one effect and all in the same direction: the shift-direction fix makes spellings that USED to lose bytes byte-exact, so tests asserting "this spelling is strictly worse" no longer hold. No match was lost — `pnpm test:matching` is 277/277 and the full board is 0 lost / 2 gained. ppc-mwcc extract, shr_and — golden text only (the match assertion is the next line and never ran). `(u32)a0 >> 5 & 255`; both still score 0. `shr_and`'s source shifts an `unsigned` and the old spelling matched only because `& 15` masks the difference away. M3, mips-controlflow — `x >> 1` is no longer a signedness discriminator, because the C now states the shift direction instead of leaving it to the parameter's declared type. Rather than delete the pin, M3 is retargeted onto DIVISION, where the choice cannot be spelled away at all (`__udivsi3` vs `__divsi3` is a different relocation): unsigned scores 0, signed scores 1. The `x >> 1` case stays as an explicit control on what changed. The MIPS test keeps its real subject, which is scorer DISPATCH. M2, M5 — RETRACTION. These asserted the sdiv-pow2 pattern MOVES the objdiff score. It does not, and the movement it used to show was this branch's bug: the raw lowering spelled the sign-bit shift as a bare `x >> 31`, C's arithmetic shift, where the target has `lsr`. Unfolded it now scores 0 on its own, so the pattern's payoff is READABILITY (`a0 / 2` over a shift tree), not bytes. The tests now assert what is true — a fold never costs bytes and lands byte-exact — and say so in place.

…e tower Adversarial round, both reviewers. The shift fix was correct in output and wrong in placement, and the placement caused a real defect in each direction. ARCHITECTURE (reviewer B). `docs/asmlift-101.md`: "every language-specific decision lives in a backend, never in the tower". Rendering `shr_u` as `>>` plus a `(u32)` cast baked a C spelling into the language-neutral L3 tree — and `l3/ast.ts` had already made exactly this call for the sibling case ("scalar deref casts are backend-owned"). The tower was DESTROYING the shr_u/shr_s distinction and then reconstructing it from a cast. L3 `BinOp` gains `>>>`, the LOGICAL right shift. `shr_u` lowers to it, `shr_s` to `>>`. 20 inhabitants across 5 projects and 4 compilers, so it earns the level; the rotate idiom now states `>>>` too rather than relying on the rotated value's recovered unsignedness. cfam `shiftOperand` synthesizes the cast, beside `legalized` — same discipline, one operator over. C output is byte-identical. pascal `>>` keeps `rshift` (byte-exact vs upas, pascal-ido `asr2`). `>>>` has no verified IDO Pascal spelling and reaches the loud decline — on the OPERATION, with a comprehensible message. Before this commit Pascal declined on an incidental cast node, having previously emitted `rshift` for a logical shift: silently wrong. CORRECTNESS (reviewer A, F1, HIGH). The structurer's variable environment holds params and locals only, so it typed `gSym[i]` from the access width and answered "signed" for every word load whatever the map said — leaving the miscompile in place SILENTLY on a u32-element array global (9158 such arrays in the pokeemerald map alone). The backend judges against `declaredTypes(sfn)`, which includes shaped globals, so moving the rule fixes this as a side effect. Pinned: `(s32)gTable[a0] >> 4`. QUALITY (reviewer A, F2). The cast made a wrong signedness pin byte-equal to the right one, so the ranker's enumeration-order tie-break silently installed the noisier spelling: 31 signature flips, 20 on MATCHING rows, `quality.casts` 424->522. `rankBy` now breaks a score tie on cast count before enumeration order — a tie means the axis did not change the bytes, so the readable spelling should win. Flips 31->9, none on a matching row; casts 424->463, the residue being casts that are load-bearing. Also remediated: • cfamily lead+legalized and the dot-form field base — two paths that would have spelled a ROW's address; both now refuse (A/F4, B). • apps/web cpp-spec.ts — the diverged copy of core's C++ receiver-hook predicate (A/F5). • `renderedIntSignedness` const arm: C89 gives `-2147483648` an unsigned type, since it lexes as unary minus on a constant too large for `int` (A/F7). • the unequal-rank hole in the usual-arithmetic-conversions clause is now documented as sound-because-core-has-no-64-bit-integer, rather than left unremarked (B). • new packages/core/test/array-rank-guards.test.ts pins the whole `lead` safety perimeter — exprEquals, both backend refusals — which was the argument for `lead` being an optional field and was untested (B). Board unchanged and re-verified: 345 match / 168 nonmatch / 162 declined / 0 noncompile, 0 lost, 2 gained. test:offline 538, test:matching 277.

…ator Adversarial round 2, both reviewers, on round 1's own remediation. The cast-count tie-break I added in dadc3a2 was ranking spellings that are not alternatives of the same thing. Its regex skips POINTER casts, so a `/raw-globals` candidate's hoisted `(u8 *)&gSys` base costs 0 while the named sibling's `(s32)` costs 1 — a systematic bias toward the raw form, inverting the named-over-raw preference the same comment block claims to protect. Observed on marioparty3:GWBoardRecordGet, where two named struct-field spellings were traded for anonymous byte offsets to save one cast. Candidate gains `group` — the symbol-variant index, carried structurally instead of inferred from enumeration order. Group beats cast count, so readability only ever arbitrates WITHIN a set of genuine alternatives. castCount now carries quality.ts's ADDRESS-CAST exemption: `(u32)&gSym` is the correct source spelling of integer arithmetic on a link-time address, not noise. The two readability counters must agree, or ranking optimizes for what the report then penalizes. The webapp had NOT been updated and its comment claimed it mirrored rankBy "exactly as rankBy spells it" — so the playground and the CLI picked different winners for the same function on 10 agbcc rows. The comparator is now `compareScored`, exported from core and called by both drivers. A duplicated ordering policy is how they diverged; this is the same defect dadc3a2 fixed in cpp-spec.ts and then re-created next door. Also from round 2: • `renderedIntSignedness` moves to backend/cfamily.ts. Round 1's complaint was "a C rule in the tower"; the rule moved out of structure/ but its implementation stayed in l3/, with a single C-backend consumer. `exprCType` stays — Pascal consults it too. • l3/ast.ts's BinOp comment claimed the split rule is "the machine keeps them apart". That rule would license splitting udiv/umod/icmp_u* too, with no inhabitant — exactly what "earn the level" forbids. It now states the real rule (a byte-load-bearing divergence with ~20 inhabitants and no other channel) AND that the collapsed operators carry the same latent hazard, tolerated only because no row has produced it. • raise/recover.ts's rotate comment still claimed the unsigned seeding was what kept the `>>` spelling round-tripping. The idiom states `>>>` on the node now; comment corrected rather than left as a second, stale owner of the same guarantee. • contracts.ts NO_PTR_OPS is `Set<BinOp>` — the next operator is a compile error, not a memory test, which is what PREC's totality already gives the backend. • array-rank-guards.test.ts pins the ordering (5 cases), the dot-form refusal, and Pascal's `>>>` decline — every behaviour round 2 found unpinned. test:offline 546, test:matching 277, typecheck + lint clean.

The results committed in dadc3a2 were stamped {commit: b0da01b, dirty: true} — numbers measured on an uncommitted tree, attributed to a commit that does not contain the fix, so nobody could reproduce them from the stamp. apps/benchmark/src/report/stale-check.ts exists to forbid exactly this. Re-run and re-merged on a clean HEAD: {commit: 005cccf, dirty:false}. Board unchanged, and unchanged from round 1 — the two remediation rounds were placement, ranking and provenance, not behaviour: asmlift 343 match / 168 nonmatch / 162 declined / 2 noncompile (main) -> 345 match / 168 nonmatch / 162 declined / 0 noncompile m2c 340 match, unchanged 0 lost, 2 gained (pokeemerald:Random, pokeemerald:FeebasRandom) Readability after the group-aware ordering: 9 signature flips, NONE on a matching row (dadc3a2 alone had 31, of which 20 were on matching rows); quality.casts 424 -> 463, down from 522 before the tie-break, the residue being casts that are load-bearing. marioparty3:GWBoardRecordGet keeps its named `GwSystem.field` spelling.

`packages/cli/package.json` has required `^0.5.0` since the array-rank commit, but the lockfile still recorded `^0.4.0` — a mismatch that fails every `--frozen-lockfile` workflow. 0.5.0 is now on the registry (gba-kit `935f8ee`), so `pnpm install` resolves it and this is just the regenerated lock. Development ran against a hand-linked local build, so the published package and the capability gate had never been exercised for real. Both now are: • The registry package produces BYTE-IDENTICAL vendored maps (22794 arrays, 444 rank>1, 0 missing dims) and a byte-identical board — a full `bench run` differs from the committed results only in temp-directory names inside recorded compiler diagnostics. 345 match / 168 nonmatch / 162 declined / 0 noncompile, unchanged. The committed results therefore stay stamped at 005cccf, which does still reproduce them. • `assertArrayDimsPresent` was verified END TO END against a real registry install of 0.4.0 (npm-packed, CLI temporarily pointed at it): `loadSymbolMap` refuses loudly with the upgrade advice instead of silently reading every array as rank 1. Until now only the unit test covered it, on a synthetic shape object. test:offline 546, test:matching 277, typecheck + lint clean.

macabeus added 7 commits August 2, 2026 15:39

macabeus mentioned this pull request Aug 2, 2026

Report an array's RANK, not just its flat element count macabeus/gba-kit#7

Merged

macabeus changed the title ~~Recover array RANK and fix the logical-shift miscompile (343→345, noncompile 2→0)~~ Recover array RANK and fix the logical-shift miscompile Aug 2, 2026

macabeus merged commit 55b4ee3 into main Aug 2, 2026
2 checks passed

macabeus deleted the match/CopyBGScrollTiles branch August 2, 2026 16:05

Navigation Menu

Recover array RANK and fix the logical-shift miscompile - #9

Recover array RANK and fix the logical-shift miscompile#9
macabeus merged 8 commits into
mainfrom
match/CopyBGScrollTiles

macabeus commented Aug 2, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

macabeus commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Board

Capability 1 — array rank

Capability 2 — shift direction

Adversarial rounds

Retraction

Not done, deliberately

Dependency

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

macabeus commented Aug 2, 2026 •

edited

Loading