Generalize reference unboxing to scalar replacement of aggregates - #8617
Generalize reference unboxing to scalar replacement of aggregates#8617cristianoc wants to merge 3 commits into
Conversation
071ed1c to
1c458b0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## lambda/remove-int-ref #8617 +/- ##
=========================================================
+ Coverage 77.32% 77.35% +0.02%
=========================================================
Files 467 468 +1
Lines 63318 63464 +146
=========================================================
+ Hits 48960 49090 +130
- Misses 14358 14374 +16
🚀 New features to boost your workflow:
|
Lam_pass_eliminate_ref turned a local single-field mutable block into a mutable variable when every use was a direct field access. Two restrictions were not required by the problem, only by its 1996 origin. It gave up on any closure mentioning the block. The comment beneath explained why: closures in a loop would share one binding, so an IIFE per iteration would be needed. That is the ES5 var problem. This backend emits block-scoped let, which already gives a fresh binding per iteration, so the restriction was guarding against a hazard the target no longer has. It also handled one field only, though the eligibility test - every use is an immediate Pfield or Psetfield on the block - is per field and says nothing about how many there are. Lam_pass_sroa lifts both. Eligibility is a separate analysis from the rewrite, so a failed check cannot leave a partly transformed term; escapes and rewrite are a matched pair and say so. Field indices are bounds checked, assignment to the block and any whole-block use are rejected, and initializer order is preserved by binding the fields in order. The traversal shares, so subtrees it does not touch are returned unchanged rather than rebuilt. The pass now runs as its own pipeline stage after simplify_lets rather than inside it. It had two call sites there, each destructuring Pmakeblock, one reachable only past an unused-variable check, and it consulted none of that pass's occurrence state. Generated output is identical either way. Multi-field scalars take their names from the record's tag info, so mario_game's five-field pressed_keys becomes pressed_keys_left through pressed_keys_bbox rather than pressed_keys$1 through $4. Single-field blocks keep the original binding name, which is why the reference cases show no churn. 24 files change, 272 insertions against 403 deletions. Escaping records still allocate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
The pass replaced every field of an eligible block with a mutable binding,
whether or not the field was ever read. Recording how each field is used costs
nothing - the eligibility walk already visits every occurrence - and decides
what each field needs.
A field that is never read needs no storage: its initializer and its writes are
kept only when they have effects. A field that is read but never written is
immutable, so a refined let will do. Only a field that is both read and written
needs a mutable scalar.
test_ramification shows the shape this is for. A ref written in both branches
of a match and never read afterwards disappears, and what is left folds:
let v = ref(0)
let y = switch x { | A(_) => v := 1; 3 | B(_) => v := 1; 4 }
- let v = 0; let y;
- if (x.TAG === "A") { v = 1; y = 3; } y = x.TAG === "A" ? 3 : 4;
- else { v = 1; y = 4; }
escapes becomes analyze, returning eligibility rather than escape so it can
report through the same walk. The short circuit still holds: on success every
occurrence has been visited, so the use table is complete, and on failure it is
discarded with the rejection.
Read-only fields go through refine_let, which may substitute the initializer at
its use sites - but only when is_safe_to_alias admits it, which is variables,
constants and module field reads. An effectful initializer read five times in a
loop, or three times through a closure, is still evaluated once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
1c458b0 to
8e80b80
Compare
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
cknitt
left a comment
cknitt
left a comment
There was a problem hiding this comment.
This is a very nice optimization!
|
Astra found a correctness issue in scalar replacement: an initializer or write to a field that is otherwise removable could be discarded even when evaluating its value may raise (the concrete reproducer was bigint exponentiation with a negative exponent). Fixed in commit 88e1004. The exception classification is centralized as |
The side-effect analysis listed bigint exponentiation and the bounds-checked array and string reads as pure, though 2n ** -1n throws and the checked reads throw out of range. Dead-code elimination then dropped an unused one, so a program that had to raise returned instead. Scalar replacement found the same hole from the other side, discarding a throwing write to a field it removes. Fix the classification in both places that make it: no_side_effects in Lam_analysis, where a bigint power is pure only with a nonnegative constant exponent, matching the rule already used for a divisor, and the three checked reads are effects; and Js_analyzer, where `**`, `/` and `%` are pure only when the right operand is a literal that cannot throw, since operand types are not known at that level. With the analysis right, scalar replacement needs no change: it already keeps a write value that has effects. A dead-binding test pins each case end to end, independently of scalar replacement. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
88e1004 to
7b26062
Compare
|
Replaced the fix in 7b26062, after the follow-up review found the earlier commit both incomplete and wider than the bug. The root cause is older than this PR:
Now the two existing classifications are fixed where they are made, with the conservative default they already had: a bigint power is pure only with a nonnegative constant exponent, matching the rule for a divisor; the checked reads are effects; and at the JS level Coverage: the original end-to-end test is unchanged; the unit test now asserts the value is kept rather than the replacement refused; three direct |
2 participants
Reference unboxing replaced a single-field mutable block with a scalar
binding. This generalizes it: any non-escaping local mutable block whose uses
are all direct field accesses becomes one binding per field, so multi-field
records and references captured by JavaScript closures are covered too.
Fields are classified by how they are used, read or written, so a field that
is only written does not force the block to stay.
Generated output changes where the optimization now applies; the diff shows
those snapshots.
Part of #8573. Stacked on #8616.