Repository navigation
Conversation
When each arm of an if/else chain allocates one node of the same class outside any inner block, allocate it once before the chain. Clang then turns the arms' stores into selects instead of a branch on the tag. The JS backend never emits heap_alloc, so emit_chain skips the scan there and returns after each arm's body.
Main is at 65,909 of 66,000 tokens and the hoist takes it to 66,085.
These comment sentences restate code a few lines away, and cutting them
brings comp.ts to 65,975:
- the header's "The C and JS runtimes close the file" (the RuntimeC and
RuntimeJs section headers);
- char_new's "The JS runtime holds this function's text" (the JS
runtime's `const char_new = ${char_new};`);
- the Globals comment's device-program source (BEND_SRC, below it);
- the Gpu comment's "a launch loads it or compiles" (both gpu_load
bodies);
- the Compile comment's "An effect source is read once, in one
namespace" (effect_src's FL.effs check and its error);
- the C Io comment's request and parked-request sentences (io_step,
io_wait_on and io_wait);
- the JS Io comment's "Waits stay in deadline order" (io_park_on's
sorted insert).
|
@Lorenzobattistela per your request |
Lorenzobattistela
left a comment
There was a problem hiding this comment.
Thanks for splitting this out. I merged it onto main 5ce1e07 and ran all the gates: repo 54/54, test 1697/1697, perf 124/124, ping 73/73, safe 0 false rejects. comp.ts is at 65,678 / 66,000. I found no correctness bug in the hoist. Three things before merging:
-
The hoisted alloc now runs before frees in the arm. Some arms free a node and then allocate one. In tests/io/tcp_try.bend, the else arm used to do
term_sink(e, _m_4); u64 _nd_8 = heap_alloc(e, cls_fit(1));, so the alloc reused the slot that was just freed, which is still in cache. It now allocates before theif, so it takes a different slot and the peak heap is one node larger. A recursive function that frees one node and builds one per step pays this on every step, and the benches only measured arms that don't free first. Could you skip the hoist when an arm frees before its alloc, or show this costs nothing on such a loop? -
Arms are compared by the text of the heap_alloc call, not by size class.
heap_alloc(e, cls_fit(3))andheap_alloc(e, cls_fit(4))are both class 2, but they are different strings, so that chain is never hoisted. The new comment says "one node of one class". Emitting the class as a literal in ctr and seg_clo (heap_alloc(e, ${cls_fit(n)})) would match the comment and catch those chains. -
Please keep two of the cut comments. With the hoist, comp.ts has about 320 tokens to spare, and these two say things the code nearby doesn't:
- "The JS runtime holds this function's text" (char_new). It is the only warning that char_new's source is pasted into every JS program (line 488), so the function has to stay self-contained. If it ever calls a comp.ts helper, it still type-checks, but every JS program throws a ReferenceError at run time.
- "Waits stay in deadline order" (JS Io). io_wait (line 6444) and io_run (line 6505) read io.waits[0] as the earliest deadline, far from the sorted insert in io_park_on.
Optional: the hoist finds the alloc by regex-matching the emitted C, and treats a { as the only sign of a block. If node_fill's line format changes, the optimization silently turns off. WL_JMP, WL_RETN and WL_AGAIN contain no {, so an arm that jumps out before its alloc would still be hoisted. Recording the alloc in the Scope when ctr or seg_clo emits it would avoid reading C back, and would also drop the u64 _nd_k = _nd_K; alias each arm keeps now.
An arm that frees a node before it allocates one (tcp_try's else arm sinks a term first) let the alloc reuse the freed slot. Hoisting the alloc above the chain took a different slot and grew the peak heap by a node. Now an arm qualifies only if its first line is its one alloc, so nothing in it, a free, a jump or an exit, runs before the alloc. ctr_build and seg_clo write the size class as a literal, so arms are compared by class, as the comment says, not by word count. Keep char_new's "The JS runtime holds this function's text" and the JS Io comment's "Waits stay in deadline order". Main has reached 66,000 tokens since the review, so these comment sentences come out instead, each restating the code right below it, which brings comp.ts merged with main to 65,998: - the C-only ops' list of their own four names (shortened); - compile_book's "the datatypes of the constructors a pure main prints are roots too" (the file_book call below it); - gpu_pass's "A bang launches 1, 2, 4 .. 64 rounds between syncs, until the pack empties the frontier" (its loop); - io_loop's "A busy loop still checks due timers, and fds periodically" (the block below it); - io_list's "io_unlist answers null if a value is past 255" (io_unlist, a few lines down).
|
Thanks for the review. Since then #1542 landed and brought main's comp.ts to exactly 66,000 / 66,000, so this PR merged with main read 66,066. I merged main (b8664b0) into the branch rather than rebasing, so there is no force push. The fixes are in 5d0bab8.
Optional: I kept reading the C back, but narrowed what it accepts.
Checks, on the branch (main is merged in):
I couldn't run perf, ping or safe here; they need the minis. |
What changes
This is commit 2 of #1596 on its own, as asked in the closing review there, now merged with main at b8664b0. Everything is in
bend2/comp.ts.if/elsechain opens with its one node allocation, all of one size class, with no inner block,emit_chainallocates the node once before the chain. Before, the inlineheap_allocfast/slow branch in each arm kept clang from merging the arms. Now their stores become selects instead of a branch on the tag.ctr_buildandseg_clowrite the class as a literal (heap_alloc(e, 2), notheap_alloc(e, cls_fit(3))), so arms are compared by class.heap_alloc, so onFL.jsthe arm callback returns right after the arm's body and scans nothing.compile_book's "the datatypes of the constructors a pure main prints are roots too" (thefile_bookcall below it);effect_src'sFL.effscheck and its error);BEND_SRC, below it);gpu_loadbodies);gpu_pass's "A bang launches 1, 2, 4 .. 64 rounds between syncs, until the pack empties the frontier" (its loop);io_step,io_wait_onandio_wait);io_loop's "A busy loop still checks due timers, and fds periodically" (the block below it);io_list's "io_unlist answers null if a value is past 255" (io_unlist, a few lines down).Checks
bun gates/repo.ts: 54 / 54.tsc -p bend2/pack/tsconfig.json(TypeScript 5.9.3): the same 11 errors as main, none incomp.ts.gates/test.ts, run locally: 1705 / 1707. The two failures, gfx_clicks and gfx_window, expect "no display". This Mac has a desktop session, so they fail the same way on main here..bendfile undertests/,bench/anddemos/built to C and JS with main and with this branch (866 build to C, 910 to JS; the same builds fail on both):cls_fit(n)as its class and undoing every hoisted chain (putting the sharedheap_allocback in each arm), 865 of the 866 programs are main's C outside comment lines. The last,fork_leaf_result_loop, differs only in how its_nd_locals are numbered.cc -std=c11 -O3to the same machine code as on main (otool -tXtext identical); onlyBEND_SRC's embedded text differs. That includes symreg, whose chain has an arm that computes a predecessor before its alloc, and editdist, kmeans and tree-matmul.bend main.bend -o main.c, the wholebunrun, 7 interleaved rounds, medians): merkle takes 0.183 s on main and 0.188 s here, and tree-bitonic takes 0.112 s on both. The ranges overlap (merkle 0.174–0.196 s on main, 0.178–0.198 s here).Benches (the hoist alone)
These were measured on an Apple M3 Max, not the minis' M4, with main at b8664b0 against this branch's head. Builds use the gate's
cc -std=c11 -O3 main.c -lpthreadand run with+RTS --threads N --gpu off. Each binary runs once to warm up. Then main and this branch alternate, for 5 rounds on SEQ (--threads 1) and 9 rounds on PAR (--threads 16). Ratios are PR / main, so lower is faster. Every run printed the pinned output.Notes:
-falign-functions=64, which takes function layout out of the comparison. The aligned medians were 0.79 (SEQ) and 0.80 (PAR) for tree-bitonic, and 0.99 and 0.97 for tree-radix. So the gain is not a layout effect.