perf(codegen): #6794 follow-up (a) — region-scoped i32 chains in the ta_i32 masked-window copy#6844
Merged
Merged
Conversation
added 2 commits
July 26, 2026 10:10
…ta_i32 masked-window copy The straight-line masked-window region versioner (masked_window_region.rs) refines untyped-init numeric locals to Type::Number (f64) in every fast copy. bcryptjs _encipher inits its Feistel halves from a dynamic read (l = lr[off]), which is_strictly_i32_bounded_expr rejects, so l/r never earn a static i32 slot and every l>>>24 / l&0xff in the round pays a branchless ToInt32 tower. In the ta_i32 fast copy — where the masked array reads are already native i32 — bind such locals to a region-scoped i32 shadow slot (i32_counter_slots) at their refinement point, so the whole >>>/&/^/|0 bit-mixing chain lowers to native i32 (tower-free) and the double slot is maintained by every write via the existing LocalSet i32 path. The slot is removed at copy end so the plain_f64 / slow copies (which share ctx.i32_counter_slots and whose reads are f64) are unaffected. A local qualifies only if EVERY in-region LocalSet to it is strictly-i32-bounded (bitwise / |0 / Math.imul — a true signed i32; empty oracle, so copies are conservatively excluded) AND it is never un-refined, so every write maintains the slot and the value is always an in-range i32. Disabled for the plain_f64 tier, where an i32 slot would be maintained by no write. Unoptimized ta_i32 copy for the Blowfish-F shape drops from 186 ToInt32-tower selects to 0. LLVM -O already folds ~74% of those towers on its own; removing the residual is worth ~11% on bcryptjs.compareSync (Int32Array S-boxes -> ta_i32 tier). New gap test covers the round shape, overflow-wrapping, post-region full-Number reads, an un-refine on a fractional write, and the plain-Array tier — all byte-identical to Node.
📝 WalkthroughWalkthroughAdds region-scoped i32 shadow slots for eligible ChangesMasked-window i32 specialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RegionMatcher
participant lower_region_copy
participant i32_counter_slots
RegionMatcher->>lower_region_copy: pass eligible as_i32 refinements
lower_region_copy->>i32_counter_slots: bind scoped i32 shadow slots
lower_region_copy->>i32_counter_slots: remove bound slots after copy
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/stmt/masked_window_region.rs`:
- Around line 255-276: Update the write scan in the masked-window promotion
logic to recursively traverse expression children, matching the traversal used
by the existing strict-write collector. Ensure nested LocalSet and Update
operations are added to written/disqualified using the same strict i32-bounded
validation, so any nested non-strict write prevents as_i32 promotion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c8556d62-8119-4f7c-a99c-13ff4684a708
📒 Files selected for processing (4)
changelog.d/6844-region-i32-chains.mdcrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/src/stmt/masked_window_region.rstest-files/test_gap_region_masked_window_i32_chain.ts
Comment on lines
+255
to
+276
| for stmt in stmts { | ||
| match stmt { | ||
| Stmt::Expr(Expr::LocalSet(id, value)) => { | ||
| written.insert(*id); | ||
| let strict = crate::collectors::is_strictly_i32_bounded_expr( | ||
| value, | ||
| &empty, | ||
| &empty, | ||
| &empty, | ||
| &empty, | ||
| &mut |_| {}, | ||
| ); | ||
| if !strict { | ||
| disqualified.insert(*id); | ||
| } | ||
| } | ||
| Stmt::Expr(Expr::Update { id, .. }) => { | ||
| disqualified.insert(*id); | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recursively inspect writes before promoting an i32 slot.
This scan only sees top-level LocalSet/Update statements. A nested assignment or update inside an RHS is ignored, so a fractional/full-f64 write can leave as_i32 enabled and later reads can use a truncated i32 shadow. Traverse expression children, as the existing strict-write collector does.
Proposed fix
fn region_i32_bounded_write_locals(stmts: &[Stmt]) -> std::collections::HashSet<u32> {
let empty = std::collections::HashSet::new();
let mut written = std::collections::HashSet::new();
let mut disqualified = std::collections::HashSet::new();
+ fn visit_expr(
+ expr: &Expr,
+ empty: &std::collections::HashSet<u32>,
+ written: &mut std::collections::HashSet<u32>,
+ disqualified: &mut std::collections::HashSet<u32>,
+ ) {
+ match expr {
+ Expr::LocalSet(id, value) => {
+ written.insert(*id);
+ if !crate::collectors::is_strictly_i32_bounded_expr(
+ value, empty, empty, empty, empty, &mut |_| {},
+ ) {
+ disqualified.insert(*id);
+ }
+ visit_expr(value, empty, written, disqualified);
+ }
+ Expr::Update { id, .. } => {
+ disqualified.insert(*id);
+ }
+ _ => perry_hir::walker::walk_expr_children(expr, &mut |child| {
+ visit_expr(child, empty, written, disqualified);
+ }),
+ }
+ }
+
for stmt in stmts {
- match stmt {
- Stmt::Expr(Expr::LocalSet(id, value)) => { /* ... */ }
- Stmt::Expr(Expr::Update { id, .. }) => { /* ... */ }
- _ => {}
+ if let Stmt::Expr(expr) = stmt {
+ visit_expr(expr, &empty, &mut written, &mut disqualified);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for stmt in stmts { | |
| match stmt { | |
| Stmt::Expr(Expr::LocalSet(id, value)) => { | |
| written.insert(*id); | |
| let strict = crate::collectors::is_strictly_i32_bounded_expr( | |
| value, | |
| &empty, | |
| &empty, | |
| &empty, | |
| &empty, | |
| &mut |_| {}, | |
| ); | |
| if !strict { | |
| disqualified.insert(*id); | |
| } | |
| } | |
| Stmt::Expr(Expr::Update { id, .. }) => { | |
| disqualified.insert(*id); | |
| } | |
| _ => {} | |
| } | |
| } | |
| fn visit_expr( | |
| expr: &Expr, | |
| empty: &std::collections::HashSet<u32>, | |
| written: &mut std::collections::HashSet<u32>, | |
| disqualified: &mut std::collections::HashSet<u32>, | |
| ) { | |
| match expr { | |
| Expr::LocalSet(id, value) => { | |
| written.insert(*id); | |
| if !crate::collectors::is_strictly_i32_bounded_expr( | |
| value, empty, empty, empty, empty, &mut |_| {}, | |
| ) { | |
| disqualified.insert(*id); | |
| } | |
| visit_expr(value, empty, written, disqualified); | |
| } | |
| Expr::Update { id, .. } => { | |
| disqualified.insert(*id); | |
| } | |
| _ => perry_hir::walker::walk_expr_children(expr, &mut |child| { | |
| visit_expr(child, empty, written, disqualified); | |
| }), | |
| } | |
| } | |
| for stmt in stmts { | |
| if let Stmt::Expr(expr) = stmt { | |
| visit_expr(expr, &empty, &mut written, &mut disqualified); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/stmt/masked_window_region.rs` around lines 255 -
276, Update the write scan in the masked-window promotion logic to recursively
traverse expression children, matching the traversal used by the existing
strict-write collector. Ensure nested LocalSet and Update operations are added
to written/disqualified using the same strict i32-bounded validation, so any
nested non-strict write prevents as_i32 promotion.
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.
#6794 follow-up (a): region-scoped i32 chains
PR #6794 made untyped-param masked-window array reads fast (loop + straight-line
region versioner) and left two open follow-ups on the residual ~15× on bcryptjs's
unrolled
_encipher. This is (a): region-scoped i32 chains.Problem
The region versioner (
stmt/masked_window_region.rs) refines untyped-init numericlocals to
Type::Number(f64) in every fast copy. bcryptjs inits its Feistel halvesfrom a dynamic read —
l = lr[off]— whichis_strictly_i32_bounded_exprrejects, sol/rnever earn a static i32 slot. In the ta_i32 fast copy the masked arrayreads are already native
i32, but eachl >>> 24/l & 0xffin the round stillloads
las f64 and pays a branchless ToInt32 tower per op.Fix
In the ta_i32 copy only, bind a local whose every in-region write is
strictly-i32-bounded (bitwise /
| 0/Math.imul) and that is never un-refinedto a region-scoped i32 shadow slot (
i32_counter_slots) at its refinement point. Theexisting
LocalSeti32 path then keeps the whole>>>/&/^/| 0chain in nativei32 and maintains the double shadow. The slot is removed at copy end, so the
plain_f64/slowcopies (which sharectx.i32_counter_slotsand whose reads aref64) are unaffected. Soundness: an empty oracle keeps only truly-i32 writes (copies
excluded); the double shadow carries the correct signed value out for post-region reads.
Measured
selects → 0.-Oalready folds ~74% of those towers on its own (sitofp(i32)→ToInt32round-trips); removing the residual is worth ~11% on
bcryptjs.compareSync(Int32Array S-boxes → the ta_i32 tier). A real, free win on crypto/codec workloads;
the larger remaining gap is the per-call region probe + shadow-slot ops (follow-up b).
Correctness
New gap test
test_gap_region_masked_window_i32_chain.ts— the round shape,overflow-wrapping, post-region full-Number reads, an un-refine on a fractional write,
and the plain-
Arraytier — all byte-identical to Node. Also verified against theexisting i32-overflow / toint32 / typed-array / crypto gap suite and #6794's own
untyped-masked-window matrix.
Summary by CodeRabbit
Performance
bcryptjs.compareSync.Bug Fixes
Tests