test(gpu): make test-cuda-d1 actually exercise DECODE - #947
Open
MauroToscano wants to merge 1 commit into
Open
Conversation
`test-cuda-d1` set LAMBDA_VM_GPU_LDE_THRESHOLD=64 on the premise that 64 is "the exact LDE size of fib_iterative_1M's DECODE ROM". It is 32, and the reasoning behind the number was wrong too: DECODE's rows come from the ELF's executable words, not from cycles. fib_iterative_1M is 13 executable words (one 52-byte executable PT_LOAD; the variants differ only in the `li a0, <count>` immediate, so fib_iterative_16M is 13 too). 13 + 1 CPU-padding entry = 14 -> next_power_of_two() = 16 rows -> blowup 2 -> DECODE LDE 32. At threshold 64 that is below the gate, so DECODE failed the R1 split-tree commit, had no gpu_main() handle, and evaluate_dev declined - DECODE never reached the d=1 path at all. The counter could therefore only be fed by KECCAK_RC, the only other num_parts==1 table (a d=1 table is one with a single bus interaction), whose fixed NUM_ROWS=32 gives LDE 64 and passes `64 < 64` by one unit. So the target, the test name, the module docs and the assert message all named the one d=1 table guaranteed not to be exercised. No threshold fixes this with a fib fixture: DECODE (32) sits below KECCAK_RC (64), so <=32 engages both and 33..=64 engages only KECCAK_RC. Switch to all_instructions_64 - 66 executable words -> 128 rows -> DECODE LDE 256 - at threshold 128, where DECODE engages with 2x margin and KECCAK_RC declines, so a nonzero gpu_comp_h_slabs_calls() uniquely attributes to DECODE. 128 is also higher than the previous 64, so strictly fewer tables land on the GPU-committed path: it narrows rather than widens the R4 gather_proofs_dev abort site that gpu_lde.rs warns about for lowered thresholds. Tighten the test's own guard while here. `thr > 0 && thr < 1<<14` passed vacuously for any wrong value - including the 64 that caused this - so pin the window to (KECCAK_RC_LDE, DECODE_LDE] against named constants instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Targets
decode-deep-fri-gpu(#946), notmain— the code it fixes only exists on that branch.The problem
test-cuda-d1setsLAMBDA_VM_GPU_LDE_THRESHOLD=64on the premise that 64 is "the exact LDE size of fib_iterative_1M's DECODE ROM". It is 32, and the reasoning behind the number is also wrong: DECODE's rows come from the ELF's executable words, not from cycles.fib_iterative_1Mhas one executablePT_LOADof 52 bytes = 13 words (li a0, 199999expands tolui+addi).executor/src/elf.rsiterates exactp_memsz—p_alignis bound as_p_alignand never read, so there is no page rounding.next_power_of_two()= 16 rows → blowup 2 → DECODE LDE 32.gpu_main()handle, andevaluate_devdeclines. DECODE never reaches the d=1 path at all.So the counter could only ever be fed by KECCAK_RC — the only other
num_parts == 1table (a d=1 table is one with a single bus interaction), fixed atNUM_ROWS = 32→ LDE 64, passing64 < 64by one unit. The make target, the test name, the module docs and the assert message all named the one d=1 table guaranteed not to be exercised.Why the threshold alone can't fix it
DECODE (32) sits below KECCAK_RC (64), so with a fib fixture no threshold isolates DECODE:
<=32engages both,33..=64engages only KECCAK_RC. The fixture has to change — and nofib_iterative_*variant helps, since they differ only in theli a0, <count>immediate (fib_iterative_16Mis 13 words too).The fix
Switch to
all_instructions_64— 66 executable words → 128 rows → DECODE LDE 256 — at threshold 128. DECODE engages with 2× margin, KECCAK_RC (64) declines, so a nonzerogpu_comp_h_slabs_calls()uniquely attributes to DECODE.128 is also higher than the previous 64, so strictly fewer tables land on the GPU-committed path: it narrows rather than widens the R4
gather_proofs_devabort site thatgpu_lde.rswarns about for lowered thresholds.The test's own guard is tightened too.
thr > 0 && thr < 1<<14passed vacuously for any wrong value — including the 64 that caused this — so it now pins the window to(KECCAK_RC_LDE, DECODE_LDE]against named constants.Verification
Arithmetic verified by parsing both ELFs' program headers directly and reading the sizing chain (
elf.rssegment loop →decode.rspadding →with_blowup(2)).all_instructions_64is a known-good provable fixture (prove_elfs_tests.rs,disk_spill_tests.rs).Not executed — no GPU available here, so the change is arithmetically verified but unrun. One merge-queue Group 3 log line confirms it. Note this is also the run that settles whether KECCAK_RC currently engages at all: if it does not, the existing target has been failing, and this fix is what makes the group meaningful either way.
Trade-off worth a look
all_instructions_64is a small program, so at threshold 128 most other tables also decline the GPU path — the test covers the d=1 wiring on a minimal proof rather than inside a steady-state prove. The full chain still runs for DECODE (de-interleave → R2 commit → R3 OOD → R4 DEEP → FRI → openings) and the final verify is real. Having both would need a new ≥32-instruction loop fixture, which felt out of scope here.