refactor: version-based ThreadTimestamps (#2099) - #2122
Open
OlivierBBB wants to merge 2 commits into
Open
Conversation
Replace the base+offset stamp lattice (7 stampValue kinds, position-tagged bases resolved through a temps map) with SSA-style integer versioning: the k-th movement executed on a path advances the stamp to version k, and each version names one register -- version zero the row's entry register, the largest version live at any exit the canonical M$stamp_out (preserving the one-instruction cost of one-line functions), every other version a fresh temporary. Paths meeting with different versions are equalised by copies into the largest incoming version's register, on the deficient paths only; advances past a never-observed stamp (e.g. before a fail) are retracted. Deleted concepts: position-tagged bases and their two-phase resolution, branch normalisation, and the forced merge of agreeing-but-offset states. Emitted code on the linea riscv corpus: 8087 -> 7913 instructions (-174), row structure identical; 4 functions pay +12 total (write_16, read_32, full_round, read_and_pad_64) where sibling paths each define a version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the ZkC VM timestamp-threading transform to use SSA-style integer “versions” instead of the prior base+offset stamp lattice, simplifying merge handling and removing the need for position-tagged temporaries and resolution.
Changes:
- Replaces symbolic stamp “location” tracking with per-effect versioning (join = max), and performs merge equalisation by copying into the merged version’s register.
- Reworks the rewrite sweep to bind (effect, version) → register per row, advance versions on movements, and retract unused post-access “bumps”.
- Simplifies branch handling by removing prior branch-point normalisation, retaining only the jump-table canonicalisation special case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/zkc/vm/internal/transform/thread_timestamps.go | Updates the rewrite sweep to SSA-style version→register binding, merge equalisation via copies, and bump retraction for unobserved advances. |
| pkg/zkc/vm/internal/transform/stamp_analysis.go | Replaces the symbolic base+offset lattice with a forward DFA computing per-effect stamp versions (with join=max) and exit canonicalisation rules. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Closes #2099.
What
Replaces the base+offset stamp lattice with SSA-style integer versioning, as proposed in #2099: within one row, the k-th movement (memory access or effectful call) executed on a path advances the stamp from version k-1 to version k, and each version names one register.
The one rule added to the issue's sketch: the largest version live at any exit of a row binds to the canonical register (
M$stamp_out, or main's computed stamp). This is what preserves the guarantee that a one-line function costs exactly one added instruction — the advance past its single access is the exit's largest version, so it writesM$stamp_outdirectly. Advances whose result is never observed (e.g. past the last access before afail) are retracted.What this deletes
stampValuekinds with position-tagged bases (callOut@pc,norm@pc,merge@pc) and their two-phase resolution through thetempsmap (resolve()and its "unresolved stamp temporary" panic);stampNorm): a version is a register on every path, so branches need no parking temp;The dataflow value is now
{version, lit, canon}with join = max; auditing "the k-th access executed carries stamp_in + k" is a local check against the version at each access.Cost (linea riscv corpus,
zkc compile --bci, all 102 functions)Wins come from dropped merge copies and from calls returning their updated stamp directly into
M$stamp_out(e.g.interpreter−20,permutation−18,main−36). Four functions pay +12 total (write_16+2,read_32+2,full_round+4,read_and_pad_64+4): when two sibling branches each perform an access, both define the same version register, and the path that exits early keeps a definition only the other path consumes. Recovering those 12 requires either per-path register identities (the position tags this PR removes) or value-chain folding with reachability-based retraction — both were implemented and measured, and cost more complexity than this refactor saves, so the +12 is accepted deliberately.Testing
Test_ZkcUnit|Test_ZkcMixed|Test_ZkcInvalid), race detector, lint: green;🤖 Generated with Claude Code