jit: a dead-var link-arg trim scoped on the wrong reachability, a shared-receiver locals_w_mut!, bridge opt-fuel, and 91 stale CPython-suite baseline entries - #1138
Conversation
`locals_cells_stack_w` is a raw pointer, so it reads through a shared `&PyFrame` and the macro let a caller holding one mint `&mut` to the array. The accessor this macro replaced took `&mut self`; `&mut *` on the receiver restores that requirement. The result's lifetime stays unconstrained — `&mut *ptr` has no lifetime to bind — so overlapping calls still alias invisibly to the borrow checker. Binding it needs a function signature, which is the accessor form whose separate graph defeats the getfield/setfield pairing. Assisted-by: Claude
`prune_dead_phis` Step 7 drops a dead inputarg from every reachable block, but Step 6 trimmed the matching `Link.args[i]` only for links whose *source* block was reachable. A block the reachability walk excludes — an orphan `eliminate_empty_blocks` bypassed, or a merge block jtransform leaves whose inputargs are phi targets rather than parameters — can still name a reachable block as its link target, and its link kept the full arg list. `remove_duplicate_inputargs` reads each phi column by index across every incoming link, so the untrimmed link contributes the value one slot over. The union-find then merges two variables that hold different values, and the resulting rename is applied to every block. Reachability is closed under exits, so keying the trim on the target adds links and never drops one. `simplify.py:512-516` has no equivalent gap: `graph.iterblocks()` is upstream's block list, so an unreachable block is not in `blocks` and has no link to skip. Assisted-by: Claude
`MAJIT_NO_BRIDGE` is all-or-nothing, so a wrong value that only appears once bridges are compiled can be attributed to the bridge path but not to a bridge. `MAJIT_MAX_BRIDGES=N` compiles the first N and declines the rest, which turns that attribution into a bisection over N: the boundary names the compilation that first produces the wrong value. The counter is consumed last in the `should_bridge` chain, so it only advances on a guard failure the other conditions already admitted and the index counts bridges actually taken. `MAJIT_BRIDGE_FUEL_LOG=1` prints each one, which is what pairs an index with the `@@@GUARD` line `MAJIT_GUARDLOG` emits beside it. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
![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: 729bcb51c9
ℹ️ 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".
| /// `MAJIT_MAX_BRIDGES=N` (diagnostic): allow the first N bridge compilations | ||
| /// and behave as `MAJIT_NO_BRIDGE` from then on. Bisecting N names the bridge | ||
| /// whose compilation first produces a wrong value, at seconds per run rather | ||
| /// than a rebuild per arm. Consumes fuel only when the rest of `should_bridge` | ||
| /// already held, so the count is bridges actually taken — place it last in the | ||
| /// `&&` chain. `MAJIT_BRIDGE_FUEL_LOG` reports each one taken. | ||
| fn bridge_fuel_take() -> bool { |
There was a problem hiding this comment.
When a guard failure arrives through CALL_ASSEMBLER, MAJIT_MAX_BRIDGES does not limit it: jit_ca_handle_guard_failure and try_compile_ca_bridge perform their own must_compile_with_values checks and call trace_and_compile_from_bridge directly (pyre/pyre-jit/src/call_jit.rs:3959-4017 and 4074-4109), while that function checks only no_bridge_enabled at lines 3240-3244. Consequently even MAJIT_MAX_BRIDGES=0 can compile these bridges, and larger limits no longer identify the global Nth bridge as documented. Make the fuel gate available to these frontend-owned bridge decisions, just as MAJIT_NO_BRIDGE is.
AGENTS.md reference: AGENTS.md:L252-L253
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 2ee9f3a). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
… broken The suite gate runs only the modules `baseline.json` records as PASS; every other entry is deselected and carries no signal. Entries therefore go stale in one direction only — a module that starts passing stays outside the gate until someone runs the non-PASS set and says so. Running the full 434 on dynasm finds 94 such modules. 91 reproduce as PASS in a second run at lower concurrency and are recorded here: 63 were IMPORTERROR, 16 CRASH, 9 FAIL, 3 TIMEOUT. The CRASH set alone includes test_dict, test_tuple, test_range, test_slice, test_decimal, test_baseexception, test_userdict, test_userlist, test_weakset, test_queue, test_configparser and test_format. The remaining 3 (test_embed, test_frozen, test_tools) pass under `--full` but stay put: they are `KNOWN_SKIPS` entries, and that list is a policy about what the suite should never gate on, not a record of what currently works. `--full` also reports test_c_locale_coercion as PASS -> FAIL. That module is a `KNOWN_SKIPS` entry whose stated reason is the failure it produces — it asserts a child's stderr is empty while `MAJIT_STATS=1` writes a `[jit-stats]` line to every process — so the gate lane never runs it and its stale PASS is inert. The gate now selects 201 modules and runs 200 of them in 158s, against 109 in 170s before. Assisted-by: Claude
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ee9f3a7b4
ℹ️ 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".
| // | ||
| // Upstream cannot reach this: `graph.iterblocks()` *is* its block list, | ||
| // so an unreachable block is not in `blocks` and has no link to skip. | ||
| for block_idx in 0..graph.blocks.len() { |
There was a problem hiding this comment.
When FunctionGraph retains the orphan predecessor described here, extending Step 6 to every source block is a workaround in the wrong pass: upstream transform_dead_op_vars walks only its reachable blocks, while upstream mkentrymap(graph) also sees only reachable iterlinks. The later Rust remove_duplicate_inputargs still builds its entry map from every raw graph.blocks link, so the orphan continues to participate in phi-tuple equality and can suppress legitimate duplicate-phi elimination. Preserve the upstream structure by filtering unreachable links when constructing that entry map (or removing the orphan), rather than making dead-variable trimming process links upstream never sees.
AGENTS.md reference: AGENTS.md:L231-L234
Useful? React with 👍 / 👎.
| return true; | ||
| }; | ||
| static USED: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); | ||
| let n = USED.fetch_add(1, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
With MAJIT_MAX_BRIDGES set, this counter advances before bridge tracing is known to be possible. In the green-resume path, for example, should_bridge consumes fuel before blackhole recovery can return pc == usize::MAX, which skips start_bridge_tracing; other paths call start_bridge_tracing, which can normally return false for !state.can_trace(), an evicted owning token, or a declined retrace. Thus MAJIT_MAX_BRIDGES=1 can consume its only slot without compiling any bridge, contradicting the documented “first N bridge compilations” semantics and misleading the intended bisection. Count or log only after the final bridge-path gates have succeeded.
AGENTS.md reference: AGENTS.md:L252-L253
Useful? React with 👍 / 👎.
|
`format(x, 'n')` never consulted the locale. The integer path inserted a separator only inside `if let Some(separator) = p.grouping`, which `'n'` never enters — it carries no `,` or `_` — and the group size came from a per-radix constant with no channel for a locale's grouping vector. The float path delegated render, group and pad end-to-end to the shared engine, which treats `'n'` as `'g'`. Under `LC_ALL=en_US.UTF-8`, `format(123456789, 'n')` was `'123456789'` and `format(1234.5, 'n')` was `'1234.5'`. Port the three routines upstream splits this across. `_get_locale` (`newformat.py:642-658`) branches on the presentation code: `'n'` takes the current locale, an explicit `,`/`_` takes that separator at a fixed group size — four for the power-of-two radices, three otherwise — and anything else carries a stop sentinel. `_group_digits` (`:738-778`) and `_fill_digits` (`:727-736`) perform the insertion for all of those, so `separate_integer_digits` goes away and the `,`/`_` specs travel the same route as `'n'`. The zero fill of a `0=` spec belongs inside the grouping rather than after it: `format(1234, '012n')` is `'0,000,001,234'`, thirteen characters from a width of twelve, because the padding digits are separated like any other. That is what `_calc_num_width`'s `n_min_width` (`:695-704`) carries, and both call sites compute it — `width - (sign + prefix)` for integers, `width - (sign + decimal point + remainder)` for floats, matching `extra_length`. The float path grows the split `_format_float` performs (`:1004-1044`): render unpadded, take the sign off, separate the integer digit run alone, re-emit the decimal point as the locale's, and leave the fraction and any exponent in the remainder, so `format(1e300, 'n')` keeps its exponent ungrouped. Zero padding is rejected for complex specs, so the per-lane `complex_component_spec` split reaches `_group_digits` only at `n_min_width` 0 and needs nothing further. `numeric_formatting` (`rlocale.py:173-178`) is new, placed beside the `_locale` module port so it shares that module's raw `localeconv()` walk: the grouping `format(x, 'n')` separates by and the grouping `locale.localeconv()` reports come out of one read. The walk keeps a `CHAR_MAX` element, which `rustpython_host_env::locale`'s reader drops — dropping it collapses "stop" onto "repeat the last group". Off unix, without `host_env`, and under sandbox the C locale's values stand in; upstream declares `localeconv` `sandboxsafe=True` and reads the host locale even there, but pyre's sandbox build replaces `_locale`'s host entry points with raising stubs and `format()` must not acquire a raising path. Six unit tests pin `_group_digits` against values taken from the vendored source: the zero-fill widths, the repeat-the-last-group rule, the stop sentinel, and a multi-byte separator surviving the buffer reverse. None are covered by `test_format.test_locale`, which asserts only that the separator appears — the test that made this visible when #1138 promoted its module into the gated set. Assisted-by: Claude
MAJIT_MAX_BRIDGES and MAJIT_BRIDGE_FUEL_LOG arrived in 36666ef (#1138) and had no rows. Both are read inside bridge_fuel_take() in majit-metainterp/src/jitdriver.rs. Both descriptions are sourced to MAJIT_MAX_BRIDGES's doc comment, which is also the only place MAJIT_BRIDGE_FUEL_LOG is described - it has no doc comment of its own, so its row carries the one clause that names it plus the message format read off the site. Retirement conditions are UNRECORDED for both; the introducing commit states neither. The catalog is 50 rows. Assisted-by: Claude
1 participant
Three independent changes that came out of chasing a bridge-path wrong-code
failure. The failure itself turned out to be #1130, already merged — this
branch is rebased onto it and carries only what the hunt produced on its own
merits.
prune_dead_phistrimmed a link the way its own reachability walk saw itprune_dead_phis(majit/majit-translate/src/model.rs, the port ofsimplify.py:425-479 transform_dead_op_vars) drops a deadblock.inputargs[i]from every reachable block in Step 7, and trimmed the matching
Link.args[i]in Step 6 only for links whose source block was reachable.Those two scopes are not the same set. A block the reachability walk excludes
can still name a reachable block as its link target — an orphan
eliminate_empty_blocksbypassed, or one of the merge blocksjtransformleaves whose inputargs are phi targets with no backing
OpKind::Input. Itslink kept the full argument list while the target lost a column.
Nothing asserts
len(link.args) == len(link.target.inputargs), so the damagesurfaces one pass later.
remove_duplicate_inputargsreads each phi column byindex across every incoming link, so the untrimmed link contributes its value
one slot over; the union-find then merges two variables that hold different
values, and the resulting rename is applied to every block in the graph.
Keying the trim on the target's reachability closes the gap. Reachability
is closed under exits, so this only ever adds links to the trim — it can never
drop one that was previously trimmed.
Upstream has no equivalent gap:
graph.iterblocks()is its block list, so anunreachable block is not in
blocksand there is no link for it to skip.A regression test builds the shape directly — entry and an orphan both jumping
to a merge whose inputargs are
[dead, live]— and fails on the previous codewith the surviving column reading back as the orphan's value.
locals_w_mut!accepted a shared receiverlocals_cells_stack_wis a raw pointer, so it reads fine through a shared&PyFrame. The macro that replaced the old&mut selfaccessor therefore leta safe caller holding a shared frame mint
&mutto the array.&mut *on thereceiver restores the requirement the accessor had.
The result's lifetime stays unconstrained —
&mut *ptrhas none to bind — sooverlapping calls still alias invisibly to the borrow checker. Binding it needs
a function signature, which is the accessor form whose separate graph defeats
the getfield/setfield pairing the macro exists for; the doc comment says so
rather than leaving the remaining hole unmarked.
MAJIT_MAX_BRIDGES— opt-fuel over bridge compilationMAJIT_NO_BRIDGEis all-or-nothing. It can attribute a wrong value to thebridge path but not to a bridge.
MAJIT_MAX_BRIDGES=Ncompiles the first N anddeclines the rest, which turns that attribution into a bisection over N: the
boundary names the compilation that first produces the wrong value.
The counter is consumed last in the
should_bridgechain, so it only advanceson a guard failure the other conditions already admitted and the index counts
bridges actually taken.
MAJIT_BRIDGE_FUEL_LOG=1prints each one, which is whatpairs an index with the
@@@GUARDlineMAJIT_GUARDLOGemits beside it.On the failure that motivated it the bisection was 6 runs of a few seconds each
against a repro that had cost a full rebuild per arm, and it landed on a single
bridge index with a stable 3/3 boundary on both sides.
The suite baseline recorded 91 working modules as broken
Separate from the three changes above, and found while asking what the green
gate actually covers.
pyre/cpython_tests/run.pyruns only the modulesbaseline.jsonrecords asPASS — everything else is deselected and carries no signal. That makes the file
stale in one direction: a module that starts passing stays outside the gate
until someone runs the non-PASS set and records it. Nothing does that
automatically.
Running the full 434 on dynasm turns up 94 such modules. 91 reproduce as PASS in
a second run at lower concurrency and are promoted here — 63 IMPORTERROR, 16
CRASH, 9 FAIL, 3 TIMEOUT. The CRASH set alone includes
test_dict,test_tuple,test_range,test_slice,test_decimal,test_baseexception,test_userdict,test_userlist,test_weakset,test_queue,test_configparserandtest_format.Three more (
test_embed,test_frozen,test_tools) pass under--fullbutstay put: they are
KNOWN_SKIPS, which is a policy about what the suite shouldnever gate on rather than a record of what works.
--fullalso reportstest_c_locale_coercionasPASS -> FAIL; that is aKNOWN_SKIPSentry whosestated reason is the failure it produces (it asserts a child's stderr is empty
while
MAJIT_STATS=1writes a[jit-stats]line to every process), so the gatelane never runs it and its stale PASS is inert.
The gate now selects 201 modules and runs 200 in 158s, against 109 in 170s
before — the coverage roughly doubles and the wall clock does not move.
What remains genuinely broken on dynasm, for the record: 137 FAIL, 26 TIMEOUT,
10 IMPORTERROR, and 4 CRASH (
test_mmap,test_resource,test_statistics,test_weakref).Gates
All on
729bcb51c9e, which did not move across the runs; working tree clean;build/llbc/*.ullbcre-extracted for all three crates first, and no runreported
LLBC STALE.cargo test --all --no-default-features --features dynasmtest resultlinespyre/check.py --backend dynasmpyre/check.py --backend craneliftpyre/extra_tests/parity_tests/run.pyall parity tests passtest.test_pickletools— red on the previous base and the reason this branchwent looking — passes here both with bridges on and under
MAJIT_NO_BRIDGE=1,which is #1130 doing its job; the baseline already recorded it as
PASS, so itwas a real regression rather than a stale entry.
🤖 Generated with Claude Code