diff --git a/AGENTS.md b/AGENTS.md index 380ad80b5b3..824c00dc071 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,311 +1,217 @@ # AGENTS.md -## How pyre's JIT is built: meta-tracing by source translation - -pyre is structured like PyPy. `pyre-interpreter` is the Rust interpreter (the -analog of PyPy's RPython interpreter). **The JIT is not hand-written** — -`majit-translate` reads the interpreter's Rust source and *generates* it: -`front/ast.rs` (parse) → `flowspace/` (flow-graph build, the -`flowcontext.py`/`framestate.py` analog) → `annotator/` (`annrpython.py` type -inference) → `rtyper/` (low-level lowering) → `codewriter/` -(`jtransform.py`/`codewriter.py`, emits JitCode). This is the same pipeline -RPython's translator + `jtransform` run over PyPy's interpreter. - -**Consequence — "Rust can't be meta-traced" is never a valid excuse for a -deviation.** Generating the JIT from the interpreter source *is* meta-tracing, -by the same principle: whatever semantics the interpreter source expresses is -what the generated JIT must preserve. A JIT that diverges from the -interpreter's behavior has a *generation defect to fix*, not an inherent -limitation of "the JIT is Rust, not Python." Do not justify a mismatch by -appeal to the implementation language. - -### Frame identity must be preserved per frame - -PyPy keeps one frame object per inlined Python call — `MIFrame` while tracing, -`BlackholeInterpreter` on resume — each carrying its own -`jitcode → pycode → w_globals → locals`. `LOAD_GLOBAL` reads -`self.get_w_globals()` off the *live* frame (`pyframe.py:128-132`: -`jit.promote(self.pycode).w_globals`); guard-failure resume rebuilds one frame -per encoded jitcode header (`resume.py:1042-1057`). Caller/callee namespace -confusion is therefore *impossible* — there is no shared frame slot. - -The frame is the interpreter loop's single **red** input; `pycode` is the -**green**. The generated per-code jitcode must thread that red frame for -**every** frame, including inlined non-portal callees. Collapsing inlined -callees onto a single shared anchor (one `portal_frame_reg`, or a single -bridge-resume root frame) drops the callee's own pycode/globals/locals and -makes a cross-module `LOAD_GLOBAL` resolve against the *caller's* globals. -This whole class of bug (the pycode-`names` miscompile, the LOAD_GLOBAL -namespace mismatch, bridge-resume inline-frame globals, vable-resident root -locals) is one root cause — a *frame-identity collapse*. Fix it by restoring -the per-frame red frame (converging on RPython's 1-red-arg frame shape), never -by baking a single anchor's value as a constant. - -## Charon LLBC extraction — the prepass/census input - -The annotator/rtyper prepass (and the `PYRE_RTYPER_VERBOSE` census) does **not** -read the interpreter's Rust source directly — it reads pre-extracted Charon -`.ullbc` artefacts under `build/llbc/*.ullbc`. **These are frozen snapshots: a -change to `pyre-interpreter` / `pyre-object` / `pyre-jit` *source* is invisible -to the prepass until the `.ullbc` is re-extracted.** Only `majit-translate` -(translator) changes take effect without re-extraction, because the translator -runs live over the frozen `.ullbc` bodies. - -Charon is a **shared, pre-installed** dependency — it lives in the communal -build cache (`../.pyre-build/charon//charon`, pinned -`nightly-2026.05.29`), **not** on `PATH`. So `which charon` finds nothing, yet -the scripts below work because they resolve that cache path directly. Do **not** -conclude "charon is missing" from `which charon`; check -`../.pyre-build/charon//` (override with `PYRE_SHARED_BUILD` / -`CHARON_DEST`). - -Two scripts manage this (both `python3`, run from repo root): - -- **`scripts/install-charon.py`** — fetch/build the pinned Charon into the - shared cache. Idempotent: prints "already installed" and exits if the stamped - version matches. Usually a no-op since the cache is communal. -- **`scripts/extract-llbc.py [crates…]`** — (re)extract `.ullbc`. No args ⇒ - `DEFAULT_CRATES` (`pyre-object pyre-interpreter pyre-jit`); known crates are - `corpus pyre-object pyre-module pyre-interpreter pyre-jit`. It has - **source-fingerprint skip logic**: a crate whose tracked source is unchanged - prints `=== skipping … (fingerprint unchanged) ===` and is *not* - rewritten. Force a full rebuild with `--force` (or `LLBC_FORCE_REEXTRACT=1`). - `pyre-interpreter.ullbc` is ~300 MB and a forced re-extract takes minutes. - - While extracting `pyre-jit.ullbc`, the shared extraction driver sets the - internal `MAJIT_LLBC_EXTRACTION=1` mode. This breaks the - `pyre-jit → pyre-jit-trace → pyre-jit.ullbc` bootstrap cycle by making - `pyre-jit-trace/build.rs` emit compile-only placeholder artifacts. Do not set - this variable for ordinary builds: the placeholders contain no usable JIT - metadata. `cargo::rerun-if-env-changed` makes the next normal build replace - them with artifacts generated from the completed LLBC set. - -```bash -python3 scripts/install-charon.py # ensure charon (usually no-op) -python3 scripts/extract-llbc.py # re-extract the 3 default crates -``` - -After re-extraction, **rebuild the prepass** to re-run it against the new -`.ullbc`, then bucket the census: +## The JIT is generated from the interpreter source + +pyre is structured like PyPy: `pyre-interpreter` is the RPython-interpreter +analog, and **the JIT is not hand-written**. `majit-translate` reads the +interpreter's Rust source and generates it — `front/ast.rs` (parse) → +`flowspace/` (`flowcontext.py`/`framestate.py`) → `annotator/` (`annrpython.py`) +→ `rtyper/` → `codewriter/` (`jtransform.py`/`codewriter.py`, emits JitCode) — +the same pipeline RPython's translator runs over PyPy. + +**So "Rust can't be meta-traced" is never a valid excuse for a deviation.** +Whatever the interpreter source expresses is what the generated JIT must +preserve; a JIT that diverges from the interpreter has a generation defect to +fix. Never justify a mismatch by appeal to the implementation language. + +### One red frame per frame + +PyPy keeps one frame object per inlined Python call (`MIFrame` tracing, +`BlackholeInterpreter` resuming), each with its own +`jitcode → pycode → w_globals → locals`. `LOAD_GLOBAL` reads it off the *live* +frame (`pyframe.py` `get_w_globals`); resume rebuilds one frame per encoded +jitcode header (`resume.py` `rebuild_from_resumedata`). No shared frame slot +exists, so namespace confusion is impossible. + +The frame is the loop's single **red** input, `pycode` the **green**. Thread +that red frame through **every** frame, inlined non-portal callees included. +Collapsing callees onto one anchor (a single `portal_frame_reg`, a single +bridge-resume root frame) drops the callee's own pycode/globals/locals — the one +root cause behind the pycode-`names` miscompile, the LOAD_GLOBAL namespace +mismatch, bridge-resume inline-frame globals and vable-resident root locals. Fix +it by restoring the per-frame red frame, never by baking an anchor's value as a +constant. + +## Charon LLBC extraction — the prepass input + +The annotator/rtyper prepass and the `PYRE_RTYPER_VERBOSE` census read +**pre-extracted `.ullbc` under `build/llbc/`, not the Rust source**. A change to +`pyre-interpreter` / `pyre-object` / `pyre-jit` is invisible until re-extraction; +`majit-translate` changes take effect immediately, because the translator runs +live over the frozen bodies. + +Charon is shared and pre-installed at `../.pyre-build/charon//charon` +(pinned `nightly-2026.05.29`), **not on `PATH`** — `which charon` finding nothing +does not mean it is missing. ```bash +python3 scripts/install-charon.py # idempotent, usually a no-op +python3 scripts/extract-llbc.py # pyre-object pyre-interpreter pyre-jit touch pyre/pyre-jit-trace/build.rs -PYRE_RTYPER_VERBOSE=1 cargo build --release -p pyre-jit-trace # build.rs runs the prepass -# newest stderr: target/release/build/pyre-jit-trace-*/stderr -# rg -c 'PREPASS phaseA fail' # / phaseB +PYRE_RTYPER_VERBOSE=1 cargo build --release -p pyre-jit-trace # runs the prepass +# census: target/release/build/pyre-jit-trace-*/stderr, rg -c 'PREPASS phaseA fail' ``` -So: interpreter-source work that should move the census ⇒ `extract-llbc.py` -first, then the prepass rebuild. Translator-only work ⇒ just the prepass -rebuild. (`rg` note: a stray `--replace` in user config can mangle these long -lines, e.g. `GcType`→`n`; pass `rg --no-config` when bucketing.) +- `extract-llbc.py` **skips** a crate whose source fingerprint is unchanged; + `--force` / `LLBC_FORCE_REEXTRACT=1` overrides. `pyre-interpreter.ullbc` is + ~300 MB and takes minutes. +- It internally sets `MAJIT_LLBC_EXTRACTION=1` to break the + `pyre-jit → pyre-jit-trace → pyre-jit.ullbc` bootstrap cycle, which makes + `build.rs` emit placeholder artifacts. Never set it for an ordinary build. +- Interpreter-source work ⇒ re-extract, then rebuild the prepass. + Translator-only work ⇒ prepass rebuild alone. +- `rg` note: a stray `--replace` in user config mangles long lines; pass + `rg --no-config` when bucketing. + +## The wasm gate runs on every OS + +Its only prerequisite beyond the native backends is the +`wasm32-unknown-unknown` target — wasmtime is linked into `pyre-wasm-runner`, +and the guest builds on stable with no `-Z build-std`. Once the target is +installed `DEFAULT_BACKENDS` **adds wasm by itself**, so a bare +`python3 pyre/check.py` runs three backends and `--backend dynasm,cranelift` +*narrows* it. + +CI installs that target on the ubuntu leg alone because wasm output is +platform-independent — **a cost decision, not a capability limit.** Never defer +a wasm-only failure to CI on the grounds that the host cannot reproduce it. ## Data structure parity with RPython/PyPy -**Treat every `HashMap` and every thread-local (`thread_local!`, TLS) as -suspicious when porting RPython/PyPy code.** Find the corresponding PyPy or -RPython owner and storage shape before choosing a Rust container or lifetime. - -majit and pyre are line-by-line ports. The data structure choice is part of -the port — it must match what RPython/PyPy actually uses, even when a Rust -collection looks more convenient. - -### Rules - -1. **Look up the RPython/PyPy source first.** Before adding `HashMap`, `HashSet`, - `BTreeMap`, etc., find the corresponding RPython attribute and check what - container it uses (`dict`, `list`, an attribute on a class instance, a - field on `_forwarded`, …). Port that exact shape. - - A Rust `HashMap` is not the default translation of a Python `dict`. When - lookup is over a small/dense key space, stable insertion order matters, or - identity/index lookup is sufficient, `VecMap`, `IndexMap`, or an ordinary - `Vec` is often the closer representation. Prove the required semantics from - the upstream code before choosing among them. - -2. **Side-tables are usually wrong.** RPython optimizers store information - *on the box itself* via `box._forwarded` / `PtrInfo` / `IntBound` / - descr attributes. If you find yourself reaching for - `HashMap` to track a per-box property, that is almost - always a sign you skipped the proper PtrInfo / forwarded slot and are - inventing a parallel store that RPython does not have. Stop and route - the data through the existing forwarded/PtrInfo machinery instead. - -3. **Borrow-checker workarounds must be minimal and documented.** A - `HashMap` introduced purely because the borrow checker rejected a more - direct port is acceptable only when (a) every alternative has been - tried, (b) the deviation is the smallest possible, and (c) a comment - cites the RPython original it stands in for. See the - "RPython Parity Rules" section below. - -4. **Removing an RPython method to "simplify" things is not allowed.** - If `optimizer.py` defines `ensure_ptr_info_arg0`, the Rust port has - `ensure_ptr_info_arg0`. Do not delete it because callers can be - rewritten to a shortcut — the shortcut diverges from RPython and the - next porter will have no idea why their `heap.py` line-by-line port - no longer compiles. - -5. **TLS is almost never the right owner for runtime state.** Type objects, - module state, registries, semantic caches, and any value whose identity or - contents must be visible across threads are process-global or - interpreter-owned in PyPy and must remain shared in pyre. Never duplicate - them per thread merely to make a raw pointer satisfy Rust's `Sync` rules; - use the proper global/interpreter owner (for example the established - process-global immortal-type `OnceLock` pattern) and preserve GC - rooting as required. - - TLS is acceptable only when the PyPy/RPython source makes the state itself - thread-specific (for example the current thread/execution context or errno), - or for a disposable temporary cache that cannot affect observable identity, - semantics, lifetime, or GC reachability. Every other TLS use requires an - upstream citation and a written justification in the code. When in doubt, - assume TLS is wrong and find the shared owner. - -### Why - -We have already been bitten by this. A previous change deleted -`ensure_ptr_info_arg0` and replaced `arrayinfo.lenbound.make_gt_const(...)` -with a side-table `OptHeap.array_min_lengths: HashMap`. The -side-table then could not be read by `postprocess_arraylen_gc`, so that -function was crippled to a hardcoded `IntBound::nonnegative()`, which then -forced `ExportedValueInfo` to grow a parallel `int_lower_bound` field. -One non-orthodox `HashMap` cascaded into four files of divergence from -RPython. Don't start the cascade. - -### When in doubt - -Grep RPython: - -``` -rg -t py 'lenbound|getlenbound|_x86_arglocs|_ll_loop_code' rpython/jit/ -``` - -For a *behavioural* question rather than a structural one — "does upstream -really do this?" — grepping is the second step; see "The PyPy oracle" below. +majit and pyre are line-by-line ports, so the container choice is part of the +port. **Treat every `HashMap` and every `thread_local!` as suspicious**: find the +upstream owner and storage shape first. + +1. **Look up the upstream attribute before choosing a container.** A Rust + `HashMap` is not the default translation of a Python `dict` — over a + small/dense key space, or where insertion order or index lookup suffices, + `VecMap` / `IndexMap` / `Vec` is closer. Prove the semantics from upstream. +2. **Side-tables are usually wrong.** RPython stores per-box information *on the + box* (`box._forwarded`, `PtrInfo`, `IntBound`, descr attributes). Reaching for + `HashMap` means you skipped that machinery; route through + `OptContext::with_intbound_mut` / `set_ptr_info` instead. +3. **A borrow-checker workaround** is acceptable only when every alternative was + tried, the deviation is minimal, and a comment cites the RPython original. +4. **Do not delete an RPython method to "simplify".** If `optimizer.py` has + `ensure_ptr_info_arg0`, the port has it — the shortcut diverges, and the next + porter's `heap.py` port stops compiling for no visible reason. +5. **TLS is almost never the right owner.** Type objects, module state, + registries and semantic caches are process-global or interpreter-owned in + PyPy and stay shared here; never duplicate them per thread to satisfy `Sync`. + TLS is right only where upstream makes the state itself thread-specific + (current thread/execution context, errno) or for a disposable cache that + cannot affect identity, semantics, lifetime or GC reachability. Anything else + needs an upstream citation in the code. + +The measured cascade this prevents: deleting `ensure_ptr_info_arg0` for a +side-table `OptHeap.array_min_lengths` left `postprocess_arraylen_gc` unable to +read it, so it was crippled to a hardcoded `IntBound::nonnegative()`, which then +forced a parallel `ExportedValueInfo::int_lower_bound`. One non-orthodox +`HashMap`, four files of divergence. + +## The PyPy oracle: run it before arguing about orthodoxy + +`rpython/` says what upstream *claims*; a real `pypy3` shows what it *does*. For +"is this orthodox or our deviation?", run the oracle **first** — before reading +source, forming a theory, or recording a verdict. - -### Workflow guideline - -If RPython stores it on an object attribute, store it on the equivalent -Rust struct field. If RPython stores it on `box._forwarded`, route it -through `OptContext::with_intbound_mut` / `set_ptr_info` / etc. Reach -for `HashMap` only after you have proven that RPython itself uses a -dict-like container in that exact spot. Apply the same test to TLS: locate the -upstream owner, then preserve whether it is global, interpreter-local, -execution-context-local, or genuinely thread-local. - -## The PyPy oracle: run it before you argue about orthodoxy - -Reading `rpython/` tells you what upstream *says*. Running a real `pypy3` tells -you what upstream *does*. When the question is "is this JIT behaviour orthodox, -or is it our deviation?", run the oracle FIRST — before reading source, before -forming a theory, and certainly before recording a verdict. - -``` -PYPYLOG=jit-summary:- pypy3 pyre/bench/synth/.py +```bash +PYPYLOG=jit-summary:- pypy3 pyre/bench/synth/.py # vs MAJIT_STATS=1 ``` -Most `pyre/bench/synth` fixtures use no stdlib and run unmodified under the -real interpreter, so this costs one command. Read these keys: `Total # of -loops` / `Total # of bridges`, `forcings`, `virtualizables forced`, every -`abort: *`, `nvirtuals`. Compare against `MAJIT_STATS=1` on the same file. - -**A counter that differs is a pointer, not the answer.** Go find the upstream -line that produces it — the decision is usually one JIT hint -(`@jit.look_inside_iff`, `@jit.dont_look_inside`, `@jit.elidable`, -`@jit.unroll_safe`) sitting on the function in question. Cite it. If the -summary is too coarse, `PYPYLOG=jit-log-opt:FILE` dumps the optimized trace. - -Worked example (2026-08-03). `getframe_inline_subwalk_multiframe` failed 8948 -GUARD_NOT_FORCED, and the standing conclusion was "a GUARD_NOT_FORCED never -compiles a bridge (`compile.py:950-953`), so this is unfixable by construction." -The oracle reported one loop, no bridges, **`forcings: 0`**, **`virtualizables -forced: 0`**, no aborts — PyPy never forces here at all, so the guard should not -exist. `pypy/module/sys/vm.py:41` then names the decision in one line: -`@jit.look_inside_iff(lambda space, depth: jit.isconstant(depth))` on -`getframe`, which pyre folds into a single opaque builtin. A verdict that had -stood for weeks was overturned, and an unbounded "epic" turned into a named -port, by one command. Note also `pypy3` is 3.11 — a fixture using newer syntax -needs trimming to the subset that runs. - -## Spec follows CPython 3.14; implementation follows PyPy - -Standing ruling: **pyre's *implementation* is a port of PyPy; pyre's *spec* — -what a Python program can observe — is CPython 3.14.** A behavioural difference -from PyPy is a parity regression **unless** a CPython 3.14 artefact shows PyPy is -wrong about what the caller observes. Then it is a spec fix, and PyPy's shape -still governs every other line on the way there. - -**This is not a 3.11-vs-3.14 question, and reading it as one is why the same -findings get re-filed every review cycle.** Of seven adjudicated cases, six have -no version delta at all: `sched_setscheduler` has returned None since 3.3, -`PyUnicode_FSConverter` has accepted bytes since 3.3, PEP 529 surrogatepass is -3.6, `DirEntry` has cached its `stat_result` since PEP 471. These are standing -PyPy-vs-CPython divergences, not PyPy lagging a release. "3.14" pins *which* -CPython you read (`lib-python/stdlib-version.txt`); it does not narrow the rule -to version lag, and the absence of a delta is not grounds to refuse the -exception. Conversely a real delta earns nothing on its own. - -**What the spec governs** is only what a caller can observe: a return value, an -exception's type / message / attributes, object identity, an -encoding-and-errors contract, and which argument shapes are accepted or -rejected. Everything else follows PyPy **unconditionally** — names, module +Most `pyre/bench/synth` fixtures need no stdlib, so this costs one command. Read +`Total # of loops`/`bridges`, `forcings`, `virtualizables forced`, every +`abort: *`, `nvirtuals`. **A differing counter is a pointer, not the answer** — +find the upstream line behind it, usually a single JIT hint +(`@jit.look_inside_iff`, `dont_look_inside`, `elidable`, `unroll_safe`), and cite +it. `PYPYLOG=jit-log-opt:FILE` dumps the optimized trace when the summary is too +coarse. `pypy3` is 3.11, so trim newer syntax out of the fixture. + +This overturned a weeks-old "unfixable by construction" verdict on +`getframe_inline_subwalk_multiframe` in one command: the oracle reported +`forcings: 0`, and the `@jit.look_inside_iff` on `getframe` +(`pypy/module/sys/vm.py`) named the reason. + +## Spec follows CPython 3.14t; engineering follows PyPy + +**Pursue PyPy parity to the extreme as engineering, and CPython 3.14t as spec; +where they collide, take the 3.14t behaviour and engineer it the way PyPy +would.** Neither goal yields wholesale — the axis decides which one governs the +line in front of you. + +The `t` is the **free-threaded** build: "CPython does X" is an answer only once X +holds without the GIL. Correct-because-a-global-lock-serialises-it is not +on-spec. + +**The spec governs only what a caller can observe** — return value, exception +type/message/attributes, identity, encoding-and-errors contract, accepted +argument shapes. Everything else follows PyPy **unconditionally**: names, module paths, control-flow order, data structures, storage owner, JIT hints. A structural divergence does not become a spec fix by sitting next to one. -**Six tests, in order; stop at the first leaf.** The full procedure, with the -evidence rules and worked examples, is in the `/parity` skill under -"SPEC-DEVIATION"; do not invoke this ruling without reading it. - -1. **Can a Python snippet print a difference?** No → ordinary parity finding. -2. **Do you hold an admissible artefact for the 3.14 side?** An in-tree - `lib-python/3/…:line` assertion, a measured run at the pinned version, or C - source read at that tag in a named checkout. Prose (docs, PEPs) is not - admissible, and a comment in pyre's own source is never the artefact. No - artefact → you may not invoke this section. -3. **Do the two upstreams actually disagree?** If they agree and pyre differs - from both, that is a plain regression and no spec reasoning rescues it. -4. **Is PyPy's shape load-bearing for a mechanism pyre also has?** Search the - whole definition — decorators included — plus the class/module bindings it - reads, in `rpython/` as well as `pypy/`, for `@jit.*`, `_immutable_*`, - `_attrs_`, `unrolling_iterable`, `make_sure_not_resized`, `rgc.*`. A hint - that governs the value you are changing → **STOP, follow PyPy**; that is - implementation, which this ruling assigns to PyPy. Record the negative - search too. -5. **Per-site artefact plus a blast-radius census.** Every departing - `file:line` needs the artefact that forces *that site*; "consistency with a - sibling" is not one. Then `rg` pyre's own readers of the shape you are - deleting, `pyre-jit*` and `majit*` included. -6. **Does pyre land on 3.14 across the whole decision?** Adjacency is what - reads the state you changed, not "the same function". Landing where - **neither** upstream sits is a defect regardless of which axis matched. - -Reaching the end: file it under the review's `## 4. Structural adaptations` as -`[3.14-spec] our_file.rs:line ↔ pypy_file.py:line — ; evidence: -`, and comment at the site citing both sides. That records the -decision; it does not close it. - -## RPython Parity Rules -- When porting from RPython/PyPy, do STRICT line-by-line structural parity. Do NOT take shortcuts, reimplement from scratch, or declare phases 'complete' without the literal refactor. -- If a parity fix causes regressions, investigate root cause before reverting. Do not declare success if structural alignment was skipped, even if benchmarks pass. -- Always verify which worktree/repo you're in (`git rev-parse --show-toplevel`) before editing. Common worktrees: pypy/main, pypy-pyre, pypy-stdlib, pypy-side. - -## Before Committing -- Always run `cargo check` and `cargo test` with `--features dynasm`. -- Run the full benchmark suite (all 8 benchmarks) after JIT changes. A regression - is a finding to explain, not an automatic veto. The `/parity` skill's - Principle 4 governs: *"Performance can temporarily regress. If a benchmark - slows down because parity-correct code replaced a clever local shortcut, - accept it. Performance is recovered by further line-by-line porting of the - upstream optimization, not by reintroducing the shortcut."* So: if the slower - code is the line-by-line port and the faster code was the shortcut, the port - stands — record the regression and name the upstream optimization that would - recover it. Revert only when the regression has no such explanation. This file - is loaded every session and Principle 4 is not; do not restate the rule here - in a form that contradicts it. -- Check `git status` and `git rev-parse --show-toplevel` before staging to confirm correct worktree. -- When rebasing/cherry-picking, verify the fix isn't already on main first (`git log main --grep=...`). - -## Debugging Discipline -- When adding trace/debug logs, verify the code path is actually reached (check gating, feature flags) before running the test. -- For root-cause bugs, do NOT implement workarounds (e.g., builtin fallback modules) - fix the actual interpreter/JIT issue. +**It is not a 3.11-vs-3.14 question** — six of seven adjudicated cases had no +version delta at all (`sched_setscheduler` since 3.3, `PyUnicode_FSConverter` +since 3.3, PEP 529, PEP 471). "3.14" pins *which* CPython you read +(`lib-python/stdlib-version.txt`); a missing delta is not grounds to refuse the +exception, and a real one earns nothing by itself. + +**Six tests, in order; stop at the first leaf.** The full procedure is in +`/parity` under "SPEC-DEVIATION" — do not invoke this ruling without reading it. + +1. Can a Python snippet print a difference? No → ordinary parity finding. +2. Do you hold an admissible 3.14 artefact — an in-tree `lib-python/3/…` + assertion, a measured run at the pinned version, or C source read at that tag? + Prose is not admissible; a comment in pyre's own source is never the artefact. +3. Do the two upstreams actually disagree? If they agree and pyre differs from + both, it is a plain regression that no spec reasoning rescues. +4. Is PyPy's shape load-bearing for a mechanism pyre also has? Search the whole + definition, decorators included, in `rpython/` and `pypy/` for `@jit.*`, + `_immutable_*`, `_attrs_`, `unrolling_iterable`, `make_sure_not_resized`, + `rgc.*`. A hint governing the value you are changing → **STOP, follow PyPy**. + Record the negative search too. +5. Per-site artefact plus a blast-radius census: every departing site needs the + artefact forcing *that* site ("consistency with a sibling" is not one), then + `rg` pyre's own readers, `pyre-jit*` and `majit*` included. +6. Does pyre land on 3.14t across the whole decision? Landing where **neither** + upstream sits is a defect however well one axis matched. + +File the result under the review's `## 4. Structural adaptations` as +`[3.14-spec] ↔ — ; evidence: `, and +comment at the site citing both sides. + +## Porting discipline + +- Strict line-by-line structural parity. No shortcuts, no reimplementation from + scratch, no declaring a phase complete without the literal refactor. +- If a parity fix regresses, find the root cause before reverting. Structural + alignment skipped is not success, even with green benchmarks. +- Cite upstream by **symbol**, not `file:line`. Numbers rot silently and a + rotted citation still reads as authoritative; a symbol stays checkable with + `rg`. Use a line number only where no symbol pins the claim, and name the + enclosing symbol beside it. +- Confirm the worktree (`git rev-parse --show-toplevel`) before editing and + before staging — dozens of sibling worktrees share one `.git`. + +## Before committing + +- `cargo test --all --features dynasm`. The feature flag is mandatory: without it + `majit-metainterp` emits `compile_error!` and every error after it is noise. +- `python3 pyre/check.py` — every backend the host can build. A perf regression + is a finding to explain, not an automatic veto: if the slower code is the + line-by-line port and the faster was a shortcut, **the port stands** — record + it and name the upstream optimization that would recover it (`/parity` + Principle 4). Revert only when the regression has no such explanation. This + file is loaded every session and Principle 4 is not, so do not restate that + rule here in a form that contradicts it. +- Re-record a `.jitstats` baseline only when the new number is the one that + should hold. "The recorded number no longer matches" is never on its own a + reason: a gate is a target to reach, not a figure to refit. +- When rebasing or cherry-picking, check the fix isn't already on main + (`git log main --grep=…`). + +## Debugging discipline + +- Before running the test, verify the traced path is actually reached — check + gating and feature flags. +- Fix the interpreter/JIT root cause. Do not build workarounds (fallback + modules, special cases) around it. diff --git a/majit/majit-backend-wasm/src/codegen.rs b/majit/majit-backend-wasm/src/codegen.rs index bf249e32204..205455eee7a 100644 --- a/majit/majit-backend-wasm/src/codegen.rs +++ b/majit/majit-backend-wasm/src/codegen.rs @@ -3168,24 +3168,6 @@ fn build_function( let region_spans = InlinedRegionSpan::collect(ops.len(), inlined_bridges); let liveness = HomeLiveness::collect_with_regions(inputargs, ops, ®ion_spans); - // `LOAD_FROM_GC_TABLE` is the backend form of a ConstPtr. Native PyPy - // keeps such loop-invariant references in their allocated location across - // a loop; reloading the GC-table slot on every iteration is not part of - // the operation's semantics. Do the same for a trace with no collecting - // operation: eager loads are safe, and no moving collection can stale the - // local before the trace exits. Traces containing a call/allocation keep - // the original program points and the ordinary home/reload machinery. - let hoisted_gc_table_loads: indexmap::IndexSet = - if has_loop && !ops.iter().any(|op| op.opcode.can_malloc()) { - ops.iter() - .filter(|op| op.opcode == OpCode::LoadFromGcTable) - .map(|op| op.pos.get()) - .filter(|result| *result != OpRef::NONE && !result.is_constant()) - .collect() - } else { - indexmap::IndexSet::new() - }; - // Resume-at-LABEL: a peeled loop wraps its preamble in a dispatch so a // loop-closing bridge can re-enter AT any LABEL — key = label ordinal + 1 // — skipping the code before it, in-module instead of round-tripping @@ -3331,23 +3313,6 @@ fn build_function( } } - // Seed loop-invariant GC-table references after the fresh-entry home clear - // and input setup. Store-on-def is mirrored here because the original op - // arm and its common tail are skipped below. - for result in &hoisted_gc_table_loads { - emit_seed_gc_table_ref( - &mut sink, - ops, - constants, - value_types, - ref_homes, - frame, - gc_table_base, - gc_table_bases, - *result, - ); - } - // Seed with the fail-index base so each guard/finish exit writes // `base + local` into `frame[0]` (every trace passes the next free index // of the global fail-index space, `failguard::fail_descr_base`). The local @@ -3427,25 +3392,6 @@ fn build_function( sink.i64_store(mem64(frame.home_slot_base + h as u64 * SLOT_SIZE)); } } - // The dispatch branch skipped the eager ConstPtr loads emitted on - // key 0. Their ordinary homes are the low prefix a chained bridge - // clears and remaps for its own Refs, so what sits there on resume - // may be zero or another trace's object; the gc_table slot is the - // root the collector forwards in place, so read it again exactly - // as fresh entry does. LABEL entry is cold enough to pay for it. - for result in &hoisted_gc_table_loads { - emit_seed_gc_table_ref( - &mut sink, - ops, - constants, - value_types, - ref_homes, - frame, - gc_table_base, - gc_table_bases, - *result, - ); - } sink.end(); // end B_j $past_loader labels_passed += 1; } @@ -4846,7 +4792,7 @@ fn build_function( // address; the collector forwards the slot in place, so the load // reads the reference at its current address. let vi = op.pos.get().raw(); - if !OpRef::raw_is_constant(vi) && !hoisted_gc_table_loads.contains(&op.pos.get()) { + if !OpRef::raw_is_constant(vi) { let index = resolve_const_bits(constants, op.arg(0).to_opref()); let base = gc_table_bases.get(&vi).copied().unwrap_or(gc_table_base); let slot = @@ -6174,9 +6120,7 @@ fn build_function( // skipped. Each value-producing arm is operand-stack-neutral, so this // appended store is balanced. let result = op.pos.get(); - if !hoisted_gc_table_loads.contains(&result) - && let Some(h) = ref_homes.home(result) - { + if let Some(h) = ref_homes.home(result) { sink.local_get(0); sink.local_get(value_types.local(result.raw())); sink.i64_store(mem64(frame.home_slot_base + h as u64 * SLOT_SIZE)); @@ -6449,49 +6393,6 @@ fn resolve_const_bits(constants: &indexmap::IndexMap, opref: OpRef) -> }) } -/// Load one hoisted `LoadFromGcTable` result from its table slot into its -/// local and refresh the ordinary Ref home the in-loop reload path reads. -/// -/// The table slot is the root the collector forwards in place -/// (`assembler.py:1545 genop_load_from_gc_table`), which is why both the -/// fresh-entry seeding and the LABEL resume loader read it rather than a home. -#[allow(clippy::too_many_arguments)] -fn emit_seed_gc_table_ref( - sink: &mut PeepSink<'_, '_>, - ops: &[Op], - constants: &indexmap::IndexMap, - value_types: &ValueLocals, - ref_homes: &RefHomes, - frame: FrameGeometry, - gc_table_base: u32, - gc_table_bases: &HashMap, - result: OpRef, -) { - let producer = ops - .iter() - .find(|op| op.pos.get() == result) - .expect("hoisted GC-table result must have a producer"); - let index = resolve_const_bits(constants, producer.arg(0).to_opref()); - let base = gc_table_bases - .get(&result.raw()) - .copied() - .unwrap_or(gc_table_base); - let slot = base as u64 + index as u64 * std::mem::size_of::() as u64; - sink.i32_const(slot as i32); - sink.i32_load(MemArg { - offset: 0, - align: 2, - memory_index: 0, - }); - sink.i64_extend_i32_u(); - sink.local_set(value_types.local(result.raw())); - if let Some(h) = ref_homes.home(result) { - sink.local_get(0); - sink.local_get(value_types.local(result.raw())); - sink.i64_store(mem64(frame.home_slot_base + h as u64 * SLOT_SIZE)); - } -} - fn emit_resolve( sink: &mut PeepSink<'_, '_>, constants: &indexmap::IndexMap, diff --git a/majit/majit-backend-wasm/tests/codegen_test.rs b/majit/majit-backend-wasm/tests/codegen_test.rs index 9e7d8842011..afaa2daac31 100644 --- a/majit/majit-backend-wasm/tests/codegen_test.rs +++ b/majit/majit-backend-wasm/tests/codegen_test.rs @@ -2342,8 +2342,16 @@ fn test_single_label_peeled_loop_validates() { assert!(!guards[0].is_finish); } +/// A `LoadFromGcTable` placed inside the loop body is emitted inside the loop. +/// +/// `rewrite.py:1100-1115 remove_constptr` caches one load per gc-table index, +/// but `rewrite.py:1003-1006 emit_label` clears `gcrefs_recently_loaded` at +/// every LABEL, so a reference constant used after the LABEL is loaded again on +/// each iteration. The comment there rejects keeping the value alive across the +/// label ("don't spill it") as "the wrong level" — the backend emits the op +/// where the trace puts it and leaves that decision to the optimizer. #[test] -fn loop_invariant_gc_table_load_stays_outside_non_collecting_loop() { +fn gc_table_load_inside_a_loop_body_is_emitted_inside_the_loop() { let inputargs = vec![InputArg::from_type(Type::Int, 0)]; let ops = vec![ make_op( @@ -2435,16 +2443,16 @@ fn loop_invariant_gc_table_load_stays_outside_non_collecting_loop() { } } } - // Without these two, the zero above also holds for a body that emitted no - // loop at all, or dropped the table load entirely. + // Without this, the counts below also hold for a body that emitted no loop + // at all. assert!(saw_loop, "codegen emitted no loop for a looping trace"); - assert!( - loads_outside_loop > 0, - "the hoisted ConstPtr table slot is never loaded" + assert_eq!( + loads_inside_loop, 1, + "the in-loop LoadFromGcTable must be emitted inside the loop" ); assert_eq!( - loads_inside_loop, 0, - "ConstPtr table slot was reloaded on the hot backedge" + loads_outside_loop, 0, + "no gc-table load belongs outside the loop for this trace" ); } diff --git a/majit/majit-metainterp/src/blackhole.rs b/majit/majit-metainterp/src/blackhole.rs index b77f613f46d..2282cf83372 100644 --- a/majit/majit-metainterp/src/blackhole.rs +++ b/majit/majit-metainterp/src/blackhole.rs @@ -6066,7 +6066,15 @@ fn bhimpl_hint_force_virtualizable(_r: i64) {} /// because the value is a distinct compile-time constant per instruction and /// `check_result`'s 256-entry per-kind cap rejects one pool entry per /// instruction — registers this hook and writes the field itself. -pub type LiveMarkerHook = fn(&BlackholeInterpreter, usize); +/// The hook also owns the register file, because the marker names the live +/// set: `cleanup_registers` (`blackhole.py:385`) clears `registers_r` "to +/// avoid keeping references alive", but it only runs at `release_interp` +/// (`blackhole.py:253`), so a register whose live range ended keeps its +/// object for the rest of the run. RPython is insulated by liverange-based +/// colouring reusing that register almost immediately +/// (`rpython/tool/algo/regalloc.py:28-75`); a codewriter whose colours are +/// not reused that densely needs the same clear at marker granularity. +pub type LiveMarkerHook = fn(&mut BlackholeInterpreter, usize); static LIVE_MARKER_HOOK: std::sync::OnceLock = std::sync::OnceLock::new(); diff --git a/majit/majit-translate/src/codewriter/assembler.rs b/majit/majit-translate/src/codewriter/assembler.rs index 63d84f5d3f5..a2f802b37e1 100644 --- a/majit/majit-translate/src/codewriter/assembler.rs +++ b/majit/majit-translate/src/codewriter/assembler.rs @@ -4427,6 +4427,18 @@ fn vable_arraydescrof( base_size: crate::layout::target_word_size(), itemsize, len_offset: Some(0), + // No layout identity, and neither zero is a placeholder to fill in. + // `PyFrame.locals_cells_stack_w` is reached through two allocators — + // `alloc_frame_locals_array`'s GC arm stamps the object-array tid into + // the header, its `alloc_fixed_array_with_header` arm (taken for a + // frame the collector does not own, and as the GC arm's own + // out-of-memory fallback) leaves the prepended header zeroed — so one + // tid cannot describe every block a trace will meet. Stamping either + // slot would put a `GUARD_GC_TYPE` on the short-preamble entry that is + // false for the other arm's blocks. `ArrayPtrInfo::make_guards` + // (`optimizeopt/info.rs`) instead refuses to build the entry, which + // costs the one unrolled attempt `unroll_free_retry_rescued` counts and + // keeps the guard honest. type_id: 0, gc_type_id: 0, item_type, diff --git a/pyre/bench/synth/nested_for_outer_local_postread.cranelift.jitstats b/pyre/bench/synth/nested_for_outer_local_postread.cranelift.jitstats index 36aa2073c1b..f64c6cf6bd9 100644 --- a/pyre/bench/synth/nested_for_outer_local_postread.cranelift.jitstats +++ b/pyre/bench/synth/nested_for_outer_local_postread.cranelift.jitstats @@ -1,4 +1,4 @@ -bridges_compiled=9 +bridges_compiled=8 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=2141 +guard_failures=2113 internal_compile_panics=0 loops_aborted=0 loops_compiled=5 +retraces_compiled=0 diff --git a/pyre/bench/synth/nested_for_outer_local_postread.dynasm.jitstats b/pyre/bench/synth/nested_for_outer_local_postread.dynasm.jitstats index 36aa2073c1b..f64c6cf6bd9 100644 --- a/pyre/bench/synth/nested_for_outer_local_postread.dynasm.jitstats +++ b/pyre/bench/synth/nested_for_outer_local_postread.dynasm.jitstats @@ -1,4 +1,4 @@ -bridges_compiled=9 +bridges_compiled=8 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=2141 +guard_failures=2113 internal_compile_panics=0 loops_aborted=0 loops_compiled=5 +retraces_compiled=0 diff --git a/pyre/bench/synth/nested_for_outer_local_postread.wasm.jitstats b/pyre/bench/synth/nested_for_outer_local_postread.wasm.jitstats index 36aa2073c1b..f64c6cf6bd9 100644 --- a/pyre/bench/synth/nested_for_outer_local_postread.wasm.jitstats +++ b/pyre/bench/synth/nested_for_outer_local_postread.wasm.jitstats @@ -1,4 +1,4 @@ -bridges_compiled=9 +bridges_compiled=8 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=2141 +guard_failures=2113 internal_compile_panics=0 loops_aborted=0 loops_compiled=5 +retraces_compiled=0 diff --git a/pyre/pyre-interpreter/src/jit_fnaddr.rs b/pyre/pyre-interpreter/src/jit_fnaddr.rs index 3708727f220..4b0dde158eb 100644 --- a/pyre/pyre-interpreter/src/jit_fnaddr.rs +++ b/pyre/pyre-interpreter/src/jit_fnaddr.rs @@ -3563,17 +3563,27 @@ mod tests { for (path, addr) in jit_trace_fnaddrs() { by_addr.entry(addr).or_default().push(path); } + // Collect every colliding address before failing. Asserting inside + // the loop reports whichever collision the hash order reached first + // and hides the rest, so each repair looks complete and the next run + // names a different pair. + let mut collisions: Vec = Vec::new(); for (addr, paths) in &by_addr { let leaves: std::collections::BTreeSet<&str> = paths .iter() .map(|p| p.rsplit("::").next().unwrap_or(p)) .collect(); - assert_eq!( - leaves.len(), - 1, - "fnaddr {addr:#x} is claimed by unrelated functions {paths:?}", - ); + if leaves.len() > 1 { + collisions.push(format!("{addr:#x} {leaves:?}")); + } } + collisions.sort(); + assert!( + collisions.is_empty(), + "{} fnaddr(s) claimed by unrelated functions:\n {}", + collisions.len(), + collisions.join("\n "), + ); } #[test] diff --git a/pyre/pyre-interpreter/src/pyopcode.rs b/pyre/pyre-interpreter/src/pyopcode.rs index 9abfb9c6b1c..f6fbff6c35c 100644 --- a/pyre/pyre-interpreter/src/pyopcode.rs +++ b/pyre/pyre-interpreter/src/pyopcode.rs @@ -1818,6 +1818,7 @@ pub fn label_arg_to_usize( delta: crate::bytecode::Arg, op_arg: OpArg, ) -> usize { + keep_fnaddr_distinct(2); delta.get(op_arg).as_usize() } @@ -1844,6 +1845,7 @@ pub fn jump_target_forward_from_oparg( next_instr: usize, op_arg: OpArg, ) -> usize { + keep_fnaddr_distinct(3); jump_target_forward(&code.instructions, next_instr, op_arg_as_usize(op_arg)) } @@ -1919,6 +1921,25 @@ pub fn convert_value_arg( conv.get(op_arg) } +/// Materialise `tag` behind an optimisation barrier so the caller's machine +/// code carries an immediate no sibling shares. +/// +/// The decode helpers below differ only in the phantom type of their `Arg` +/// parameter, so several of them compile to byte-identical bodies. Each is a +/// residual-call target whose address `jit_fnaddr.rs` registers, and +/// `runtime_fnaddr_patch` re-pairs a build-time address with the runtime one +/// by that address alone — two functions folded onto a single address make +/// that pairing ambiguous and can send one callee's call to the other. A +/// linker that folds identical code (MSVC `/OPT:ICF`, on by default) is what +/// performs the fold, and which pair it picks moves with unrelated layout +/// changes, so the bodies have to differ by construction rather than by luck. +/// `drain_list_append` keeps its `#[inline(never)]` forwarding call for the +/// same reason; these have no callee to forward to, so they carry a datum. +#[inline(always)] +fn keep_fnaddr_distinct(tag: u32) { + std::hint::black_box(tag); +} + /// Decode `LOAD_SPECIAL`'s enum oparg behind a first-party helper. #[inline] #[majit_macros::dont_look_inside] @@ -1926,6 +1947,7 @@ pub fn special_method_arg( method: crate::bytecode::Arg, op_arg: OpArg, ) -> SpecialMethod { + keep_fnaddr_distinct(1); method.get(op_arg) } diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index 8bf560374c3..5a405551cb3 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -1907,6 +1907,19 @@ static SPECIALISED_TUPLE_OO_DESCR_GROUP: LazyLock = LazyLo ) }); +/// The `[capacity][items…]` header shared by every list/tuple backing block. +/// +/// The `0` type id is load-bearing, not a slot waiting to be filled: +/// `items_block_capacity_descr()` is the capacity read for all three list +/// strategies, and their blocks carry three different runtime tids +/// (`GC_INT_ARRAY_GC_TYPE_ID`, `GC_FLOAT_ARRAY_GC_TYPE_ID`, +/// `PY_OBJECT_ARRAY_GC_TYPE_ID` — see the three arms of +/// `helpers::emit_promote_empty_list_inline`). One descr fronting three tids +/// can name none of them, so `StructPtrInfo::make_guards` (`optimizeopt/info.rs`) +/// declines the short-preamble entry rather than pin a layout that holds for +/// one strategy and not the other two; `unroll_free_retry_rescued` counts the +/// unrolled attempt that costs. Stamping any single tid here makes the guard +/// false for the other two block kinds. static ITEMS_BLOCK_DESCR_GROUP: LazyLock = LazyLock::new(|| { build_object_descr_group_with_def_path( pyre_object::object_array::ITEMS_BLOCK_ITEMS_OFFSET, diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs index ad080ad29cc..9fafffc2d00 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -570,6 +570,140 @@ pub(crate) fn exception_string_override_straight_line(body_code: &[u8]) -> bool true } +/// Whether descending into this jitcode body can reach a residual call whose +/// funcbox is an un-lowered helper's symbolic hash. +/// +/// [`try_execute_residual_call_via_executor`] refuses to record such a call +/// while inlining a sub-jitcode — the hash is not a code address, so a +/// compiled trace would branch to it — and raises +/// `OrthodoxSubWalkTraceUnsupported` at that call. By then the descent has +/// executed every earlier op for real, including residual calls that advance +/// a generator's internal state, and the abort resumes the enclosing frame at +/// its own `CALL`. The Python call therefore runs a second time and the first +/// result is discarded: `random.random()` in a loop advances the Mersenne +/// Twister once per aborted descent without producing a value for it +/// (`gen.random()` drew 4003 times for 4000 appends). +/// +/// The funcbox is a jitcode constant, so whether a body holds such a call is a +/// static property of the body. Answering it before the descent starts turns +/// the mid-descent abort into an ordinary residual call, which applies the +/// effect exactly once. +/// +/// The scan follows `inline_call_*` into the callee bodies the descent would +/// enter, because the abort propagates from any depth. A body already on the +/// scan stack is a cycle and answers `false`: the occurrence that opened it +/// decides. +/// Whether descending into this jitcode body can reach a residual call whose +/// funcbox is an un-lowered helper's symbolic hash. +/// +/// [`try_execute_residual_call_via_executor`] refuses to record such a call +/// while inlining a sub-jitcode — the hash is not a code address, so a +/// compiled trace would branch to it — and raises +/// `OrthodoxSubWalkTraceUnsupported` at that call. By then the descent has +/// executed every earlier op for real, including residual calls that advance +/// a generator's internal state, and the abort resumes the enclosing frame at +/// its own `CALL`. The Python call therefore runs a second time and the first +/// result is discarded: `random.random()` in a loop advances the Mersenne +/// Twister once per aborted descent without producing a value for it +/// (`gen.random()` drew 4003 times for 4000 appends). +/// +/// The funcbox is a jitcode constant, so whether a body holds such a call is a +/// static property of the body. Answering it before the descent starts turns +/// the mid-descent abort into an ordinary residual call, which applies the +/// effect exactly once. +/// +/// The scan follows `inline_call_*` into the callee bodies the descent would +/// enter, because the abort propagates from any depth. A body already on the +/// scan stack is a cycle and answers `false`: the occurrence that opened it +/// decides. +fn descent_reaches_unlowered_helper_call(jitcode_index: usize) -> bool { + thread_local! { + static VERDICTS: std::cell::RefCell> = + std::cell::RefCell::new(std::collections::HashMap::new()); + } + if let Some(cached) = VERDICTS.with(|v| v.borrow().get(&jitcode_index).copied()) { + return cached; + } + let verdict = scan_body_for_unlowered_helper_call(jitcode_index, &mut Vec::new()); + VERDICTS.with(|v| v.borrow_mut().insert(jitcode_index, verdict)); + verdict +} + +/// Recursive worker of [`descent_reaches_unlowered_helper_call`]. `seen` is +/// the stack of jitcode indices currently being scanned. +fn scan_body_for_unlowered_helper_call(jitcode_index: usize, seen: &mut Vec) -> bool { + if seen.contains(&jitcode_index) { + return false; + } + let Some(body) = crate::jitcode_dispatch::sub_jitcode_body_by_index(jitcode_index) else { + // No installed body means no descent, so there is nothing to answer + // for; the caller declines on the same lookup. + return false; + }; + seen.push(jitcode_index); + let descrs = crate::jitcode_runtime::descr_ref_table(); + // What each Int-bank slot is known to hold. `allocate_callee_register_banks` + // pre-fills the slots at and above `num_regs_i` from `constants_i`; the + // rest start unknown and are tracked below, because the codewriter loads a + // call's funcbox into an ordinary register before the call reads it. + let mut known_i = vec![None; body.num_regs_i + body.constants_i.len()]; + for (slot, &value) in body.constants_i.iter().enumerate() { + known_i[body.num_regs_i + slot] = Some(value); + } + let mut pc = 0usize; + let mut verdict = false; + while pc < body.code.len() { + let Some(d) = crate::jitcode_runtime::decode_op_at(body.code, pc) else { + break; + }; + if d.opname.starts_with("residual_call") { + // Every `residual_call_*` argcode string opens with the `i` funcbox + // operand, so it is the byte right after the opcode. + let funcbox = body.code.get(d.pc + 1).copied().unwrap_or(0) as usize; + if let Some(Some(fnaddr)) = known_i.get(funcbox) + && majit_translate::codewriter::call::is_symbolic_fnaddr(*fnaddr) + { + verdict = true; + break; + } + } else if d.opname.starts_with("inline_call") { + // `inline_call_*` opens with the `d` descr operand, a two-byte + // index into the descriptor pool, naming the callee JitCode. + let descr_index = body.code.get(d.pc + 1).copied().unwrap_or(0) as usize + | ((body.code.get(d.pc + 2).copied().unwrap_or(0) as usize) << 8); + if let Some(callee) = descrs + .at(descr_index) + .and_then(|descr| descr.as_jitcode_descr().map(|jc| jc.jitcode_index())) + && scan_body_for_unlowered_helper_call(callee, seen) + { + verdict = true; + break; + } + } + // An op writes at most one register, named by the argcode suffix after + // `>` and encoded as the instruction's last byte. Only `int_copy/i>i` + // carries a known value forward; every other Int-bank write makes its + // destination unknown again. + if d.argcodes + .split_once('>') + .is_some_and(|(_, dst)| dst == "i") + && let Some(&dst) = body.code.get(d.next_pc.wrapping_sub(1)) + { + let carried = (d.key == "int_copy/i>i") + .then(|| body.code.get(d.pc + 1)) + .flatten() + .and_then(|&src| known_i.get(src as usize).copied()) + .flatten(); + if let Some(slot) = known_i.get_mut(dst as usize) { + *slot = carried; + } + } + pc = d.next_pc; + } + seen.pop(); + verdict +} + /// Whether an exception string-override body issues a nested Python call. The /// bounded string-override route inlines the override as a leaf; a nested call /// (`CallFn` residual, or a `cond_call`/`call_assembler`/`inline_call`) forces a @@ -2411,6 +2545,10 @@ pub(crate) fn try_walker_inline_builtin_call( if body.num_regs_r < 1 { return Ok(None); } + if descent_reaches_unlowered_helper_call(jitcode.index()) { + builtin_inline_decline!("un-lowered helper call in body", fnaddr); + return Ok(None); + } let nested_helper = ctx.fbw_mode.inline_subwalk; // A translated helper is transparent to blackhole execution, but its // Python caller is not, so a nested inline preserves that caller at the diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs index 9fe2273855e..eb5b6f34c65 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs @@ -1403,37 +1403,55 @@ pub(crate) fn concrete_ref_for_color( /// Resolve a recorded box to the concrete ref it stands for. /// -/// A NULL `Ref` is an UNRESOLVED box, not a slot whose value is null: a box -/// carries a concrete only once the walk materialized one, and an operand the -/// walk still holds symbolically — a deferred `LOAD_ATTR name + NULL|self` -/// pair is the shape that reaches here — reads back `Ref(0)`. Answer -/// unresolved, as [`concrete_ref_for_color`] and the virtualizable-shadow -/// fallback in [`collect_call_stack_overrides`] already do, so the slot stays -/// ABSENT and the outer-call flush declines to the legacy replay. Publishing -/// it instead writes a null into a live operand-stack slot, and the -/// interpreter resumed there dispatches through it: `out.append(f(i))` in an -/// `except` handler faulted in `classify_callable` on the method-load slot -/// this used to hand back as a resolved value. The one operand whose correct -/// value IS null — a `CALL`'s own `null_or_self` — is named and supplied -/// separately by that function. +/// `GcRef::NO_CONCRETE` is the sentinel for "no runtime value is known for +/// this box" (`value.rs:51-59`), and `heapcache_ops` stamps it on a box whose +/// load could not be replayed, so it never answers a slot. A stamped NULL is +/// judged by what carries it: +/// +/// * An input argument is bound from the real frame when the loop is entered +/// and rebound from the fail args at every guard failure, so a NULL stamped +/// on one is the operand's value, the same way RPython's +/// `MIFrame.registers_r` entry for a loop input box holds it. +/// `LOAD_FAST_AND_CLEAR` saves an unbound local as exactly that null and +/// leaves it on the operand stack below an inlined comprehension for the +/// whole loop, so answering unresolved left every paused-caller image built +/// over that stack one slot short and `capture_root_parent_resume_stack` +/// declined it. +/// * Any other box may read back `Ref(0)` while the walk still holds the +/// operand symbolically — a deferred `LOAD_ATTR name + NULL|self` pair is +/// the shape that reaches here. Answer unresolved, as +/// [`concrete_ref_for_color`] and the virtualizable-shadow fallback in +/// [`collect_call_stack_overrides`] already do, so the slot stays ABSENT and +/// the outer-call flush declines to the legacy replay. Publishing it +/// instead writes a null into a live operand-stack slot, and the interpreter +/// resumed there dispatches through it: `out.append(f(i))` in an `except` +/// handler faulted in `classify_callable` on the method-load slot this used +/// to hand back as a resolved value. +/// +/// The one operand whose correct value IS null whatever carries it — a +/// `CALL`'s own `null_or_self` — is named and supplied separately by +/// [`collect_call_stack_overrides`]. pub(crate) fn concrete_ref_for_opref( ctx: &WalkContext<'_, '_, Sym>, opref: OpRef, ) -> Option { // RPython history.py:361 defines CONST_NULL as a real // `ConstPtr(lltype.nullptr(llmemory.GCREF.TO))`. Preserve that typed - // constant before consulting the runtime-concrete table. A symbolic Ref - // operation whose concrete lookup happens to return Ref(NULL) is still an - // unresolved box and must remain absent, but an inline ConstPtr(NULL) is - // positive proof that this operand-stack slot contains Python's call - // sentinel. LOAD_SPECIAL records exactly that constant for its - // `self_or_null` half, including the `__exit__` pair retained below a + // constant before consulting the runtime-concrete table: an inline + // ConstPtr(NULL) is positive proof that this operand-stack slot contains + // Python's call sentinel. LOAD_SPECIAL records exactly that constant for + // its `self_or_null` half, including the `__exit__` pair retained below a // nested CALL inside a `with` body. if let OpRef::ConstPtr(value) = opref { return Some(value.as_usize() as pyre_object::PyObjectRef); } + let null_is_a_value = matches!(opref, OpRef::InputArgRef(_)); match ctx.trace_ctx.concrete_of_opref(opref) { - Some(Value::Ref(r)) if !r.is_null() => Some(r.as_usize() as pyre_object::PyObjectRef), + Some(Value::Ref(r)) + if r != majit_ir::GcRef::NO_CONCRETE && (null_is_a_value || !r.is_null()) => + { + Some(r.as_usize() as pyre_object::PyObjectRef) + } _ => None, } } diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 8cd5f2d9aa2..c23cec7f7ed 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -1102,6 +1102,111 @@ pub fn publish_last_instr_at_live_marker( }); } +/// Drop every Ref register the `-live-` marker at `marker_pc` does not name. +/// +/// `cleanup_registers` (`blackhole.py:385`) clears `registers_r` "to avoid +/// keeping references alive", but it runs from `release_interp` +/// (`blackhole.py:253`) — after the run, not during it. Inside a run the only +/// thing that ends a register's hold on its object is a later write to the +/// same register, which `rpython/tool/algo/regalloc.py:28-75` makes near-certain +/// by colouring on liveranges and reusing a dead value's colour. This +/// codewriter walks one Python function per jitcode rather than one giant +/// `dispatch_bytecode` graph, so a colour whose only definition sits inside a +/// loop is never redefined afterwards: the loop's iterable stays in its +/// register for the whole remainder of the frame, and `walk_bh_regs` roots the +/// bank unconditionally. A resumed frame that then calls `gc.collect()` keeps +/// the iterable and everything it reaches. +/// +/// The marker's Ref set is a sound bound to clear against. It is the SSA-live +/// set — "written before and read afterwards" — computed by the backward pass +/// in `liveness.rs` (`liveness.py:5-12`), so a register missing from it is +/// re-written before any read. `filter_liveness_in_place` only ever adds to it +/// (the FOR_ITER frame-live re-add, the portal reds, a residual call's result +/// register), and a folded marker carries the union over its group's PCs. +/// +/// The clear stops at `num_regs_r()`: the slots above it are the constants +/// window `copy_constants` preloads, which `cleanup_registers` also leaves +/// alone. Anything unresolvable — a pc that anchors no marker, a liveness +/// table that does not cover the offset, a length that cannot describe this +/// bank — clears nothing, which is exactly the behaviour without this hook. +fn clear_dead_ref_registers_at_live_marker( + bh: &mut majit_metainterp::blackhole::BlackholeInterpreter, + marker_pc: usize, +) { + let num_regs_r = bh.jitcode.num_regs_r().min(bh.registers_r.len()); + if num_regs_r == 0 { + return; + } + // `get_live_vars_info` panics on a pc that anchors no `-live-`; the + // blackhole reaches this hook only from `handler_live`, but the marker + // still has to resolve against this jitcode's own code stream. + if !bh.jitcode.can_decode_live_vars(marker_pc, bh.op_live) { + return; + } + let info = bh.jitcode.get_live_vars_info(marker_pc, bh.op_live); + // Read the pool through the store rather than `liveness_info_snapshot`: + // this runs once per replayed instruction, and that accessor re-runs + // `ensure_finish_setup` and takes a reference count each time. A borrow + // already held (a reentrant walker path) declines, the same way the + // `last_instr` publish above does, instead of panicking. + METAINTERP_SD.with(|r| { + let Ok(sd) = r.try_borrow() else { + return; + }; + let all_liveness: &[u8] = &sd.liveness_info; + // `enumerate_vars` indexes the three length bytes unguarded. + if info + 3 > all_liveness.len() { + return; + } + // A live set cannot name more Ref registers than the bank holds; a + // wider count means the offset is not describing this jitcode. + let length_r = all_liveness[info + 1] as usize; + if length_r > num_regs_r { + return; + } + // An empty Ref set is not a claim that nothing is live. A marker whose + // Python PCs are all unreachable is emitted with no registers at all + // (`filter_liveness_in_place`'s `any_reachable` arm), while a reachable + // portal marker always names at least the `frame` red + // (`interp_jit.py:67 reds = ['frame', 'ec']`). Decline rather than + // clear the whole bank on the one shape that cannot be told apart. + if length_r == 0 { + return; + } + // Register indices are single bytes (`assembler.py:127-138` asserts + // `0 <= val < 256`), so the live set fits a fixed 256-bit mask and the + // hook allocates nothing. + let mut live_r: [u64; 4] = [0; 4]; + majit_translate::codewriter::jitcode::enumerate_vars( + info, + all_liveness, + |_| {}, + |index| { + let index = index as usize; + if index < 256 { + live_r[index / 64] |= 1u64 << (index % 64); + } + }, + |_| {}, + ); + for index in 0..num_regs_r.min(256) { + if live_r[index / 64] & (1u64 << (index % 64)) == 0 { + bh.registers_r[index] = 0; + } + } + }); +} + +/// `-live-` marker hook: stamp the frame's `last_instr`, then drop the Ref +/// registers the marker does not name. +pub fn on_live_marker( + bh: &mut majit_metainterp::blackhole::BlackholeInterpreter, + marker_pc: usize, +) { + publish_last_instr_at_live_marker(bh, marker_pc); + clear_dead_ref_registers_at_live_marker(bh, marker_pc); +} + /// Whether a JitCode exception exit came from the Python bare-reraise /// instruction path. `RAISE_VARARGS 0` and `RERAISE` both use /// RaiseWithExplicitTraceback and skip record_application_traceback. diff --git a/pyre/pyre-jit/src/call_jit.rs b/pyre/pyre-jit/src/call_jit.rs index 9121503d12a..a28956a94c2 100644 --- a/pyre/pyre-jit/src/call_jit.rs +++ b/pyre/pyre-jit/src/call_jit.rs @@ -2055,6 +2055,7 @@ fn jit_blackhole_resume_from_guard( guard_exc, false, // CALL_ASSEMBLER portal is jd0 (virtualizable) all_virtuals, + None, // `raw_deadframe` is rooted only by the copy made inside ); return handle_blackhole_result(result, actual_green_key); } @@ -2351,6 +2352,12 @@ pub fn blackhole_resume_via_rd_numb( // materialized, when this resume is the GUARD_NOT_FORCED that follows a // force. `None` for every other guard. all_virtuals: Option<(Vec, Vec)>, + // The caller's own rooting of the `deadframe` slice it passed, when it has + // one. It covers the same window as `deadframe_roots` below and must end at + // the same point, so the caller hands ownership over instead of holding it + // across this call: a caller-side scope would stay registered for the whole + // forward run. + caller_deadframe_roots: Option, ) -> BlackholeResult { // Same window as `handle_fail`, for every blackhole resume including the // CALL_ASSEMBLER caller: the decode below rebuilds virtuals through the @@ -2531,6 +2538,7 @@ pub fn blackhole_resume_via_rd_numb( // `deadframe`'s live range at `_prepare_resume_from_failure`, before // `_run_forever`, for the same reason. drop(deadframe_roots); + drop(caller_deadframe_roots); // resume.py:1332-1343 builds the caller chain (`nextblackholeinterp`) // but does not set the virtualizable-info handle on each frame. pyre // stores the vinfo per-`BlackholeInterpreter` (RPython reads it from diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index b59e83e131a..e00b1958824 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -4935,10 +4935,9 @@ fn build_jit_driver_pair() -> JitDriverPair { // source-level one upstream and rides in the jitcode; this codewriter // unrolls the bytecode, so the same store would need one int pool // constant per instruction. The blackhole publishes it at the `-live-` - // marker instead. - majit_metainterp::blackhole::register_live_marker_hook( - pyre_jit_trace::state::publish_last_instr_at_live_marker, - ); + // marker instead, and clears the Ref registers the marker leaves out — + // the marker is the one program point that names the live set. + majit_metainterp::blackhole::register_live_marker_hook(pyre_jit_trace::state::on_live_marker); // warmspot.py:1039 handle_jitexception_from_blackhole parity: // portal_runner is called when ContinueRunningNormally is raised // at a recursive portal level during blackhole execution. @@ -9573,8 +9572,13 @@ pub(crate) fn resume_in_blackhole_from_exit_layout( novable: bool, ) -> crate::call_jit::BlackholeResult { // Same deadframe rooting as `handle_fail`: `decode_ref`'s TAGBOX arm reads - // these slots after the resume construction has already allocated. - let _deadframe_roots = unsafe { + // these slots after the resume construction has already allocated. The + // scope is handed to `blackhole_resume_via_rd_numb` below rather than held + // here: that call runs the resumed frame forward to completion, and a slot + // still registered pins whatever object the guard left in it for the whole + // run. `blackhole.py:1782-1796 resume_in_blackhole` ends `deadframe`'s + // live range at `_prepare_resume_from_failure`, before `_run_forever`. + let deadframe_roots = unsafe { majit_metainterp::resume::DeadFrameRefRoots::enter(raw_values, |index| { exit_layout.exit_types.get(index) == Some(&majit_ir::Type::Ref) || exit_layout.gc_ref_slots.contains(&index) @@ -9625,6 +9629,7 @@ pub(crate) fn resume_in_blackhole_from_exit_layout( guard_exc, novable, all_virtuals, + Some(deadframe_roots), ); if majit_metainterp::majit_log_enabled() { eprintln!( diff --git a/pyre/pyre-jit/src/jit/codewriter.rs b/pyre/pyre-jit/src/jit/codewriter.rs index 1c129703a66..d7dc997b4a5 100644 --- a/pyre/pyre-jit/src/jit/codewriter.rs +++ b/pyre/pyre-jit/src/jit/codewriter.rs @@ -11199,9 +11199,13 @@ impl CodeWriter { } Instruction::PopIter => { - // pop iterator: net -1 - current_depth = current_depth.saturating_sub(1); - emit_vsd!(current_depth, py_pc); + // pop iterator: net -1. `PyFrame::pop` + // (pyframe.rs:3002-3010, `popvalue_maybe_none` + // pyframe.py:411-417) writes NULL over the slot + // before it lowers `valuestackdepth`, so the pop + // goes through `emit_popvalue_ref!` like every + // other one rather than moving the depth alone. + let _ = emit_popvalue_ref!(current_depth, py_pc); } // BinarySlice: obj[start:stop] — pops 3 (stop, start, obj), pushes 1 (result). diff --git a/pyre/pyre-object/src/dict_eq_hook.rs b/pyre/pyre-object/src/dict_eq_hook.rs index 04146831e4d..6e4b0feb5e2 100644 --- a/pyre/pyre-object/src/dict_eq_hook.rs +++ b/pyre/pyre-object/src/dict_eq_hook.rs @@ -412,6 +412,15 @@ pub unsafe fn try_compares_by_identity(w_type: PyObjectRef) -> Option { /// the registered fnaddr. #[majit_macros::dont_look_inside] pub extern "C" fn has_compares_by_identity_hook() -> bool { + // Materialise a datum no sibling carries, behind an optimisation barrier. + // This predicate and [`has_eq_w_hook`] are both registered residual-call + // targets (`jit_fnaddr.rs`), and `runtime_fnaddr_patch` re-pairs a + // build-time address with the runtime one by that address alone, so two + // bodies folded onto one address make the pairing ambiguous. Which pair a + // folding linker picks moves with unrelated layout changes, so the bodies + // must differ by construction; `drain_list_append` keeps its + // `#[inline(never)]` forwarding call for the same reason. + std::hint::black_box(4u32); COMPARES_BY_IDENTITY_HOOK.with(|cell| cell.get().is_some()) }