refactor(codegen): one place where a value becomes a local (provably IR-identical) - #10973
Closed
proggeramlug wants to merge 1 commit into
Closed
proggeramlug wants to merge 1 commit into
proggeramlug wants to merge 1 commit into
Conversation
`Expr::LocalSet`'s arm lowers its initialiser and then discharges every obligation that binding a value to a local carries: the alias addref, closure captures and boxed cells, the canonical-i32 slot, the plain-slot store with its shadow-frame and i32 mirrors, module globals, the arena owner, buffer views and int facts. All of it depends on nothing but `(ctx, id, v, value)`. Move it, VERBATIM, into `bind_lowered_value_to_local`; `LocalSet` becomes `lower_expr` plus a call to it. The caller this exists for is a read region's fast arm (PerryTS#10884, the slice after PerryTS#10946), which holds a value it loaded out of an object slot rather than one `lower_expr` produced. Today such a caller can only emit a bare store and remember each obligation separately, and a skipped addref there is a use-after-free that no fixture reliably catches. PerryTS#10946 handles that by DECLINING every binding whose obligations are not "a bare store", which is exactly why it cannot reach a binding whose initialiser refines its type. The guarantee wanted is structural rather than attentive: there is one place where a value becomes a local, and both arms of a region go through it. Nothing else changes, and both halves of that are checked rather than asserted: * VERBATIM. Applying exactly `value.as_ref()`->`value`, `*id`->`id`, `.get(id)`->`.get(&id)` (and `contains`/`contains_key`), `&v`->`v` and `source_id != id`->`*source_id != id` to upstream/main's tail yields the new function's 107-line body character for character. * IDENTICAL EMISSION. `PERRY_SAVE_LL` over 41 programs from `test-files/`, compiled by a compiler built from upstream/main and by this branch: 41 identical, 0 differ, 0 produced no IR. `cargo test --release -p perry-codegen -- --test-threads=1`: 2170 passed, 0 failed. fmt clean; no new clippy warning in the touched file.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe local binding logic in ChangesLocal binding extraction
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to This refactor centralizes local-binding bookkeeping without changing generated behavior, with reported identical output and passing checks. It is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
… slice 1 alone #10946 (step 4b slice 2) is held back: its region fast arm writes loaded values into bindings -- roots -- and emits no incremental-mark shading barrier for them (shadow_slot.rs: a pointer stored into a root after the collector scanned roots still has to be shaded). Counted by the lane: PERRY_INCREMENTAL_MARK_ BARRIER_ACTIVE_COUNT 32 region-off, 32 with slice 2, 40 with the fix. A missing shading barrier is invisible to every runtime probe. #10973 makes both arms share one binder, which is the durable fix. #10936 (slice 1) stays: its fast arm produces values, not bindings. Its region_read_run.rs has exactly one store -- an i32 miss counter into a state global -- so it writes nothing into a root and owes no barrier. With slice 2 gone, region_guard.rs does not exist, so the census callsite refreshed for it moves back to slice 1's region_read_run.rs: vs main, exactly one entry added, summary 42 -> 43, nothing removed. The knob registration still applies (slice 1 reads both PERRY_REGION_READS and PERRY_REGION_DIAG, and DIAG's census is still read only in ModuleDiag::drop); its comment no longer says 'both slices'.
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
… slice 1 alone #10946 (step 4b slice 2) is held back: its region fast arm writes loaded values into bindings -- roots -- and emits no incremental-mark shading barrier for them (shadow_slot.rs: a pointer stored into a root after the collector scanned roots still has to be shaded). Counted by the lane: PERRY_INCREMENTAL_MARK_ BARRIER_ACTIVE_COUNT 32 region-off, 32 with slice 2, 40 with the fix. A missing shading barrier is invisible to every runtime probe. #10973 makes both arms share one binder, which is the durable fix. #10936 (slice 1) stays: its fast arm produces values, not bindings. Its region_read_run.rs has exactly one store -- an i32 miss counter into a state global -- so it writes nothing into a root and owes no barrier. With slice 2 gone, region_guard.rs does not exist, so the census callsite refreshed for it moves back to slice 1's region_read_run.rs: vs main, exactly one entry added, summary 42 -> 43, nothing removed. The knob registration still applies (slice 1 reads both PERRY_REGION_READS and PERRY_REGION_DIAG, and DIAG's census is still read only in ModuleDiag::drop); its comment no longer says 'both slices'.
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
Contributor
Author
|
Landed on main in merge train 256 (#11018, v0.5.1638), main Carried at head Trains rebase-merge, so commits get new SHAs and GitHub cannot mark this PR merged. Closed as landed. |
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.
What
Expr::LocalSet's arm lowers its initialiser and then discharges every obligation that binding a value to a local carries: the alias addref, closure captures and boxed cells, the canonical-i32 slot, the plain-slot store with its shadow-frame mirror and its i32 mirror, module globals, the arena owner, buffer views, int facts. All of it depends on nothing but(ctx, id, v, value).This moves it, verbatim, into
bind_lowered_value_to_local.LocalSetbecomeslower_exprplus a call to it. Nothing else changes — see the two checks below.Why
The caller this exists for is a read region's fast arm (#10884, the slice after #10946), which holds a value it loaded out of an object slot rather than one
lower_exprproduced. Today such a caller can only emit a bare store and remember each obligation separately — and a skipped addref there is a use-after-free that no fixture reliably catches. #10946 handles that by declining every binding whose obligations are not "a bare store", which is why it cannot reach a binding whose initialiser refines its type.The guarantee wanted is structural rather than attentive: there is one place where a value becomes a local, and both arms of a region go through it. That is a stronger faithfulness argument than any review of a second implementation could give.
The two checks, because a verbatim move should be provable rather than asserted
1. It is verbatim. Take
upstream/main'sLocalSettail, apply exactly these rewrites, and the result is character-identical to the new function's 107-line body:No line differs beyond those. The move is mechanical and the diff reads as one with
--color-moved.2. It emits identical code.
PERRY_SAVE_LLon 41 programs fromtest-files/, compiled by a compiler built fromupstream/mainand by this branch, and the saved LLVM IR is byte-identical on all 41 (0 differ, 0 produced no IR).That second property is the whole risk of touching
LocalSetisolated into a step that provably changes nothing. If a later commit in the slice stack changes emission, it will be that commit, not this one.Tests
cargo test --release -p perry-codegen -- --test-threads=1: 2170 passed, 0 failed. fmt clean. Clippy reports no NEW warning in the touched file — the twoidentical blockswarnings it emits atliterals_vars.rs:1047/1049are pre-existing, in the Headers-method dispatch this PR does not touch, and have only shifted line number.No test is added. A refactor whose contract is "the IR does not change" is tested by the IR not changing; a unit test asserting that
LocalSetstill stores would pass equally on a version that dropped the addref, which is the failure this PR is preparing against.Summary by CodeRabbit