Retire five spec folds the orthodox walk already covers - #1972
Merged
Merged
Conversation
Each row was suppressed with PYRE_FBW_NO_SPECIALIZE=<label> over every pyre/bench/synth fixture that consults it; stdout and [jit-stats] matched the unsuppressed run, so the row, its gate and its try_walker_specialize_* body are removed and the label is dropped from fixture spec-folds headers: - import_cached: cached `import` / `from math import pi` CallR fold. - object_class_attr: `obj.__class__` load_attr fold. - format_with_spec: format(value, spec) CallR fold. - load_super_attr_descent: consulted 1, fired 0. - builtin_locals_trace_limit_cut: the census row around the `locals()` expansion's trace_limit cut; the cut itself stays. builtin_fold2, exception_reduce, setslice and subscr_tuple_slice2 matched jit-stats when suppressed but ran several times slower, so they stay. spec_folds rows 93 -> 88, try_walker_specialize_ 80 -> 78. Assisted-by: Grok 4.7 Assisted-by: Claude Opus 5.5
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (5)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe change removes several JIT specialization paths and their census entries, adjusts benchmark fold declarations, and adds exact-string and containment helpers. It also adds or relocates descriptions for existing specialization paths. ChangesJIT specialization updates
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to No actionable issue remains from the reviewed changes; merge after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the folds at dawn 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: d65d16a50b
ℹ️ 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".
Comment on lines
7422
to
7424
| { | ||
| return Ok(inlined); | ||
| } |
There was a problem hiding this comment.
For hot exact-builtin formatting with a spec outside the decimal-int subset, such as f"{i:x}", f"{i:.2f}", or f"{s:>5}", FormatWithSpecInt declines and try_walker_inline_format explicitly declines builtin __format__ methods. Removing the subsequent fallback therefore sends these cases to the CallFlavor::MayForce residual instead of the prior non-forcing jit_format_w CallR, reintroducing a potentially forcing call on every iteration; str_fstring.py only exercises 05d, so it does not cover this regression. Replace the removed path with a generated descent for these builtin forms rather than residualizing them.
AGENTS.md reference: AGENTS.md:L67-L73
Useful? React with 👍 / 👎.
Assisted-by: Claude Opus 5.5
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 1fdbf0b). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
Owner
Author
|
CI at 1fdbf0b: both red jobs fail on main 17c8884 with the same output.
— commented by Claude Opus 5.5 |
youknowone
added a commit
that referenced
this pull request
Sep 26, 2026
73 spec_folds! rows (4 descent), 66 try_walker_specialize_ functions (62/1/3), 18 try_walker_orthodox_ functions, 559 synth fixtures, 50 distinct rows named by 54 spec-folds= headers. spec_folds! rows 73 -> 73, fn try_walker_specialize_ 66 -> 66. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Sep 26, 2026
73 spec_folds! rows (4 descent), 66 try_walker_specialize_ functions (62/1/3), 18 try_walker_orthodox_ functions, 559 synth fixtures, 50 distinct rows named by 54 spec-folds= headers. spec_folds! rows 73 -> 73, fn try_walker_specialize_ 66 -> 66. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Sep 27, 2026
73 spec_folds! rows (4 descent), 66 try_walker_specialize_ functions (62/1/3), 18 try_walker_orthodox_ functions, 559 synth fixtures, 50 distinct rows named by 54 spec-folds= headers. spec_folds! rows 73 -> 73, fn try_walker_specialize_ 66 -> 66. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Sep 27, 2026
…h gateways; retire their folds (#1964) * math: descend math.sqrt through an interp2app gateway, drop the sqrt fold `math.sqrt` is now installed as `__majit_wrap_math_sqrt` (fixed arity 1), the interp_math.py `math1(space, math.sqrt, w_x)` / `_get_double` body with ll_math.py `ll_math_sqrt`'s branches: exact float or machine int, `x >= 0.0`, `isfinite(x)` -> `_float_sqrt(x)` (`sqrt_nonneg`, oopspec `math.sqrt_nonneg`); +inf returns the operand; every other shape goes to the `dont_look_inside` `sqrt_slow`, which runs the old body behind the declared-arity check. The generic builtin-call descent walks the gateway, so `try_walker_specialize_math_sqrt`, `try_walker_orthodox_float_sqrt`, `FLOAT_SQRT_DESCENT`, `MathFloatDomain::NonNegativeFinite` and `is_math_sqrt_function` go. Instructions retired, dynasm, upstream/main -> this tree: math_sqrt_hot.py 4,266,475,480 -> 4,189,283,971 math_folds_hot.py 5,248,358,508 -> 5,269,222,715 spec_folds! rows: 93 -> 92 fn try_walker_specialize_: 80 -> 79 Assisted-by: Claude * jit-trace: restore the binary_slice_str fold The synth-only census that retired it missed `pyre/extra_tests/parity_tests/binary_slice_str_specialization.py` and one other parity test, where it fires 6 times: `str[start:stop]` on an exact `str` with exact-int bounds. `binary_slice` still lowers to the `RuntimeHelperKind::BinarySlice` MayForce residual, so without the fold that loop keeps the opaque call. The other six folds retired with it fire in neither the synth nor the parity-test census. spec_folds! rows: 92 -> 93 fn try_walker_specialize_: 79 -> 80 Assisted-by: Claude * math: descend the math builtins through interp2app gateways, drop their folds Every math builtin except frexp is now an interp2app gateway (`__majit_wrap_math_*`) whose fast arm reads an exact float or machine int, pins the domain before the C call, and calls one unboxed leaf in descroperation; anything else runs the old body through a `dont_look_inside` `_slow`. The generic builtin descent walks the gateways, so the math_log_trig, math_float1, math_float2, math_fabs, math_isclose, math_ldexp, math_isqrt, math_floor, math_ceil and math_trunc folds and their helpers are removed. The fast arms pin overflow before the C call (exp/expm1 x < 709, exp2 x < 1023, sinh/cosh |x| < 709, pow via the frexp exponent bound) instead of testing the result, because the `_slow` fallback has no executable address once the call has run and the descent declines. ldexp takes only the arm where scaling is exact, and `_float_ldexp_raw` builds 2**exp from its bits instead of `2.0.powf(exp)`, which returned 0 for ldexp(1024.0, -1080). asinh/acosh/atanh call pymath (libm) through elidable raw leaves; std's formulas differ from libm in the last place. math_frexp keeps its fold: the gateway has to root the mantissa box across the exponent box, and `push_roots` does not lower in a walked body. frexp is installed from extra_init; `math_builtin_name` now answers only "frexp", by its BuiltinCode function pointer. The boxing leaves keep their arithmetic in their own bodies; the pow and ldexp gateways share the frexp-exponent and exact-ldexp bit arithmetic with them through local macros. wasm32 links no BUILTIN_WRAPPER_DESCRIPTORS, so publish_optional_fnaddrs binds every math gateway's descriptor path to its address there (math_gateway_fnaddrs), the way jit_fnaddr binds the interpreter's own gateways; without it the descent found no jitcode and import_math ran at 31x dynasm. Removed helpers with no remaining caller: trace_opcode's ccall_pow, float_pow_jit, sqrt_nonneg_jit and math_{log,cos,sin}_*_jit, walker_float_helper_addrs, and interp_math's jit_math_frexp_*, jit_math_ldexp_raw, jit_math_isclose_default and jit_math_isqrt_i64, and the math1_gamma_result_finite optional-module hook. spec_folds! rows 93 -> 83, try_walker_specialize_ fns 80 -> 72. Assisted-by: Claude * math: make the gateway leaves lift in the rtyper prepass The math gateways joined the rtyper skip-subject ratchet with 20 names: each wrapper failed Phase A because a leaf it calls did not lift. - `f64::abs` and `f64::to_int_unchecked::<i64>` reach the flowspace adapter as the unary ops `abs` and `cast_float_to_int`, which `normalize_unary_op_name` refused. `abs` is `operation.py`'s `abs` (`FloatRepr.rtype_abs`); `cast_float_to_int` is what `FloatRepr.rtype_int` emits for `int(x)`, so it maps to `int`. - The frexp/ldexp bit arithmetic uses `f64::to_bits` / `f64::from_bits`, which the front lowers to `longlong2float` calls, instead of `transmute`. - erf, erfc, ulp, gamma, lgamma, remainder and fmod call one `#[majit_macros::elidable]` raw leaf each (`ll_math.py` llexternals are `elidable_function=True`); float `%` and the pymath calls stay inside those leaves. Locally the darwin ratchet no longer lists any math name, and 40 previously skipped graphs (`_float_abs`, `_float_math1`, `_int_from_*`, `abs_structural`, ...) now lift. Instructions retired, dynasm, origin/main 4f8c388 -> this change: erf 0.687, erfc 0.689, gamma 0.715, lgamma 0.705, remainder 0.416, ldexp 0.92; fmod, ulp, frexp, pow, fabs, ceil, floor, trunc, isqrt, isclose within +-1.3%. Output is identical. spec_folds! rows 83 -> 83, try_walker_specialize_ fns 72 -> 72. Assisted-by: Claude * str: descend startswith/endswith through their gateways, drop their folds `__majit_wrap_str_descr_startswith` / `_endswith` called the arity and keyword checks before the fast arm. `reject_kwargs` reaches `w_dict_str_entries_wtf8`, which does not lower, so the builtin descent always declined and the `str_startswith` / `str_endswith` folds answered. The fast arm now runs first (two operands, both exact `str`, no tuple): a keyword dict rides the same slice and is never a `str`. It calls the elidable `unicodeobject::startswith` / `endswith` leaf instead of the inline `rstring_prefix_eq!` byte loop; walking that loop in a descended body gave wrong counts for a mixed hit/miss input. Every other shape runs `str_method_startswith` / `str_method_endswith` through a `dont_look_inside` slow path. Removed `try_walker_specialize_str_prefix_match` and its two residual-call gates. spec_folds! rows: 83 -> 81 fn try_walker_specialize_: 72 -> 71 Assisted-by: Claude * builtins: descend abs/hash/ord/min/max through their gateways, drop builtin_fold1/2 Each builtin is now installed as a `__majit_wrap_builtin_*` gateway whose fast arm pins an exact operand before any call and whose other shapes run the original body through a `dont_look_inside` slow path: - `abs`: an exact int other than `i64::MIN` calls `_int_abs`; an exact float calls `_float_abs`; an exact complex calls `complex_abs`, which keeps the `_float_math1` hub (and the `frexp` / `gamma` leaves it mints) reachable now that `builtin_abs` sits behind the slow path. - `hash`: an exact int computes `_hash_int` inline; an exact `str`, long, bytes or non-NaN float calls the `elidable_cannot_raise` leaf `hash_exact_scalar`. The `str` arm of `hash_value` moved into `str_hash_value` so the leaf does not reach `hash_value`'s tuple arm. - `ord`: the `elidable_cannot_raise` leaf `ord_exact_str_char` returns the code point of a one-code-point exact `str`, or -1. - `min` / `max`: two exact ints or two exact floats compare inline and return the winning operand. `is_builtin_hash_function` / `is_builtin_ord_function` compare against the new gateways. `hash`, `ord`, `min` and `max` publish their descriptors through `builtin_wrapper_descriptor!`. Removed `try_walker_specialize_builtin_fold1`, `try_walker_specialize_builtin_fold2`, their residual-call gates and helpers, and the `jit_builtin_folds` raw helpers except `is_repr_builtin` / `builtin_code_fn_of`. `builtin_folds_hot.py` drops its `spec-folds=` header. spec_folds! rows: 81 -> 79 fn try_walker_specialize_: 71 -> 69 Assisted-by: Claude * math: publish the gateway descriptors through builtin_wrapper_descriptor! Each `__majit_wrap_math_*` descriptor is now declared with `pyre_interpreter::builtin_wrapper_descriptor!`, which appends a linkme slice element natively and registers the same path through a constructor on wasm32. `math_gateway_fnaddrs` and the wasm32 loop in `publish_optional_fnaddrs` that bound those paths by hand are removed. spec_folds! rows: 79 -> 79 fn try_walker_specialize_: 69 -> 69 Assisted-by: Claude * jit-trace: drop the binary_slice_str fold again Reverts f2e7eb7. `str[start:stop]` on an exact `str` with exact-int bounds goes back to the `RuntimeHelperKind::BinarySlice` residual, as on main since #1939. design.md's census numbers follow. spec_folds! rows: 79 -> 78 fn try_walker_specialize_: 69 -> 68 Assisted-by: Claude * math: check frexp's arity in its body; frexp fold declines before guarding `frexp` is installed through the arity-1 `install` closure without the `py_checked_arity_fn!` wrapper the `py_module!` table gave it, so `math.frexp(1, 2)` and keyword calls reached the body. The body now runs `check_declared_positional_arity("frexp", 1, args)` itself, which keeps its function pointer the one `math_builtin_name` recognises. `try_walker_specialize_math_frexp` now runs the authoritative-executor, exact-type and finite/non-zero/normal checks (`frexp_fold_operand`) before recording the callable `GuardValue` and the operand coercion. `_float_ldexp_raw` documents that its caller has checked `-1022 <= exp <= 1023`. spec_folds! rows: 78 -> 78 fn try_walker_specialize_: 68 -> 68 Assisted-by: Claude * result_exc: skip the from_exc_object rebuild of a discarded Err `err_payload_is_dead` answers true when the Result shell reaches a block that neither reads nor forwards it (`let _ = f();`). `catch_and_rewrap` then keeps the caught word instead of calling `from_exc_object`, whose `PyObject` parameter does not union with the caught `Exception`. That union was the prepass phase-A failure of `pyre_interpreter::error::chain_context` (`let _ = _break_context_cycle(exc, active);`). New unit test: catch_and_rewrap_does_not_rebuild_a_discarded_err. spec_folds! rows 73 -> 73, fn try_walker_specialize_ 66 -> 66. Assisted-by: Claude * design.md: recount the fold census on top of #1972 73 spec_folds! rows (4 descent), 66 try_walker_specialize_ functions (62/1/3), 18 try_walker_orthodox_ functions, 559 synth fixtures, 50 distinct rows named by 54 spec-folds= headers. spec_folds! rows 73 -> 73, fn try_walker_specialize_ 66 -> 66. Assisted-by: Claude * majit: pay the darwin rtyper skip-subject baseline down 61 subjects no longer skipped on darwin at this branch (the math/abs gateway leaves and wrappers, chain_context, and graphs #1974 already fixed); no additions. The linux baseline is unchanged. spec_folds! rows 73 -> 73, fn try_walker_specialize_ 66 -> 66. Assisted-by: Claude * pyre-object: pass str/bytes leaf operands as object pointers `jit_str_find`, `jit_str_rfind`, `jit_str_contains`, `jit_str_compare`, `jit_bytes_contains` and `jit_bytes_contains_byte` took their `W_UnicodeObject` / `W_BytesObject` operands as `i64`, and their interpreter callers passed `obj as i64`. The codewriter lowers that cast to `cast_ptr_to_int`, so a traced call carried the receiver as an Int box the GC map does not track, the shape #1975 fixed for the bounded find/rfind/count leaves. These leaves now take `PyObjectRef`, and `contains_str`, `contains_bytes_like`, the descroperation caller and the specialize.rs caller pass the objects directly. spec_folds! rows 72 -> 72, fn try_walker_specialize_ 65 -> 65. Assisted-by: Claude * design.md: recount the fold census on top of #1976 72 spec_folds! rows (4 descent), 65 try_walker_specialize_ functions (61/1/3), 560 synth fixtures, 49 distinct rows named by 54 spec-folds= headers. spec_folds! rows 72 -> 72, fn try_walker_specialize_ 65 -> 65. Assisted-by: Claude
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.
Removes five
spec_folds!rows (and twotry_walker_specialize_*functions). The orthodox walk already records the same traces for them:import_cachedobject_class_attrformat_with_specload_super_attr_descentbuiltin_locals_trace_limit_cut(the row only; the trace-limit cut itself stays)The fixture
spec-folds=headers were updated to match (class_reassign_hot.py,import_name.py,locals_expansion_trace_too_long.py,str_fstring.py).spec_folds!rowsfn try_walker_specialize_Each row was A/B'd with
PYRE_FBW_NO_SPECIALIZE=<label>across every fixture that consults it. Stdout and jit-stats matched, and CPU time was within 15%.Local gate on the previous base 665ead5 (darwin):
cargo test --all --no-default-features --features dynasmpass.dispatcher-graph acceptancejob at 17c8884 fails with the same list, plus 2 more there. The only other difference is the darwin-only kqueueclosepair. So the ratchet failure comes from main, not from this change.— commented by Claude Opus 5.5
Summary by CodeRabbit