jit(wasm): the last two bridge declines — pointer-chase induction, and the trampoline scratch inside the JitFrame - #1058
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
WalkthroughThe wasm backend now uses shared static scratch storage and a fixed compact trampoline ABI. CALL_ASSEMBLER metadata and admission checks are simplified. Heap loads count as loop-state advancement. Interpreter documentation describes guard-based mapdict protection. ChangesWasm trampoline and CALL_ASSEMBLER changes
Interpreter attribute documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GeneratedWasm
participant jit_call_area_addr
participant jit_call_compact
participant JIT_CALL_AREA
GeneratedWasm->>jit_call_area_addr: obtain static scratch address
GeneratedWasm->>JIT_CALL_AREA: write trampoline arguments
GeneratedWasm->>jit_call_compact: invoke compact trampoline
jit_call_compact->>JIT_CALL_AREA: write result
GeneratedWasm->>JIT_CALL_AREA: read result
Possibly related issues
Possibly related PRs
Poem
✨ 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit d58a27a). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![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: 593f4d1b14
ℹ️ 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".
| | IntAddOvf | ||
| | IntSubOvf | ||
| | IntMulOvf | ||
| | GetfieldGcR |
There was a problem hiding this comment.
When a loop-closing bridge jumps a value produced by an immutable or otherwise unchanged field/array load and the bridge has no store/call, this new load classification makes compile_bridge treat the bridge as advancing even though the loaded value can be byte-identical on every pass. That bypasses the existing !advances && !mutates_heap decline for guard side-traces, so the loop↔bridge chain can be installed and spin forever at constant heap/loop state; loads should only count when the loaded address/content is proven to change, not merely because a load opcode produced the jump arg.
Useful? React with 👍 / 👎.
b1f974d to
a5d8881
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@majit/majit-backend-wasm/src/codegen.rs`:
- Around line 3608-3622: In the compact trampoline path around
emit_call_area_addr and emit_jit_call, reload local 0 after the
wasm_jit_ca_alloc_frame call when WASM_DIRECT_RESIDUAL_CALL is disabled. Add the
same caller-frame reload fallback used by ca_reload_caller_fn_ptr before
emit_reload_ca_input_refs_from_homes, so Ref inputs use the moved JitFrame’s
current caller reference.
In `@majit/majit-backend-wasm/src/lib.rs`:
- Around line 153-173: Update the advances_loop_state predicate to include
GetarrayitemGcPureI, GetarrayitemGcPureR, and GetarrayitemGcPureF alongside the
existing GC array-load opcodes, so build_function’s pure GC array loads are
recognized as loop-state advancement.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a3040c41-adf9-42e0-98e4-3a3df8feed75
📒 Files selected for processing (12)
majit/majit-backend-wasm/src/codegen.rsmajit/majit-backend-wasm/src/failguard.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-backend-wasm/tests/codegen_test.rspyre/bench/synth/ca_bridge_multiframe_resume_double_call.wasm.jitstatspyre/bench/synth/exception_catching_frame_tb_node.wasm.jitstatspyre/bench/synth/exception_inline_callee_tb_frames.wasm.jitstatspyre/bench/synth/exception_reentry_guard_finally_residual.wasm.jitstatspyre/bench/synth/exception_traceback_lineno_chain.wasm.jitstatspyre/bench/synth/foriter_call_resume_drops_iteration.wasm.jitstatspyre/bench/synth/gc_bug_bridge_flavor_traceback_names.wasm.jitstatspyre/bench/synth/recursion_memo_branch.wasm.jitstats
…osing bridge check `compile_bridge`'s livelock shield refuses a loop-closing bridge whose terminal JUMP carries no advancing value. Its advance predicate accepted only integer and float arithmetic, so a pointer chase did not qualify: `tb = tb.tb_next` lowers to GetfieldGcR, which is neither arithmetic nor a heap write, and the bridge that re-reads the link was declined even though each pass reads a different node and walks the loop toward its exit. Seven fixtures carry that traceback walk. Six of them lose their declines, and four land on dynasm exactly (guard_failures wasm -> dynasm): exception_inline_callee_tb_frames 975 -> 403 dynasm 403 exception_traceback_lineno_chain 802 -> 402 dynasm 402 exception_catching_frame_tb_node 601 -> 401 dynasm 401 exception_reentry_guard_finally_residual 2459 -> 2261 dynasm 2261 gc_bug_bridge_flavor_traceback_names 2036 -> 1837 dynasm 1655 exception_traceback_frame_lineno 814 -> 813 dynasm 811 `bridges_compiled` drops alongside on five of them, also onto dynasm's value: the extra bridges were the retraces that a declined guard forced. The shield itself stays. It refuses a bridge that resumes at the loop header with byte-identical state, whose guard then re-fails and spins the loop against the bridge; the resume-at-LABEL dispatch does not address that, and the two were introduced together. Only the predicate widens, so a JUMP built entirely of verbatim input reloads, fresh allocations and baked constants is still refused. check.py wasm 374/374. Assisted-by: Claude
…ame, and drop the CALL_ASSEMBLER gate it required The trampoline's ABI was frame-relative on both sides of the host hop: the guest stored func_ptr/nargs/args off wasm local 0 and read the result back off local 0 before reloading it, and the host computed `call_area = frame_ptr + call_area_ofs` before re-entering the guest and wrote the result there afterwards. That is sound only for the host-entry frame, which is allocated old-gen and therefore non-moving. CALL_ASSEMBLER breaks it twice. `wasm_jit_ca_alloc_frame` allocates the callee frame in the nursery, so it can move across the hop; and it sizes it `ca_frame_bytes`, which excludes the trailing call area, so a trampoline call on such a frame would store past the object's end. The backend therefore refused to compile any CALL_ASSEMBLER-bearing bridge that shared a trace or token with a trampoline residual call. The scratch is now a module-static array whose address is baked into emitted code, the way the pending-exception cells already are. Frame geometry is untouched: the call area stays reserved in the frame, unused, so frame_bytes, ca_frame_bytes and the module cache do not move. A single shared area is sound because trampoline use is strictly LIFO — the host materialises every argument before invoking the callee and the guest loads its result immediately on return. The trampoline reads its scratch at `base + offset`, and the base-only import has the offset baked host-side, so every emitting module now takes the two-argument import. With nothing left to protect, the gate and its census go: the `has_trampoline_calls` checks on the pending self target, the registered target, the live loop and the redirect, the chained-bridge census, and `ca_reentry_safe`. The BRIDGE_DIAG slots stay; slot 15 stops firing. This was the last terminal bridge decline on wasm. On the only three fixtures that carried it, `decl_shortcircuit` 543/543/89 -> 0/0/0, `decl_callasm` and `decl_ca_trampoline` -> 0, and `entered == BRIDGE_OK` exactly (16, 16, 26): ca_bridge_multiframe_resume_double_call gf 3062 -> 2581 bridges 14 -> 16 recursion_memo_branch gf 3083 -> 2602 bridges 14 -> 16 foriter_call_resume_drops_iteration gf 5182 -> 5165 bridges 23 -> 26 dynasm reports 2592/16, 2613/16 and 5150/27. No other fixture moved. check.py wasm 374/374; cargo test -p majit-backend-wasm green. Assisted-by: Claude
Both files were recorded against a base that predates #1050, #1056 and #1057. Re-measured after re-extracting LLBC, whose three crate fingerprints had all drifted: exception_inline_callee_tb_frames bridges_compiled 2 -> 3, guard_failures 403 -> 604 gc_bug_bridge_flavor_traceback_names guard_failures 1837 -> 1838 Both also gain the `fbw_rolled_back_with_effects` field the new base adds. The rest of the wasm suite is unchanged at 376 passing. Assisted-by: Claude
`allow_unboxing`, `ever_mutated`, `attr` and `typ` are declared quasi-immutable upstream, and the comments asserted that flatly. pyre installs no quasi-immutable watcher for any of them — they are plain Cells, and mapdict.rs contains no quasi-immutable code — so the comments read as a property the code does not have. State the substitute instead: every read is paired with a GuardValue on the instance map, emitted by walker_guard_mapdict_instance_shape. The full rationale sits on `allow_unboxing`, the only one of the four that gates a fold decision; the other three refer to it. Same formulation already used for the `Function` `?` fields in descr.rs. Comments only. Assisted-by: Claude
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Two wasm-only bridge declines, found by a full 383-fixture MC_DIAG census after #1038 closed the exception-edge class. Together they were everything the census had left.
1. A heap load is loop-state advancement
compile_bridgecarries a livelock shield: a loop-closing bridge whose terminal JUMP carries no advancing value would re-present byte-identical state, its guard would re-fail, and the loop would spin against the bridge. The advance predicate accepted only integer and float arithmetic.A pointer chase does not qualify under that rule.
tb = tb.tb_nextlowers toGetfieldGcR, which is neither arithmetic nor a heap write, so the bridge that re-reads the link was refused — even though each pass reads a different node and walks the loop toward its exit. Seven fixtures carry that traceback walk, and it is the only linked-list walk in the suite, which is why the whole class looked exception-shaped.There is no upstream counterpart to this shield:
send_bridge_to_backendhands the bridge todo_compile_bridgewith no terminal-JUMP shape analysis, and x86'sclosing_jumpisJMP _ll_loop_code. dynasm and cranelift have no such predicate.The predicate widens to accept heap loads. The shield itself stays — the hazard it names is real and the resume-at-LABEL dispatch does not address it, so a JUMP built entirely of verbatim input reloads, fresh allocations and baked constants is still refused.
1570 permanent blackhole deopts removed; four land on dynasm exactly.
bridges_compileddrops alongside on five of them, also onto dynasm's value — the extra bridges were retraces a declined guard had forced.2. The residual-call trampoline scratch leaves the JitFrame
The trampoline's ABI was frame-relative on both sides of the host hop: the guest stored func_ptr/nargs/args off wasm local 0 and read the result back off local 0 before reloading it, and the host computed
call_area = frame_ptr + call_area_ofsbefore re-entering the guest and wrote the result there afterwards. That is sound only for the host-entry frame, which is allocated old-gen and therefore non-moving — an invariant the backend states in so many words.CALL_ASSEMBLER breaks it twice.
wasm_jit_ca_alloc_frameallocates the callee frame in the nursery, so it can move across the hop; and it sizes itca_frame_bytes, which deliberately excludes the trailing call area, so a trampoline call on such a frame would store past the object's end. Rather than fix that, the backend refused to compile any CALL_ASSEMBLER-bearing bridge sharing a trace or token with a trampoline residual call.The scratch is now a module-static array whose address is baked into emitted code, the way the pending-exception cells already are. Frame geometry is untouched — the call area stays reserved in the frame, unused — so
frame_bytes,ca_frame_bytes, the module cache and the geometry pinning test all stay put. A single shared area is sound because trampoline use is strictly LIFO: the host materialises every argument before invoking the callee, and the guest loads its result immediately on return. That assumption is written down where the static is defined.The trampoline reads its scratch at
base + offset, and the base-only import has the offset baked host-side, so every emitting module now takes the two-argument import.With nothing left to protect, the gate and its census go: the
has_trampoline_callschecks on the pending self target, the registered target, the live loop and the redirect, the chained-bridge census, andca_reentry_safe. The BRIDGE_DIAG slots stay; slot 15 stops firing.On all three,
decl_shortcircuit543/543/89 → 0/0/0,decl_callasmanddecl_ca_trampoline→ 0, andentered == BRIDGE_OKexactly (16, 16, 26): every bridge that enters is accepted. No other fixture in the suite moved.Where the corpus stands
Suite-wide wasm
decl_shortcircuitwas 82324 before #1038, 1175 after it, and is 0 now. Every structural decline slot in the 383-fixture census is zero on all three backends, exceptbridge_no_targets_close(12 events, a bridge close that finds no compiled target and falls through tocompile_loop— benign, and identical on dynasm).Verification
check.py wasm 374/374, dynasm 378/378, cranelift 378/378, and
cargo test -p majit-backend-wasm(integration tests included) — all run on the immediately preceding base,a8677f4a0a0.This branch was then rebased onto
6b843fa4b76with no conflicts and no code change; re-running the suites at that base is pending because the build box is currently at load average 89 with several sibling worktrees compiling, where check.py's ratio gates produce false failures. CI runs on a clean machine and is the authority here. Nine.wasm.jitstatsbaselines are re-recorded.— authored by Claude
Summary by CodeRabbit
Performance
Reliability
Tests
Documentation