fix(verilog): a return inside a loop left the function and not the loop - #3021
Merged
Conversation
`__t27_ret` has existed at function scope since it was written, and every
lowered `return` already sets it. What was missing is that no loop tested
it, so a `return` from inside a loop stopped the function and left the loop
running.
The consequence is not uniform, so the repair is not either. `loop_cond()`
already routes per shape and the emitter had already decided which slot is
each loop's real termination test:
* a `while` the emitter could not bound joins the Verilog CONDITION --
the only slot that can stop it. Without it, a return on the branch that
advances neither bound means the loop cannot end at all.
* `for` / `for_range` / W700 take a BODY GATE, header untouched, because
the static trip count is the property gen_verilog_for_stmt exists to
protect. Those loops terminate by construction, so the guard stops the
writes and not the spinning.
61 sites take the condition, 23 take a body gate. Saying this "stops the
loop" is true of the 61 and false of the 23.
The gate is a scan of the loop BODY with subtree_has_return -- which does
NOT stop at a nested loop, the opposite of stage 1's break scan, because a
return escapes every enclosing loop. Gating on function scope alone, as the
first draft of this repair did, arms every loop in every non-clocked
function and moves 132 files instead of 34.
Measured:
* 34 of 643 generated files move -- the same 34 under four independent
rules: an AST scan of the parse tree, a begin/end lexer over the emitted
Verilog, and byte diffs on both the synthesis and simulation paths
* 84 guard sites: 61 while conditions + 22 for body gates + 1 W700 inner
if, plus 63 new intra-iteration barriers. Token arithmetic closes:
`!__t27_ret` goes 98 -> 245 = 183 `if (!__t27_ret) begin` (22 of which
are the for gates) + 61 + 1
* 0 for headers contaminated
* a probe of 4 declared tests, expected values confirmed by the Zig
backend: master 1 of 4 -> 4 of 4, with a no-return control passing both
ways
* the hang, through this project's own icarus-simulate: master exits 142 at
a 25s alarm having printed no [TEST] line at all; patched exits 0, PASSED
* iverilog on the 34: 0 accept before, 0 after. yosys: 14 before, 14 after
* mutation 8 of 8 -- three of the eight survived a first suite that looked
thorough, and each survivor was a test asserting something true about a
construct the change does not affect
* 72 seals re-sealed in this commit; tri seals drift reads 0 after
Not claimed: no corpus file demonstrates the hang under icarus-simulate,
because 0 of the 34 elaborate. The mechanism is demonstrated at the seam
and on a reduced probe. And the gate-level cost of a for body gate is
unmeasured -- 14 of 34 are yosys-readable and all 14 map to 0 cells in both
versions, because the functions are never instantiated.
Closes #2989
Refs #2988
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
PR DashboardGenerated at: 2026-09-03 12:25:57 UTC
Summary
Seal Status
|
Owner
Author
|
The acceptance reading the seal gate asks for, before 72 re-seals. Idle machine, whole corpus, both binaries: Identical to the digit, and predicted before the run rather than hoped: Re-sealing is a statement that the new output is the one we want. This is that statement. |
Contributor
PR DashboardGenerated at: 2026-09-03 12:36:32 UTC
Summary
Seal Status
|
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
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
1 participant
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.
A
returnleft the function and not the loop__t27_rethas existed at function scope since it was written, and every loweredreturnalready sets it. What was missing is that no loop tested it.The consequence is not uniform, so the repair is not either.
loop_cond()already routes per shape, and the emitter had already decided which slot is each loop's real termination test:whilethe emitter could not boundfor/for_range/ W700gen_verilog_for_stmtexists to protect61 sites take the condition and 23 take a body gate. Saying this "stops the loop" is true of the 61 and false of the 23.
The gate is a scan of the loop body with
subtree_has_return— which does not stop at a nested loop, the opposite of stage 1's break scan, because areturnescapes every enclosing loop. Gating on function scope alone, as the first draft of this repair did, arms every loop in every non-clocked function and moves 132 files instead of 34.Measured
whileconditions + 22forbody gates + 1 W700 innerif, plus 63 new intra-iteration barriers. The token arithmetic closes:!__t27_retgoes 98 → 245, and 245 = 183if (!__t27_ret) begin(22 of which are theforgates) + 61 + 1forheaders contaminatedicarus-simulate: master exits 142 at a 25-second alarm having printed no[TEST]line at all; patched exits 0, PASSEDiverilogon the 34: 0 accept before, 0 after.yosys: 14 before, 14 aftertri seals driftreads 0 afterThree mutants survived a suite that looked thorough
Each survivor was a test asserting something true about a construct the change does not affect — the class this repository has a section about, met three times in one hour:
whileand asked whether the firstif (!__t27_ret) begincame after the first__t27_ret = 1'b1;. That is satisfied by the function-level barriergen_verilog_fn_bodyhas always emitted below the loop. Now decided by indentation: a barrier at or below the loop's own indentation is the old one.testblock — and the synthesis path emits a test block as a comment, so it could not exercise the path at all. Now oncompile_verilog_for_simulation.if— a filter keyed onwhile (never sees it. Now scoped by declaration:reg __t27_ret;exists only inside a function body, so nothing after the lastendfunctionmay name the flag.Not claimed
icarus-simulate: 0 of the 34 elaborate, for reasons that predate this change. The mechanism is demonstrated at the seam and on a reduced probe, and that is the honest statement.forbody gate is unmeasured. 14 of 34 are yosys-readable and all 14 map to 0 cells in both versions — the functions are never instantiated, so no guarded loop is elaborated. Every cell figure in circulation is from a hand-written toy.Provenance
Designed by a map–design–attack fan-out (11 agents, 0 errors): four readers mapped the machinery, three built and measured competing patches in their own worktrees, a judge picked one, and three skeptics attacked the verdict. The judge's decisive finding was that the three designs were not alternatives — the winner is the exact disjoint union of the other two, 61 + 22 + 1 = 84, with byte-identical output on every overlap.
Closes #2989
Refs #2988