Hand back the term when an optimization pass changes nothing - #8620
Open
cristianoc wants to merge 10 commits into
Open
Hand back the term when an optimization pass changes nothing#8620cristianoc wants to merge 10 commits into
cristianoc wants to merge 10 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac231a6212
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+250
to
+254
| let rec extract rev_wrap = function | ||
| | [] -> (rev_wrap, []) | ||
| | (_, binding) :: _ as bindings | ||
| when Lam_hit.hit_variables collections binding -> | ||
| (rev_wrap, bindings) |
There was a problem hiding this comment.
When a recursive group starts with a still-recursive binding but a later binding becomes independent after aux (for example, [a = ...b...; b = <now-independent block>]), the previous implementation examined the reversed group and hoisted that independent suffix. This forward walk stops immediately on a, so b remains in the recursive group and can be forced through recursive lowering, including dummy/update paths for blocks. Preserve tail-first extraction while retaining sharing, and add focused OUnit or generated-output coverage for this case.
AGENTS.md reference: AGENTS.md:L181-L186
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
I don't think this one holds — both the old and the new implementation extract a prefix, not a suffix.
Three things make the old code a forward walk:
- The old
iterended in| [] -> (List.rev groups, set), sogroupswas handed on in source order, not reversed. Ext_list.fold_left(compiler/ext/ext_list.ml:756) is| a :: l -> fold_left l (f accu a) f— a forward fold, head first.- The
stopflag latched: once any binding hitcollections, every later binding took thestop || …branch and stayed in the recursive group.
So the old fold extracted the maximal non-hitting prefix and left everything from the first hit onward recursive, which is precisely what the new extract does.
On your example [a = …b…; b = <now-independent block>]: a refers to b, so a hits collections, extraction stops at a, and b stays in the recursive group — under both implementations alike.
I also checked the two new arms that bypass lambda_of_groups: lambda_of_groups ~rev_bindings:[] result folds over an empty list and is the identity, and Lambda.letrec (compiler/ml/lambda.ml:662) is a plain constructor with no empty-list normalization. So Lambda.letrec groups body' matches the old path exactly, including the degenerate empty-bindings case.
Happy to reconsider if you're reading fold_left differently.
cristianoc
force-pushed
the
lambda/passes-share-terms
branch
from
September 4, 2026 12:47
ac231a6 to
c80c1a9
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## lambda/as-in-the-ast #8620 +/- ##
=======================================================
Coverage ? 77.39%
=======================================================
Files ? 474
Lines ? 63512
Branches ? 0
=======================================================
Hits ? 49156
Misses ? 14356
Partials ? 0
🚀 New features to boost your workflow:
|
cristianoc
force-pushed
the
lambda/passes-share-terms
branch
from
September 4, 2026 13:21
c80c1a9 to
0240fbf
Compare
cristianoc
force-pushed
the
lambda/passes-share-terms
branch
from
September 4, 2026 13:45
8869a30 to
8a3cd41
Compare
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
cristianoc
force-pushed
the
lambda/passes-share-terms
branch
from
September 5, 2026 00:18
8a3cd41 to
25bca19
Compare
cristianoc
force-pushed
the
lambda/passes-share-terms
branch
from
September 5, 2026 00:50
25bca19 to
05a5a6e
Compare
Member
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Every rewriting pass carried its own copy of a full Lambda traversal, around twenty arms per pass of the form | Lwhile (l1, l2) -> Lambda.while_ (simplif l1) (simplif l2) that rebuilt a node identically. They now delegate to Lambda_traverse.shallow_map_sharing, which rebuilds through the same smart constructors but returns the node untouched when no child changed. What is left in each pass is the arms that do something. The arms that were kept also rebuilt unconditionally when their own analysis found nothing, so simplify_alias now falls through to the sharing traversal in those cases rather than reassembling an identical node, and its string-switch arm guards on the scrutinee being a known constant instead of rewriting either way. Measured over a 149 module stdlib build, 1937 pass invocations: total allocation falls from 2570634 words to 1093262, and the number of runs that hand back their input unchanged rises from 256 to 1130. Generated JavaScript is unchanged throughout. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
simplify_exits counted exits, then walked the term a second time to rewrite them, whether or not there were any. Over a stdlib build 397 of its 447 runs have nothing to rewrite, so the counter now creates its table on the first static exit and reports None when it never does, and the pass returns its input without the second walk. An empty table is not the same as no work: a Lstaticcatch that nothing raises to is dropped by the pass, so a catch marks the term as having exits even though it adds no count. The occurrence and substitution tables in simplify_lets were sized 83 and 32 while holding far less. A no-op run of simplify_exits now costs 13 words rather than 91, and of simplify_lets 114 rather than 258. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
deep_flatten took every let apart into groups and reassembled it, so a chain that needed no regrouping came back as a fresh copy of itself. flatten only restructures a binding when it hoists something out of the right hand side, splits a null conversion, or eliminates a tuple; every other binding is emitted as the same binding, so aux now rebuilds those in place. The reassembly runs through Lam_util.refine_let, which is not a constructor: it promotes Strict to Alias when the right hand side is safe to duplicate, downgrades to StrictOpt, and inlines a binding whose body immediately consumes it. Skipping it would drop those rewrites, so the fast path still calls it, and refine_let takes the binding it is rebuilding and returns it untouched when nothing is refined. A beta residue is itself a let chain that flatten deliberately leaves alone, so it has to be recognized before classifying by shape. Over a stdlib build the runs that hand back their input rise from 108 to 253 of 447, rebuilds that produce an identical tree fall from 217 to 72, and the words spent on them from 181858 to 54902. The number of runs that actually change the tree is 122 either way, so no rewrite is lost. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
Three arms rebuilt their node whether or not anything under it changed: the jsx-preserve primitive mapped its remaining arguments eagerly, the cross-module application reassembled itself when the callee turned out not to be inlinable, and normal () in the applied-variable arm built a fresh list of arguments before deciding it had nothing to inline. The cross-module arm rebuilt with ?ap_transformed_jsx:None, dropping the flag the original application carried. Sharing keeps it, which changes no generated output anywhere in the suite. simplify_alias now never rebuilds a tree it did not change: over a stdlib build, 398 of its 447 runs hand back their input, none of the remaining 49 produce an identical tree, and 49 is what it changed before this too. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
The three arms that fall through to rebuilding a binding now share: the two that go through refine_let pass it the binding they are rebuilding, and the alias arm compares its parts before building a new node. That last one rebuilt with Lambda.let_ Alias v (simplif l1) (simplif l2) and simplif records substitutions as it walks, so the right to left evaluation of arguments means the body was simplified before the bound expression. Naming the results in reading order would have reversed that, so the order is now written out. Over a stdlib build, runs that hand back their input rise from 36 to 75 of 149 and rebuilds producing an identical tree fall from 41 to 2, with 72 runs changing the tree either way. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
The Lletrec arm of deep_flatten rebuilt unconditionally: it mapped every binding into a fresh list, split that with a fold carrying a stop flag, and reassembled through lambda_of_groups, whether or not a binding could be lifted out of the group. It now maps with sharing, replaces the fold with a walk that stops at the first binding referring back into the group, and returns the original when nothing was extracted and nothing underneath changed. That arm was where nearly all the remaining waste was. Over a stdlib build, runs of deep_flatten that hand back their input rise from 253 to 324 of 447 and rebuilds producing an identical tree fall from 72 to 1, with 122 runs changing the tree either way. The unit test covers the sharing rather than the extraction. Removing the sharing changes no generated output, so no snapshot can catch it, while breaking the extraction moves output the existing suite already compares. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
count_helper created its table on the first static exit and reported None when it never did, which meant threading a lazy accessor through the whole counting walk. A four line predicate answers the same question before counting starts, so the counter goes back to the shape it had and the pass returns its input untouched when there is nothing to rewrite. That predicate names the two nodes Lam_pass_exits rewrites, so a case added there that rewrites anything else has to be added here too or the pass silently stops firing. It says so. subst_helper also hands its term back when a retained catch or an unresolved raise comes through unchanged. The three tests cover what nothing else can. Removing either sharing site, or the removal of a catch nothing raises to, leaves the generated JavaScript byte for byte identical, and each mutation fails exactly one of them: a dead catch is dropped by code generation anyway, so its removal here is invisible to every output fixture we have. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
Reviewing the sharing series on clarity rather than allocation found three places where chasing the property made the code worse, all of them mine. simplify_alias's string switch had become a when-guard containing a match, which ran the same lookup twice and left an arm the guard makes unreachable. It now finds the constant once and branches on that. regroups_binding mirrors flatten's cases one for one, including why a null conversion of a variable is left alone while any other one is split. Nothing said the two have to stay in step, or that drifting costs the flattening silently, because the binding then takes the fast path and never reaches flatten at all. Two passes bound `as original` for a value already in scope as `lam`, giving one idiom two spellings across seven passes. The traversal every pass delegates to had no test. Breaking the sharing in its Lapply and Lswitch arms leaves every fixture in the repository byte for byte identical and no test failing, while all seven passes quietly lose the property. The new test checks each constructor twice: that an identity map hands the node back, and that a replacing map does not, since a node whose children were never visited would pass the first by doing nothing. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
The -debug-ir labels had drifted from the sequence they describe. "initial" was dumped after collapse_var_aliases rather than before it, "flatten1" and "before-simplify-exits" each dumped a term already dumped under another name, "simplify_alias_before" named the pass that came next rather than the one that had run, and the output of guard_raises was labelled simplify_lets. Every dump is now named after the pass whose output it holds, and the three rounds of deep_flatten, simplify_alias and simplify_exits are numbered so a dump can be placed in the sequence. The initial dump now happens before collapse_var_aliases, so it is the term the pipeline was handed. Removed the commented-out scc pass with its dump label, and the commented-out collect_info and simplify_alias that followed sroa. The area guide linked to lam_convert.ml, which no longer exists, and said six constructors normalize as they build. There are seven: apply, prim, switch, stringswitch, if_, seq and not_. It now also carries the pass sequence as a table, with which statistics each pass consumes: only simplify_alias reads them, and a fresh collect_info runs immediately before each of its three rounds. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
The stacked-PR guidance did not say whether you hand `gh stack link` branches or PR numbers, so it read as though the PRs had to exist first. Either works, and branches alone are enough: the command pushes them and opens the PRs it does not find. Say what linking does to the bases too. It moves each one onto the branch below, which looks like a misconfigured PR if you are not expecting it, and CI keeps running throughout. Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
cristianoc
force-pushed
the
lambda/passes-share-terms
branch
from
September 5, 2026 09:38
05a5a6e to
3ec0396
Compare
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
2 participants
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.
Every rewriting pass carried its own copy of a full Lambda traversal, around
twenty arms apiece of the form
that rebuilt a node identically. They now delegate to
Lambda_traverse.shallow_map_sharing, which rebuilds through the same smartconstructors but returns the node untouched when no child changed. What is
left in each pass is the arms that do something. The arms that were kept also
rebuilt unconditionally when their own analysis found nothing, so those hand
back the term too, and
simplify_exitsdecides up front whether the term hasany static exit at all rather than walking it twice to find out.
A note on what this is worth. Measured over the runtime and Belt corpora, the
Lambda optimization pipeline is about 1% of user CPU, so its isolated 8.4%
gain is roughly 0.08% of compile time and the end-to-end build difference is
indistinguishable from zero. The case for this change is that five passes lose
around 230 lines of boilerplate, and that "did this pass change anything"
becomes observable, which #8573 needs for reasoning about pass timing. It is
not a speedup and should not be read as one.
Sharing is invisible to generated output, so no fixture can see it regressing:
ounit_lambda_traverse_tests.mlchecks it directly, and the two per-passsuites cover the hand-written arms. The
-debug-irlabels are also correctedhere, having drifted from the passes whose output they name.
Part of #8573. Stacked on #8619, top of the stack.