cel: complete translator cutover and deadframe integration - #1320
Open
youknowone wants to merge 9 commits into
Open
cel: complete translator cutover and deadframe integration#1320youknowone wants to merge 9 commits into
youknowone wants to merge 9 commits into
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Limit 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. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (43)
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 |
youknowone
marked this pull request as ready for review
August 18, 2026 10:47
![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: 6ed6855157
ℹ️ 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".
| entry[backend] = status | ||
| entry.setdefault("provenance", {})[backend] = dict(cell_axis) | ||
| continue | ||
| if status == shared: |
There was a problem hiding this comment.
When --update-baseline remeasures an existing cell and its status still matches the shared verdict, this branch exits without writing cell_axis. Consequently, almost every pre-existing baseline entry remains permanently provenance-free—or retains an obsolete axis—even after being freshly measured, so axis_drift_report continues reporting it as legacy/drifted. Update the shared cell's provenance here; the analogous unchanged-overlay case below also needs its provenance refreshed.
Useful? React with 👍 / 👎.
youknowone
force-pushed
the
majit-runtime-followup
branch
from
August 19, 2026 05:09
81b54fc to
5226c72
Compare
The census test's `JitDriverSpec` literal lists every field explicitly, so it also lists `green_kinds` and `red_kinds`. This driver declares no greens and no reds, so the vectors parallel to them are empty -- which is also the value that leaves the codewriter's kind check disabled. Assisted-by: Claude
The module doc was rewritten and the rewrite dropped the reason the structure has to stay per-op: an earlier version of this machine fused raw_load+add+i++ into one opcode, the clean interpreter became a one-op memory-bandwidth-bound loop with no dispatch left to eliminate, and the JIT read 0.62x. That figure measured the kernel, not the float JIT. `0.62x`, the kernel-versus-JIT attribution, and the `column` probe precedent appear nowhere else in the tree, so nothing carried them after the rewrite. What remained states that the opcodes are separate but not that they must be, which is the half a future reader needs: fusing them back reproduces the 0.62x and invites the same misreading. The rewrite is kept. This adds a paragraph after it rather than reverting to the older wording, and leaves the reworded program list alone -- the op counts there already carry what the dropped `(memory-bound ref)` / `(compute-bound)` annotations said. Doc comments only. Not covered by the rustdoc ratchet, which lists majit-rlib and majit-trace, and carries no rpython/pypy citation for check-citation-drift to validate. Assisted-by: Claude
`majit/examples/dualtape`'s shape-gate doc kept the claim that an op count cannot distinguish body shapes, but not the values it collides on (`1` an empty dispatch, `5` a segmented runaway) nor the record that the doc asserted the count was sufficient until the shape gate refuted it. The `pyre/gate-triage.md` half of this change is gone: it restored detail into the timeout caution in §1b, and `a6684cd6641` retired that section. Assisted-by: Claude
The documented command was `cargo test --all --features dynasm`. With default features on, the workspace's example interpreters default to `cranelift` and supply it to the one shared `majit-metainterp`, while `pyre-jit` compiles in its dynasm registrations — so that command builds a binary that dispatches through one backend and holds the other's hooks. The stated justification, that without the flag `majit-metainterp` emits `compile_error!`, describes a failure that configuration cannot produce: the examples already supply a backend, so the no-backend arm is never reached. Replaced with `--no-default-features --features dynasm` and a reason that matches what the flags do. Assisted-by: Claude
`scripts/check-new-line-citations.py` reads what a change adds and rejects
an `upstream.py:LINE` inside a `//` comment. Of the fourteen it reports for
this branch, eleven are lines already carrying that citation in the base
copy of the same file, which this branch only reindents; three were written
here.
Each of the three already named, or sits next to, a symbol the number was
redundant with: `rclass.py:162-165` quotes `OBJECT = GcStruct('object', ...`,
which occurs once in that file; `descroperation.py:706-712` is `binop_impl`,
now named because the quote beside it occurs twice; `policy.py:48-84` already
spelled `look_inside_graph`.
Assisted-by: Claude
…mbedders Two things every interpreter built on this framework writes for itself. `intrinsics` holds the untraced bodies of the raw-memory, bitcast and unsigned-arithmetic intrinsics the `#[jit_interp]` lowerer rewrites while tracing. The lowerer matches the LAST segment of the call expression's path and never looks at where the function is defined, so an imported name is recognized exactly as a local `fn` of the same name is. Two tests in `lower_value.rs` say so: `an_imported_intrinsic_lowers_like_a_local_one` lowers the bare and the fully-qualified spelling of a load, a store and an unsigned compare and asserts the two are equal, and `only_the_last_path_segment_selects_an_intrinsic` is its control -- without it, "the qualified spelling lowered the same" would also hold of a lowerer that ignored the path and matched on argument shape alone. A spelling that failed to be recognized would not error. It would lower as a residual call, which is a worse graph and not a broken one, so the property needs a test rather than a reading. `embed::Census` holds the counters an embedder tallies off the four driver callbacks, installs itself onto a driver, and reads the abort-reason counters as a DELTA. The delta is the reason the type exists: the counters are process-global, so "since process start" answers a different question from "during this run", and two overlapping consumers of a resetting read each see a fraction of the truth. Its serializing lock is a process-global `Mutex` and not a `thread_local!`, which would serialize nothing. `examples/tinyframe` is ported onto both as the proof, losing 93 lines of its own copies and gaining 43. Neither module has a same-named upstream file, so both are declared in the local-boundary list in `lib.rs` with the reason: RPython needs no intrinsics module because it lowers `rawstorage.py`, `longlong2float.py` and `rarithmetic.py` at rtype time from the one definition each already has, and the census's upstream equivalent is split between `warmspot.py`'s test harness and `jitprof.py`, neither of which is a surface an embedder calls. Assisted-by: Claude
`assert_no_degraded_dispatch_arms` asserts the degraded set is empty. A machine with a known lowering gap has a non-empty set, so the assertion fails on it from the first call and the twelve examples that filter `degraded_dispatch_arms()` by hand all wrote their own check instead. `embed::assert_degraded_dispatch_arms` takes the expected set, so a known gap is expressible: a new name means an arm stopped lowering, a missing name means the pinned gap closed. `assert_no_degraded_dispatch_arms` is now its `&[]` case. `assert_degraded_dispatch_arm_causes` pins each arm's refusal family and `assert_degraded_dispatch_arm_reason_contains` pins the source it refuses on; `degraded_dispatch_arm_names`, `degraded_dispatch_arm_causes` and `degraded_dispatch_arm_reason` read the registry without asserting. Both assertions look up the arm census first, so an uninstalled portal reports that rather than reading as a closed gap. spcount, tla and tlr drop their hand-rolled compile counters, shape flags and probe mutex for `embed::Census` and the new assertion. Assisted-by: Claude
…xes back to it `seed_bridge_virtualizable_boxes` rebuilds the virtualizable shadow out of a guard's fail args and stopped there, on the stated premise that every `[.. ; virt]` state field is `RustVec`-stored and so carved out of `synchronize_virtualizable`'s write-back. A state field holding a pointer to a `[length][payload…]` block is not carved out, and for one of those the live state struct kept the values the compiled run was entered with while the shadow held the values at the guard. Port `rebuild_state_after_failure`'s trailing `synchronize_virtualizable()` for a vinfo with no `vable_token`. Also correct the freeze premise in the `writeback_virt_array_state_fields` comment, which named the same carve-out. Assisted-by: Claude
…a terminal return A guard-resume bridge whose walk reaches the interpreted function's return sets `single_pass_finish` and reports `usize::MAX` as its position. The `jit_merge_point!` expansion reads that flag and `break`s; the `can_enter_jit!` expansion did not, and assigned the position to the loop's `pc`, which indexes out of bounds in a dispatch loop that is not bottom-tested on its program length. Emit the same `break` at the back edge, and leave the flag standing in `bridge_from_guard_resume_position` for the caller to consume. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 608c7164fc
ℹ️ 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".
| let arms = assert_portal_installed(interp, expected.is_empty()); | ||
| let degraded = degraded_dispatch_arm_names(interp); | ||
| assert!( | ||
| degraded == expected, |
There was a problem hiding this comment.
When a caller pins two or more correct arms in portal/source order—for example, ["ROLL", "ALLOCATE"]—degraded_dispatch_arm_names sorts the actual names alphabetically but this comparison leaves expected untouched, so the documented set assertion panics even though the sets are identical. Normalize the expected names before comparison; assert_degraded_dispatch_arm_causes has the same order-sensitive comparison and should be normalized as well.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 608c716). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Why
This is the CEL-specific remainder after extracting the reusable majit work into the base branch. Keeping it as one commit directly above
majit-runtime-followupmakes the merge order explicit and allows a linear merge after the base PR lands.Stack
majit-runtime-followup(must land first)Checks
majit-rlib,pyre-object,pyre-interpreter, andpyre-jitcargo check --features dynasmcargo test --features dynasmcargo test --all --no-default-features --features dynasmcargo test --release -p cel --no-default-features --features dynasm(5/5)python3 scripts/check-citation-drift.py --self-testRUSTDOCFLAGS='-D warnings' cargo doc --no-deps --locked -p majit-rlib -p majit-tracepython3 pyre/check.py --backend dynasm --no-synthetic(dynasm 10/10;fib_recursivecounters were unstable and non-gated on this run)