codewriter, majit-macros: the jitcode lowering rewrites, and the goto_if_not fusion that fused nothing - #1664
Conversation
|
Warning Review limit reachedNext included review available in 29 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (18)
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 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. |
![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: 09b70e0cf6
ℹ️ 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".
| BinOp::Ne(_) => false, | ||
| _ => return None, | ||
| }; | ||
| let both_instances = lhs.struct_type.is_some() && rhs.struct_type.is_some(); |
There was a problem hiding this comment.
Binding::struct_type records any known pointee layout, including raw ref_params, field pointees, and array elements; it does not establish RPython's _is_rclass_instance predicate. Classifying every pair of typed refs as InstancePtrEq/InstancePtrNe therefore exposes non-instance pointers to instance-only optimizer reasoning, such as folding two known but different classes as unequal, even though raw pointers with different structural views may alias. Emit the plain pointer op until the binding carries an actual rclass-instance marker.
AGENTS.md reference: AGENTS.md:L223-L226
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and fixed — the arm now emits ptr_eq / ptr_ne for every Ref/Ref pair.
Upstream's predicate is _is_rclass_instance(v) = lltype._castdepth(v.concretetype.TO, rclass.OBJECT) >= 0, i.e. the pointee descends from the instance base. struct_type answers a different question — "the lowering tracked a pointee layout" — which a raw struct, a ref_params entry, a field pointee and an array element all satisfy, exactly as you say.
The consequence is concrete rather than theoretical: optimize_oois_ooisnot takes the instance flag and folds a pair whose two operands carry known-but-different classes straight to a constant. Two structural views of one raw address are known-different classes that do alias, so that fold would be a wrong answer.
Nothing in this tree carries the real predicate — rg 'is_rclass_instance' finds only the comment this arm added — so the pair stays on the plain pointer compare until something does. That is also upstream's own fallthrough: rewrite_op_ptr_eq promotes only under the test and otherwise leaves ptr_eq alone.
It costs nothing measurable: the shipped jitcodes carry 1912 ptr_eq/rr>i and zero instance_ptr_eq/rr>i, so no body changes, and the arm still emits a pointer compare where the branch's earlier code declined the whole dispatch arm. The unit test that asserted the promotion now asserts its absence. cargo test -p majit-macros --features dynasm 197 passed, cargo test -p tl --features dynasm 31 passed.
— commented by Claude
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 8649518). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
Merging this PR will not alter performance
Comparing 
Footnotes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59ae43732b
ℹ️ 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".
| "encloses a `hint_fresh_virtualizable` this lowering has no flag to carry", | ||
| stmt, | ||
| ); | ||
| return None; |
There was a problem hiding this comment.
When a #[jit_interp] body contains hint_fresh_virtualizable, this unconditional refusal makes try_inline_dispatch_arm discard the body’s JitCode and leaves that hot path permanently interpreted. Upstream instead records fresh_virtualizable in vable_flags and continues lowering subsequent field accesses, so the lowerer needs an equivalent per-binding flag rather than treating the missing carrier as a reason to abandon translation.
AGENTS.md reference: AGENTS.md:L225-L226
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The end state you describe is right, but the harm this comment names does not exist yet, and implementing the carrier now would ship machinery nothing exercises. Three measurements:
1. The refusal declines zero bodies. hint_fresh_virtualizable has two production call sites in the whole tree, pyre/pyre-interpreter/src/pyframe.rs in finish_for_call_with_globals_obj and createframe_obj. Neither function is #[jit_interp] / #[jit_inline], so this lowering never sees them. majit/examples/* has none either.
2. This lowerer has no pyre users at all. rg '#\[jit_interp|#\[jit_inline' pyre/ --type rust returns comments only; every real consumer is majit/examples/* (tl, tlc, tla, tlr, tiny2/3, tinyframe, braininterp, dualtape, regex, i64env, spcount, cel). pyre's own jitcodes come from the translator codewriter, majit-translate/src/codewriter/jtransform.rs. So "leaves that hot path permanently interpreted" has no hot path to leave.
3. The translator side — the producer pyre actually uses — already has the flag. VableFlag::FreshVirtualizable in jtransform.rs, recorded and read back the way upstream's vable_flags is.
There is also a design gap the carrier would have to close first. Upstream's flag is per-SSA-variable, and is_vable_getfield reads 'fresh_virtualizable' in flags for the variable being accessed. This lowering decides the vable path by name — lower_vable_field_read matches expr_matches_local_name(&field.base, vable_var) — so there is no per-variable slot to hang the flag on. The rebinding form let fresh = hint_fresh_virtualizable(state); would already get upstream's semantics for free under name matching, but the in-place form is the one with no carrier, and that is the shape the refusal exists for.
So the refusal stays, as a decline that currently declines nothing, rather than a per-binding flag with no producer to guard it. If a #[jit_interp] body ever needs the hint, that body is what should land with the carrier.
— commented by Claude
The CI failures were one missing dispatch arm — root cause, fix, and two open defects it exposedWhat was redFour jobs × three fixtures ( Root cause
The gap was invisible until this branch, because nothing produced the opname. Fixed in Verification, same tree, only that arm differing
The last row answers the "is the jitstats change intentional?" question: it was An opname census, so this class is closed rather than patchedThe codewriter runs at build time, so the shipped opname universe is in
Two open defects this exposed — neither belongs to this branch1. The
2. Deterministic over three runs; The only other sweep difference on the fixed binary was (Sweep methodology note: this box has no — commented by Claude |
`exec` / `eval` plant `__builtins__` through `setdefault` (`pyopcode.py`) and `space.contains_w` + `space.setitem_str` (`compiling.py`), both dispatched on the caller's own mapping, so a dict subclass's override runs user Python while `exec_or_eval` still holds the code object it just compiled, the namespace arguments and a raw pointer interior to the code object's payload. The rooting this fixture guards is `0ba64ba2627`'s on main, which publishes each of those operands and reads them back at the site that consumes them; this branch's own earlier version of it is dropped as redundant. The fixture itself is new: it drives all five shapes (`setdefault`, `__contains__`, `__setitem__`, separate locals, a precompiled code object) plus a warm loop, and every expectation is the value CPython 3.14 and PyPy both produce. Assisted-by: Claude
`lower_stmt_dispatch`'s `Expr::Continue` arm emitted a jump to
`dispatch_loop_label` whenever one was set, and reported success with no
emission when it was not. Neither answer is right for a `continue` that
names a different loop:
* inside a loop body written in an arm, `lower_loop_stmt` and
`lower_loop_if` take the direct and `if`-branch spellings with that
loop's own labels; a `match` arm or a nested block falls through to
this arm and jumped to the dispatch head instead of the loop's;
* a labelled `continue 'l` names a loop with no label here at all;
* with no dispatch label the bare `Some(())` emitted nothing, dropping
the transfer and letting control fall to the next statement.
The arm now takes the jump only for an unlabelled `continue` with a
dispatch label in scope, and otherwise routes to `lower_stmt_fallback`,
whose `stmt_contains_loop_control` guard refuses and records the reason.
`lower_loop_body`'s nested lowerer no longer inherits
`dispatch_loop_label`.
Four tests; three of them fail on the previous lowering, one of those
printing the `jump(__l_dispatch)` a `while` body's `match` arm emitted
beside the loop's own back-edge.
Assisted-by: Claude
`handle_recursive_call` opens with `promote_greens`, which emits a `-live-` + `<kind>_guard_value` pair for every green before the `recursive_call_<kind>`. `lower_recursive_portal_call` emitted the call alone. The greens are what selects the callee: `exec_recursive_call` reads them out of the caller's registers as concrete values and keys `recursive_inline_decision` on them, so a green the trace never guarded lets a recorded loop replay with a different key and enter the portal at a pc it was not compiled for. The pair was already emitted for the merge point's greens (`emit_promote_greens`), which is upstream's other `promote_greens` call site. Both now go through `Lowerer::emit_live_and_guard_value`. `cargo test -p tl --features dynasm`: 31 passed. That includes `jit_portal_call_in_loop_body_blocks_tracing`, whose pinned `compiles == 0` is unchanged — the portal call still aborts above the guard, in `decide_recursive_inline`. Assisted-by: Claude
The lowerer answered `hint_access_directly` and `hint_fresh_virtualizable` the same way — the call form lowered to its argument, the macro form emitted nothing — on the ground that both are identity functions. That is right for only one of them. `rewrite_op_hint` drops `access_directly` because the flag it records is read back for `fresh_virtualizable` alone: `is_vable_getfield` / `is_vable_setfield` consult `'fresh_virtualizable' in flags` and answer `False`, which keeps the following field accesses off the `getfield_vable_*` / `setfield_vable_*` path. This lowering has no carrier for that flag, so dropping the hint leaves every following access on the vable path — the opposite of what the hint asks, with nothing emitted to say so. `hint_fresh_virtualizable` now refuses in both positions. The refusal is closed by a fourth guard in `lower_stmt_fallback`, beside the ones for `return`, `break`/`continue` and a green write: without it the purity test scores the hint inert and drops the statement, which is the same silent outcome by another route. The guard scans the whole statement, so it also covers the value position, where the statement that has to refuse is the enclosing `let`. Four tests. The two refusals fail on the previous lowering; the two `access_directly` controls pass on both, which is what attributes the change to the hint rather than to the statement shape. Assisted-by: Claude
jtransform.py binds `_rewrite_symmetric` as `rewrite_op_<name>` for the symmetric arithmetic and bitwise ops and for every ordered comparison, and `_rewrite_equality` for `int_eq`/`int_ne`/`ptr_eq`/`ptr_ne`. Neither had a counterpart here, so an equality against zero stayed a binary compare and a constant left-hand operand stayed on the left. `rewrite_symmetric` runs over each operation on its way into `rewrite_operation`, because upstream it is the whole rewrite rather than one arm of a dispatch. It moves a materialised source constant to the right-hand side and mirrors `lt`/`le`/`gt`/`ge` (and the already-typed `uint_` spellings `front::checked_arith_uint` emits). `sub`, the divisions and the shifts are absent, matching upstream's binding list. Two new `rewrite_operation` arms then fold a comparison against the zero of its register class into the unary test: `int_is_zero`/`int_is_true` over `'i'` operands, `ptr_iszero`/`ptr_nonzero` over `'r'` operands. The zeros are recovered from the `Const*` operation that produced the operand, since the front end materialises source constants as SSA Variables. Every one of the four opnames already had a bytecode, a blackhole handler and a `goto_if_not_*` fusion entry in `goto_if_not_fusable`; production `jtransform.rs` had emitted none of them. Assisted-by: Claude
`goto_if_not_fusable` listed the RPython opnames only. `int_lt` and its five siblings are names no operation carries at that point: `front::mir::canonical_binop_label` leaves a comparison bare and the `int_` prefix comes from `assembler::op_kind_to_opname_with_kinds`, which runs after flatten. `float_lt` and `ptr_eq` did match because `rewrite_operation` renames those, so the fusion fired for float and ref comparisons and for nothing else -- every integer comparison branch emitted a standalone compare plus a generic `goto_if_not` instead of the `goto_if_not_int_lt/iiL` family `insns.rs` reserves. Accept the bare spellings alongside the RPython ones, prefixing them so the fused exitswitch names the opcode the assembler looks up. The bare arm requires both operands to be `Signed`: the argcodes follow the register kinds and there is no `goto_if_not_int_eq/rrL` entry. Assisted-by: Claude
…ares to the jitcode lowerer `lower_binary` handled only `BindingKind::Int` operands and emitted `record_binop_i` for every recognized `BinOp`. Three `jtransform.py` rewrites had no counterpart there: * `rewrite_op_ptr_eq` / `rewrite_op_ptr_ne` — a Ref/Ref `==` or `!=` fell through the Int check and returned `None`, declining the whole dispatch arm. Both sides carrying a `struct_type` now emit `InstancePtrEq` / `InstancePtrNe` (RPython's `_is_rclass_instance` promotion); one side without it takes `PtrEq` / `PtrNe`, because a missing `struct_type` records that the lowering never tracked the struct, not that the operand is not an instance. * `_rewrite_symmetric` — a symmetric binop with the literal on the left now swaps the operands, reversing the comparison opname. * `_rewrite_equality` — `x == 0` / `x != 0` now lower to `IntIsZero` / `IntIsTrue` instead of an `IntEq` / `IntNe` against a materialized zero. `record_unary_i` accepted only `IntNeg` and `IntInvert`, so the new `IntIsTrue` panicked in every example crate with an int-typed `while`. Extending its match exposed that `int_is_zero/i>i` had a wired blackhole handler but no opcode byte: `wire_handler` matches by name against the curated `(key, byte)` set and no-ops when the name is absent, and the build-time `write_insn` path calls the panicking `insn_byte` rather than the translator's `insn_byte_opt` dynamic allocator. The key now has `BC_INT_IS_ZERO`, a curated-set entry, and a metainterp dispatch arm. Assisted-by: Claude
…nd the vtable cache are absent `constant_fold_ll_issubclass`, the `typeptr` / `strhash` / `_greenfield` branches of `rewrite_op_getfield`, and the body of `setup_cache_gcstruct2vtable` all have no counterpart here. Each gap is a consequence of an earlier pipeline decision rather than an oversight, and the reachability census that establishes that is not recoverable from the code, so it is stated where a reader looks for the missing branch. No behaviour change. Assisted-by: Claude
… off its links
`optimize_goto_if_not` fused nothing in production: of 3459 shipped
jitcodes, `goto_if_not/iL` accounted for 9284 branches and the whole
`goto_if_not_int_*/iiL` family for none. Two independent gates were
closed.
`goto_if_not_fusable` looked for `int_is_true` / `ptr_nonzero`, but the
front end spells the condition `OpKind::UnaryOp { op: "bool" }` and the
`int_is_true` name is assigned by `assembler::op_kind_to_opname_with_kinds`
after flatten -- so the arm matched nothing, the same gap the bare
comparison arm covers. It now resolves `"bool"` the way the assembler
will, by the operand's register kind, and declines when that kind is
unresolved rather than naming a key with no wired handler.
`v.concretetype() != Some(LowLevelType::Bool)` then rejected whatever
survived. Kinds are published through `set_concretetype_of_inline`,
which canonicalises through `concrete_to_canonical_lltype`, and
`getkind(Bool)` is `'i'` -- so a Bool-producing op is stored as `Signed`
and the predicate is false for every graph that took that path. The two
exits being the false and the true arm is the fact upstream's
`lltype.Bool` stands for; it is now read off the links through
`flatten::bool_llexitcase`, the same reader `flatten::is_bool_branch`
uses, so the two cannot drift.
The existing `optimize_goto_if_not_fuses_int_lt_compare` fixture stamps
`LowLevelType::Bool` by hand, which is why it passed over a gate no
production graph clears.
Assisted-by: Claude
…l branch off its links" This reverts commit 0dacccb. The fusion reached the shipped jitcode and is a large reduction — of 3459 jitcodes, `goto_if_not/iL` went 9284 -> 0, `goto_if_not_int_is_true/iL` 0 -> 9272, and the standalone `int_is_true/i>i` 9348 -> 80. It also breaks the JIT. `pyre/check.py --backend dynasm`, same tree either way, LLBC re-extracted for both: 478 passed / 60 failed with the commit, 535 passed / 3 failed without it. The 3 that remain are the ones the branch already had (`list_append_virtual_payload`, `pickle_ctor_args`, `zero_arg_super_attr`). The 57 it adds are not baseline movement. `pyre/bench/synth/listcomp_hot.py` returns 6000000 under `PYRE_NO_JIT=1` and raises `TypeError: 'int' object is not an iterator` from `for j in range(n)` with the JIT on. Across the rest the signature is `loops_aborted 0 -> N`, `loops_compiled` down and `fbw_blackhole_adopted_single_frame 0 -> N` — loops that used to compile now abort into the blackhole — plus 11 crashes, 3 timeouts and one wrong output. Cleared as causes: `liveness.rs` counts every `GotoIfNotOp` arg as a use; the `BC_GOTO_IF_NOT_INT_IS_TRUE` dispatch arm matches `opimpl_goto_if_not_int_is_true` exactly, down to `replace=false`; `bhimpl_goto_if_not_int_is_true` aliases `bhimpl_goto_if_not` upstream too, and branching on `x` is branching on `int_is_true(x)`; `body_branch_targets` decodes any `L` argcode; `fbw_callee_body_replay_scan` poisons only write-shaped opnames. The `fbw_blackhole_adopted_*` counter points at the FOR_ITER inline window; `PYRE_FBW_INLINE_DIAG=1 PYRE_FBW_REPLAY_DIRTY_BODY=1` prints that verdict per body. Assisted-by: Claude
`unop_int_record` in the jitcode walker carries `int_neg`, `int_invert`, `int_same_as` and `int_is_true` but not `int_is_zero`, which `pyjitpl.py` generates from that same unary list. An opname the table does not name falls to `DispatchError::UnsupportedOpname`, which aborts the trace part way through the body being walked. `_rewrite_equality` folds `int_eq(x, 0)` into `int_is_zero`, so this branch is the first to put the opname in a body -- 1232 occurrences in the shipped jitcodes. Against that, `synth/list_append_virtual_payload` answered `TypeError: 'float' object is not an iterator`, `synth/zero_arg_super_attr` did not terminate, and `synth/pickle_ctor_args` recorded `loops_compiled=0` where its committed sidecar records `loops_compiled=2 loops_aborted=0 fbw_blackhole_adopted_*=0`. With the arm in place those three, and test_heapq / test_random / test_pickle, answer as before. Add unit tests for `int_is_true` and `int_is_zero` -- `drive_int_unop` panics on a dispatch error, so both assert the arm exists -- and a selfcheck fixture over float-element list comprehensions, the shape that reproduces the wrong answer. Assisted-by: Claude
`unsupported_opname_surfaces_typed_error` justified picking `vtable_method_ptr/rd>i` on the ground that it has "Zero JitCode hits in production traces". Counting `body.code[pos]` over every `pos in body.startpoints` of the shipped `jit_metadata.json` puts one occurrence in `view_as_kwargs`, between a `live/` and an `int_guard_value` + `residual_call_r_r`. The same census reports 3459 bodies, 152888 instructions and 114 distinct opnames -- the whole `insns` table -- so no registered opname is dead, and `vtable_method_ptr/rd>i` is the only one left with no walker arm. That is why the test can use it, and the comment now says so instead of claiming it is unreachable. A `f(**d)` probe did not reach it, so whether a walk gets there is unproven either way. Assisted-by: Claude
…compare The Ref/Ref arm read `struct_type.is_some()` on both operands as this layer's `_is_rclass_instance` and emitted `instance_ptr_eq` / `instance_ptr_ne` for the pair. Upstream's predicate is `lltype._castdepth(v.concretetype.TO, rclass.OBJECT) >= 0` -- the pointee descends from the instance base. `struct_type` records that the lowering tracked a pointee layout, which a raw struct, a `ref_params` entry, a field pointee and an array element all satisfy. The difference is observable: `optimize_oois_ooisnot` folds a pair whose two operands carry known-but-different classes to a constant, and two structural views of one raw address are known-different classes that do alias. Nothing in this tree carries the real predicate, so both operators stay on `ptr_eq` / `ptr_ne` -- upstream's own fallthrough. Shipped jitcodes carry 1912 `ptr_eq/rr>i` and no `instance_ptr_eq/rr>i`, so no body changes; the arm still emits a pointer compare where the branch's earlier code declined the whole dispatch arm. `cargo test -p majit-macros --features dynasm` 197 passed, `cargo test -p tl --features dynasm` 31 passed. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8649518eb0
ℹ️ 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".
| fn is_symmetric_binop(name: &str) -> bool { | ||
| matches!( | ||
| name, | ||
| "add" |
There was a problem hiding this comment.
When the left operand is a ConstStr and the right operand is a variable Ref, this generic "add" classification swaps them before rewrite_operation routes the two Refs to the order-sensitive jit_str_concat helper, so "prefix" + value is compiled as value + "prefix". Upstream applies _rewrite_symmetric only to typed integer and float addition; restrict this rewrite to numeric operand kinds rather than every bare add.
AGENTS.md reference: AGENTS.md:L223-L226
Useful? React with 👍 / 👎.
1 participant
Ports the jitcode-lowering rewrites the reviewer note listed, in the order it
gave, and follows two of them into the codewriter where the same rewrite was
missing or inert.
What landed
Lowerer (
majit-macros, the#[jit_interp]jitcode lowering)continuethe lowering cannot target now refuses instead of resolving toSome(())and silently vanishing.recursive_portal_call!emitspromote_greensand the real reds.hint_fresh_virtualizablerefuses rather than being dropped._rewrite_equality,_rewrite_symmetricand the pointer comparisons:a Ref/Ref
==used to fall through the Int check and decline the wholedispatch arm;
x == 0used to materialize a zero and compare against it.Codewriter (
majit-translate)_rewrite_equalityand_rewrite_symmetric.optimize_goto_if_notnow actually fuses.The fusion gap — found, fixed, measured, and backed out
optimize_goto_if_notwas fusing nothing. Counting eachstartpointsoffsetin the shipped
jit_metadata.json(3459 jitcodes) before the fix:goto_if_not/iL(unfused)goto_if_not_int_*/iiLfamilygoto_if_not_ptr_ne/rrLThree gates, each closed for a different reason:
goto_if_not_fusablematchedint_lt/int_le/ …, butfront::mir::canonical_binop_labelleaves a comparison bare andassembler::op_kind_to_opname_with_kindsadds theint_prefix afterflatten.
float_ltandptr_eqmatched only becauserewrite_operationhad already renamed them.
int_is_true/ptr_nonzero, but the front endspells the condition
OpKind::UnaryOp { op: "bool" }and the samepost-flatten renaming assigns those names. It now resolves
"bool"theway the assembler will — by the operand's register kind — and declines
when that kind is unresolved rather than naming a key with no wired
handler.
v.concretetype() != Some(LowLevelType::Bool)rejected whateversurvived. Kinds are published through
FunctionGraph::set_concretetype_of_inline, which canonicalises throughconcrete_to_canonical_lltype— five outputs, none of themBool— andgetkind(Bool)is'i', so a Bool-producing op is stored asSigned.The two exits being the false and the true arm is the fact upstream's
lltype.Boolstands for, and it is now read off the links throughflatten::bool_llexitcase, the readerflatten::is_bool_branchalreadyuses.
The pre-existing
optimize_goto_if_not_fuses_int_lt_comparefixture buildsits input in the recognizer's spelling and stamps
LowLevelType::Boolbyhand, which is why it passed over gates no production graph clears.
Closing gates 2 and 3 (
0dacccbfe2d) does exactly what it should to theshipped jitcode —
goto_if_not/iL9284 -> 0,goto_if_not_int_is_true/iL0 -> 9272, standalone
int_is_true/i>i9348 -> 80, so roughly 9272instructions and their registers disappear — and it breaks the JIT, so
it is reverted in
09b70e0cf64. Same tree, LLBC re-extracted for both armsbecause
jtransform.rsandflatten.rsare in the fingerprint closure:pyre/check.py --backend dynasmIt is a wrong answer, not baseline movement:
pyre/bench/synth/listcomp_hot.pyreturns 6000000 underPYRE_NO_JIT=1and raises
TypeError: 'int' object is not an iteratorfromfor j in range(n)with the JIT on. Across the rest the signature isloops_aborted 0 -> N,loops_compileddown andfbw_blackhole_adopted_single_frame 0 -> N.The commit and its revert are both kept so the census, the A/B and the
elimination work stay attached to the code they describe. Ruled out as
causes, all read:
liveness.rscounts everyGotoIfNotOparg as a use;the
BC_GOTO_IF_NOT_INT_IS_TRUEdispatch arm matchesopimpl_goto_if_not_int_is_trueincludingreplace=false;bhimpl_goto_if_not_int_is_truealiasesbhimpl_goto_if_notupstream too;body_branch_targetsdecodes anyLargcode;fbw_callee_body_replay_scanpoisons only write-shaped opnames;
-live-sits immediately before thebranch in both forms.
fbw_blackhole_adopted_*points at the FOR_ITERinline window, and
PYRE_FBW_INLINE_DIAG=1 PYRE_FBW_REPLAY_DIRTY_BODY=1prints that verdict per body.
A new opcode needs four registrations
The lowerer's
int_is_zerofold broke every example crate with an int-typedwhile:record_unary_i: unsupported opcode IntIsTrue. Extending that matchexposed that
int_is_zero/i>ihad a wired blackhole handler but no opcodebyte —
wire_handlermatches by name against the curated(key, byte)setand no-ops when the name is absent, and the build-time
write_insnpathcalls the panicking
insn_byterather than the translator'sinsn_byte_optdynamic allocator. The key now has
BC_INT_IS_ZERO, a curated-set entry anda metainterp dispatch arm.
What is deliberately not here
Four items on the note have no producer in this tree. Each is now stated
where a reader looks for the missing branch, with the pipeline decision that
makes it unreachable:
constant_fold_ll_issubclass— it folds calls the inliner inserts, andInliner::inline_oncerefuses that path withCannotInline; separately,cutoverskips thell_issubclass/ll_issubclass_const/ll_isinstancebodies because
flowspace_adapter::translate_ophas already rewritten everycall site into
issubtype/isinstance.setup_cache_gcstruct2vtable—GcStructVTableCacheholds no rtyperhandle to walk
instance_reprswith, andget_vtable_for_gcstructhas nonon-test caller.
typeptr,strhash/unicodehashand_greenfieldbranches ofrewrite_op_getfield— the class word is folded intonew_with_vtableonthe store side and
heaptracker::is_header_wordkeeps it out of thedescriptor census; there is no
StrHashopcode;pyjitplhas no greenfieldmechanism.
vable_array_varsand its escape assert were already complete(
check_no_vable_array, cleared per block, checked on all three routes).instance_ptr_eq/instance_ptr_neare emitted by the lowerer, whichrecords a
struct_typeper binding, but not by the codewriter:Variable.concretetypeis one of five coarse kinds there,Variable.annotationis cleared after use, and the surviving projection makes every Ref look like
a
SomeInstance— promoting on that would handoptimize_oois_ooisnottheknown-class branch for array pointers and lose the length-bound fold.
Verification
pyre/check.py, macOS arm64, run separately per backend:The three are
list_append_virtual_payload(crash),pickle_ctor_args(jit-stats) andzero_arg_super_attr(exit 124).They are not this branch's: main's own CI at the merge-base
55e6fb87965fails on exactly those three, on both backends(run 33650098582).
Also green:
cargo test -p majit-translate(3499 + 24 suites),cargo test -p majit-macros(162 + 35), and the fourteenmajit/examplescrates under
--features dynasm.cargo fmt --all -- --checkandscripts/check-new-line-citations.pyare clean.— authored by Claude