Repository navigation
fix(compiler): box wide continuation and closure captures - #1788
sebastian-griego wants to merge 4 commits into
Conversation
Lorenzobattistela
left a comment
There was a problem hiding this comment.
Thanks! I merged this onto main 96c068f and ran the gates. test, ping, perf and safe all match main. I found no miscompile: both new tests pass, and so did two extra probes (a two-call fork holding several 67-word two-arm values shared after the call, and a closure capturing borrowed wide parameters the caller reads again). They match the interpreter at 1, 4 and 8 threads and in JS.
None of the points below are correctness issues. They are about boxing more than needed, and about code shape. comp.ts is right at its token cap, so take the ones that are cheap in tokens. If a fix costs too many tokens, skip it and say so.
-
emit_forkcounts values the call consumes (bind_capscall inemit_fork). The width is computed over every binding insc.uses, including ones the calls take as arguments. Because of this, defs that already fit get boxed. Intests/compile/record_wide_node.bend,sum4(a, b, c, d)holds 256 words. Afteraandbgo to the calls, only 128 words plus 2 results are saved. Main builds it with at most 193 params per segment. With this PR all fourR1s are boxed: 4 extraheap_allocper call, then 8 unboxes to passaandbright back. Counting only the bindings still used after the call (the continuation's live set) would leave these defs alone. -
Values that arrive boxed get boxed again. In
wide_capture_closure.bend,check(+a, +b, +c, +d)receives its four 67-word parameters boxed, through the wide-parameter rule infun_of. They are unboxed on entry (4×spare_free), thenbind_capsallocates four fresh 66-word nodes for the closure.suminwide_capture_calls.bendtakes the same unbox → box → unbox path. If the original box could be kept when the scope is already wide, both copies would go away. -
Every multiword binding gets boxed. Once the total passes
WIDE, all multiword captures are boxed, even small ones. Take one 240-word record plus ten 2-word pairs: boxing only the record fits, but this boxes all eleven. Boxing widest-first until the total fits would allocate less. -
emit_forkcopies the scope on every call.new Map(bind_caps(sc, [...sc.uses], results))builds a new array, a new Map and a new Scope for every fork or non-flat call, on every pass of the ownership fixpoint, even when nothing is boxed. Updating the entries in place (sc.uses.set(p, ...)) would avoid the copies, would drop thelet livereassignment inemit_clo, and would save tokens. -
The rule now lives in two places.
fun_of(const wide = ... > WIDE; ... wide && l.ks.length > 1 ? BOX : l) andbind_capskeep the same rule in two shapes (Lay[]vsBind). One helper over layouts plus an extra width could serve both. That would also make 3 a single change.
|
Updated in c5b3b07, based on current main b8664b0.
I shortened compiler comments to keep the file at 65,987 of 66,000 tokens. Validation: 53 programs pass at 1, 4, and 8 CPU threads and in JS. All three capture tests pass ASAN and UBSAN at those thread counts. The repository gate passes 54 / 54. The TypeScript check still reports 11 existing diagnostics, with none in |
Lorenzobattistela
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround! I merged c5b3b07 onto main b8664b0 and ran the gates. repo (54 / 54, comp.ts 65,987), test, ping, perf and safe all match main. I found no miscompile: the three new tests and four extra probes give the same output in the interpreter, JS and C, and main rejects every probe with "an arity over 247". The narrowed count in emit_fork also checks out. It leaves out exactly the bindings emit_args deletes, borrowed arguments keep their owners, and I couldn't build a frame that outgrows the check.
Please change these three before merge:
-
The kept-box check applies to the whole def, not to each parameter (
Lamcase). The test asks whether the def is wide, not whetherlay_limitboxed this particular parameter. So inside a wide def, every argument whose layout isBOXskips the unbox. That includes closure arguments, which always arrive asBOX. For example, in a widego(+a..+d: Bundle, +p: Duo), the closure+x => U32.add(x, x)emitsterm_keep(e, _x_0, 1)on a raw U32, andx => 5emitsterm_sinkon one. The same closures in a non-wide def emit neither. This is harmless today only because U32/Nat words carry tag 0, and ownership and hot facts still flow from a box that doesn't exist. Please make the test per parameter:fun_of(sc.def).lays[i]isBOXwhilelay_of(A)is not. Storing those flags onFuninfun_of, wherelay_limitalready knows them, also stops theLamcase from recomputing the def's total width for every binder. -
Please restore the unrelated comments. About 25 comments outside this fix were rewritten to fit the cap, including the file-header NOTE, and some of them lost information:
- "A spin of SPIN_FAR characters is a call on the device" → "SPIN_FAR selects device calls" (the threshold is gone)
- "An argument nested past TPL_DEEP brackets goes to a local" → "TPL_DEEP limits argument nesting" (the action is gone)
- "A frame slot holding a shared word takes the wrapped one" → "Frame slots store the shared word" (now wrong: the slot holds the
term_keep-wrapped word) - the
WL_LOADladder, and the brwl/own/hot names in the File comment, are gone
#1829 brings comp.ts down to about 65,414, so once it lands this PR fits without the rewrites. Please rebase on it then and keep only the comments this change actually needs.
-
wide_capture_mixed.benddoesn't test the new commit. It passes on 6b5e45b (before this commit) with the same output, so a regression to boxing every capture, or to unboxing incoming boxes, would go unnoticed. It needs a case that only fits with widest-first, for example a record plus enough Duos that the total stays overWIDEunless the record is boxed first. It could also use a case that fails without the kept box.
Optional, same as before: take these only if they're cheap in tokens.
-
Owners used once are still counted. An owned value with n=1, passed only to an owning parameter of the first call, still counts toward the limit even though
emit_argspops it. If it tips the total overWIDE, it gets boxed and then unboxed right away at that call. Leaving out owners whose only use is a non-borrowed argument ofx.v[0]would avoid that. -
Different defs box different parameters.
lay_limitpicks boxes per def and breaks ties by index. So two defs with the same parameter types but different counts box different parameters. For example,once(a, b, c, d, e, x)boxes a and b,sum(a, b, c, d, x)boxes only a, and every call fromoncetosumunboxes b. Widest-first moves this conversion cost between defs rather than removing it. A stable choice (by type, not index) might help, but it's not required here. -
The
bind_capscontract is implicit. It now returns void and updates its caller's array in place, and its doc comment is gone. A future caller that passes a fresh.filter()result would silently lose the boxing. A one-line comment, or settingsc.usesinsidebind_caps, would make that explicit.
|
Updated the parameter check, restored the comments, and added layout regression checks. Local tests pass. #1829 was closed without merging, so the rebase is pending and the token gate remains over its cap. |
Large continuation and closure captures can fail native compilation with
an arity over 247. This change boxes captures before saving them.Use one helper for function parameters and captures. Box the widest values until their width fits. Cache a flag for each parameter boxed by that helper, and preserve only those incoming boxes. Closure arguments and match fields use their normal layouts. Update capture bindings in place while retaining owners of borrowed arguments.
The regression tests cover calls, forks, closures, shared fields, and parent reads. Native layout assertions detect boxing every capture and unboxing a retained parameter. The original compiler comments are restored.
Related to #1069. Hundreds of separate one-word values and oversized combined call results can still exceed the limit.
Validation:
comp.ts.git diff --checkpasses.The token gate is pending: restoring the comments puts
comp.tsat 66,464 of 66,000 tokens. The requested upstream reduction, #1829, was closed without merging. This branch still needs that reduction before it can pass the gate.