From 0b8aa820c3867277d05e619d990d13cea983f5a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 31 Jul 2026 10:43:49 +0200 Subject: [PATCH 1/5] fix(repsel): scope each representation knob to its own representation (#7128) Two bisection knobs moved more than the representation they name, so every A/B taken through them measured two things at once. 1. PERRY_CANONICAL_I32_LOCALS=0 also disabled all Ptr consumption. #7121 split the FnCtx field, but the four ordinary-body construction sites still initialised both fields from one `repsel_allows` bool whose first conjunct was the canonical-i32 env read. 2. PERRY_CANONICAL_STR_LOCALS=0 also disabled three lowerings that never consult a selected Str local: the inline StringRef retag, the proven-heap string operand handle, and the tag-dispatched `.length`. All three key on a value's static string type. New `expr::repsel_gates` holds the knob table and a pure `RepselGates -> RepselContextFlags` derivation, so "one knob moves one flag" is a unit-testable property instead of a convention. The static string lowerings move to their own `PERRY_STATIC_STRING_LOWERING` knob, keyed into the object cache. Both knobs default on, so the default build is unchanged. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- crates/perry-codegen/src/codegen/closure.rs | 37 +- crates/perry-codegen/src/codegen/entry.rs | 22 +- crates/perry-codegen/src/codegen/function.rs | 39 +- crates/perry-codegen/src/codegen/method.rs | 62 ++- crates/perry-codegen/src/collectors/mod.rs | 2 +- crates/perry-codegen/src/expr/mod.rs | 21 +- crates/perry-codegen/src/expr/property_get.rs | 11 +- crates/perry-codegen/src/expr/repsel_gates.rs | 374 ++++++++++++++++++ .../perry-codegen/src/lower_string_method.rs | 16 +- .../src/native_value/materialize.rs | 21 +- .../src/commands/compile/object_cache.rs | 23 +- .../object_cache/object_cache_tests.rs | 3 + 12 files changed, 515 insertions(+), 116 deletions(-) create mode 100644 crates/perry-codegen/src/expr/repsel_gates.rs diff --git a/crates/perry-codegen/src/codegen/closure.rs b/crates/perry-codegen/src/codegen/closure.rs index 8e8612baa1..773327fe4f 100644 --- a/crates/perry-codegen/src/codegen/closure.rs +++ b/crates/perry-codegen/src/codegen/closure.rs @@ -758,28 +758,22 @@ pub(super) fn compile_closure( &cross_module.module_dispatch, ); - // Representation-selection Phase 1 context gate (see codegen/function.rs). + // Representation-selection context gates (see codegen/function.rs). // Async-step closures (CPS-rewritten `async` closures — the rewrite clears // `is_async`) and generator wrapper funcs route body locals through shared - // cells, so canonical-i32 storage is disallowed there. - let repsel_allows = crate::expr::canonical_i32_locals_enabled() - && !is_async - && !cross_module.async_step_closures.contains(&func_id) - && !cross_module.local_generator_funcs.contains(&func_id); - // Phase 3a: same context restrictions, independent env gate. - let repsel_str_allows = crate::expr::canonical_str_locals_enabled() - && !is_async - && !cross_module.async_step_closures.contains(&func_id) - && !cross_module.local_generator_funcs.contains(&func_id); - // #7106: report the structural context exclusion at the `Stmt::Let` site. - // The closure gate spells its generator/async-step reasons differently from - // the body gate, so map them onto the same rule names by hand. - let repsel_context_denial = crate::expr::body_context_denial( + // cells, so canonical storage is disallowed there. The closure gate spells + // its generator/async-step reasons differently from the body gate, so map + // them onto the same rule names here. + let repsel_flags = crate::expr::RepselContextFlags::for_body( is_async, cross_module.local_generator_funcs.contains(&func_id), cross_module.async_step_closures.contains(&func_id), ); - let report_denial = crate::expr::report_context_denial(repsel_context_denial); + let repsel_allows = repsel_flags.allows_canonical_i32; + let repsel_str_allows = repsel_flags.allows_canonical_str; + // #7106: report the structural context exclusion at the `Stmt::Let` site. + let repsel_context_denial = repsel_flags.canonical_denial; + let report_denial = repsel_flags.report_denial(); let repsel_closure_refs = if repsel_allows || repsel_str_allows || report_denial { crate::expr::collect_closure_referenced_locals(body) } else { @@ -947,11 +941,12 @@ pub(super) fn compile_closure( i32_counter_slots: HashMap::new(), local_slot_reps: HashMap::new(), repsel_context_allows_canonical_i32: repsel_allows, - // #7109: Phase 5a's Ptr context gate, split out of - // `repsel_context_allows_canonical_i32`. Ordinary bodies keep the - // exact pre-split value; only entry bodies diverge. - repsel_context_allows_ptr_shape: repsel_allows, - repsel_ptr_shape_context_denial: repsel_context_denial, + // #7109 split the FIELD out of `repsel_context_allows_canonical_i32`; + // #7128 split the VALUE, which is what the knob actually reads. Until + // then this was still `repsel_allows`, so `PERRY_CANONICAL_I32_LOCALS=0` + // disabled every Ptr consumption in the program. + repsel_context_allows_ptr_shape: repsel_flags.allows_ptr_shape, + repsel_ptr_shape_context_denial: repsel_flags.ptr_shape_denial, repsel_context_denial, repsel_closure_ref_locals: repsel_closure_refs, repsel_context_allows_canonical_str: repsel_str_allows, diff --git a/crates/perry-codegen/src/codegen/entry.rs b/crates/perry-codegen/src/codegen/entry.rs index 1c9e7b318e..146213b95a 100644 --- a/crates/perry-codegen/src/codegen/entry.rs +++ b/crates/perry-codegen/src/codegen/entry.rs @@ -674,8 +674,11 @@ pub(super) fn compile_module_entry( // is no structural context reason to deny — see // `expr::MODULE_INIT_CONTEXT` for the audit — so the only remaining // gates are the two bisection env knobs. - let repsel_allows = crate::expr::canonical_i32_locals_enabled(); - let repsel_str_allows = crate::expr::canonical_str_locals_enabled(); + // #7128: one derivation, each flag reading its own knob. `Entry` + // pins `allows_ptr_shape` off structurally (see below). + let repsel_flags = crate::expr::RepselContextFlags::for_entry(); + let repsel_allows = repsel_flags.allows_canonical_i32; + let repsel_str_allows = repsel_flags.allows_canonical_str; // The two value-level screens the `Stmt::Let` site consults (#7106 // collected them for the report only; now they are load-bearing). let repsel_closure_refs = if repsel_allows || repsel_str_allows { @@ -803,8 +806,8 @@ pub(super) fn compile_module_entry( // the canonical-i32 gate, and #6991 is a live rooting bug for a // compiled receiver held across the globalThis-population // collection — which runs around module init. - repsel_context_allows_ptr_shape: false, - repsel_ptr_shape_context_denial: Some(crate::expr::MODULE_INIT_CONTEXT), + repsel_context_allows_ptr_shape: repsel_flags.allows_ptr_shape, + repsel_ptr_shape_context_denial: repsel_flags.ptr_shape_denial, repsel_closure_ref_locals: repsel_closure_refs, repsel_context_allows_canonical_str: repsel_str_allows, repsel_str_ineligible_locals: repsel_str_ineligible, @@ -1334,8 +1337,11 @@ pub(super) fn compile_module_entry( // is no structural context reason to deny — see // `expr::MODULE_INIT_CONTEXT` for the audit — so the only remaining // gates are the two bisection env knobs. - let repsel_allows = crate::expr::canonical_i32_locals_enabled(); - let repsel_str_allows = crate::expr::canonical_str_locals_enabled(); + // #7128: one derivation, each flag reading its own knob. `Entry` + // pins `allows_ptr_shape` off structurally (see below). + let repsel_flags = crate::expr::RepselContextFlags::for_entry(); + let repsel_allows = repsel_flags.allows_canonical_i32; + let repsel_str_allows = repsel_flags.allows_canonical_str; // The two value-level screens the `Stmt::Let` site consults (#7106 // collected them for the report only; now they are load-bearing). let repsel_closure_refs = if repsel_allows || repsel_str_allows { @@ -1461,8 +1467,8 @@ pub(super) fn compile_module_entry( // the canonical-i32 gate, and #6991 is a live rooting bug for a // compiled receiver held across the globalThis-population // collection — which runs around module init. - repsel_context_allows_ptr_shape: false, - repsel_ptr_shape_context_denial: Some(crate::expr::MODULE_INIT_CONTEXT), + repsel_context_allows_ptr_shape: repsel_flags.allows_ptr_shape, + repsel_ptr_shape_context_denial: repsel_flags.ptr_shape_denial, repsel_closure_ref_locals: repsel_closure_refs, repsel_context_allows_canonical_str: repsel_str_allows, repsel_str_ineligible_locals: repsel_str_ineligible, diff --git a/crates/perry-codegen/src/codegen/function.rs b/crates/perry-codegen/src/codegen/function.rs index 86743956ee..4fdc676d53 100644 --- a/crates/perry-codegen/src/codegen/function.rs +++ b/crates/perry-codegen/src/codegen/function.rs @@ -644,25 +644,21 @@ pub(super) fn compile_function( ); } } - // Representation-selection Phase 1: canonical-i32 locals are allowed in - // plain synchronous function bodies only. Async / generator / - // `was_plain_async` bodies route locals through shared cells (the - // async-to-generator transform), which the canonical model must not touch. - let repsel_allows = crate::expr::canonical_i32_locals_enabled() - && !f.is_async - && !f.is_generator - && !f.was_plain_async; - // Phase 3a: same context restrictions, independent env gate. - let repsel_str_allows = crate::expr::canonical_str_locals_enabled() - && !f.is_async - && !f.is_generator - && !f.was_plain_async; + // Representation selection is allowed in plain synchronous function bodies + // only. Async / generator / `was_plain_async` bodies route locals through + // shared cells (the async-to-generator transform), which the canonical + // model must not touch. #7128: one derivation for all three + // representations, each reading its OWN env knob — see + // `expr::repsel_gates`. + let repsel_flags = + crate::expr::RepselContextFlags::for_body(f.is_async, f.is_generator, f.was_plain_async); + let repsel_allows = repsel_flags.allows_canonical_i32; + let repsel_str_allows = repsel_flags.allows_canonical_str; // #7106: when the context forbids selection for a STRUCTURAL reason, the // `Stmt::Let` site still reports one denial per would-be-eligible local, so // "async bodies are excluded" is a counted rule rather than a silent zero. - let repsel_context_denial = - crate::expr::body_context_denial(f.is_async, f.is_generator, f.was_plain_async); - let report_denial = crate::expr::report_context_denial(repsel_context_denial); + let repsel_context_denial = repsel_flags.canonical_denial; + let report_denial = repsel_flags.report_denial(); let repsel_closure_refs = if repsel_allows || repsel_str_allows || report_denial { crate::expr::collect_closure_referenced_locals(&f.body) } else { @@ -780,11 +776,12 @@ pub(super) fn compile_function( .collect(), i32_counter_slots: spec_i32_param_slots, repsel_context_allows_canonical_i32: repsel_allows, - // #7109: Phase 5a's Ptr context gate, split out of - // `repsel_context_allows_canonical_i32`. Ordinary bodies keep the - // exact pre-split value; only entry bodies diverge. - repsel_context_allows_ptr_shape: repsel_allows, - repsel_ptr_shape_context_denial: repsel_context_denial, + // #7109 split the FIELD out of `repsel_context_allows_canonical_i32`; + // #7128 split the VALUE, which is what the knob actually reads. Until + // then this was still `repsel_allows`, so `PERRY_CANONICAL_I32_LOCALS=0` + // disabled every Ptr consumption in the program. + repsel_context_allows_ptr_shape: repsel_flags.allows_ptr_shape, + repsel_ptr_shape_context_denial: repsel_flags.ptr_shape_denial, repsel_context_denial, repsel_closure_ref_locals: repsel_closure_refs, repsel_context_allows_canonical_str: repsel_str_allows, diff --git a/crates/perry-codegen/src/codegen/method.rs b/crates/perry-codegen/src/codegen/method.rs index 6e618a351d..b974296389 100644 --- a/crates/perry-codegen/src/codegen/method.rs +++ b/crates/perry-codegen/src/codegen/method.rs @@ -390,23 +390,17 @@ pub(super) fn compile_method( &cross_module.module_dispatch, ); - // Representation-selection Phase 1 context gate (see codegen/function.rs). - let repsel_allows = crate::expr::canonical_i32_locals_enabled() - && !method.is_async - && !method.is_generator - && !method.was_plain_async; - // Phase 3a: same context restrictions, independent env gate. - let repsel_str_allows = crate::expr::canonical_str_locals_enabled() - && !method.is_async - && !method.is_generator - && !method.was_plain_async; - // #7106: report the structural context exclusion at the `Stmt::Let` site. - let repsel_context_denial = crate::expr::body_context_denial( + // Representation-selection context gates (see codegen/function.rs). + let repsel_flags = crate::expr::RepselContextFlags::for_body( method.is_async, method.is_generator, method.was_plain_async, ); - let report_denial = crate::expr::report_context_denial(repsel_context_denial); + let repsel_allows = repsel_flags.allows_canonical_i32; + let repsel_str_allows = repsel_flags.allows_canonical_str; + // #7106: report the structural context exclusion at the `Stmt::Let` site. + let repsel_context_denial = repsel_flags.canonical_denial; + let report_denial = repsel_flags.report_denial(); let repsel_closure_refs = if repsel_allows || repsel_str_allows || report_denial { crate::expr::collect_closure_referenced_locals(&method.body) } else { @@ -524,11 +518,12 @@ pub(super) fn compile_method( i32_counter_slots: HashMap::new(), local_slot_reps: HashMap::new(), repsel_context_allows_canonical_i32: repsel_allows, - // #7109: Phase 5a's Ptr context gate, split out of - // `repsel_context_allows_canonical_i32`. Ordinary bodies keep the - // exact pre-split value; only entry bodies diverge. - repsel_context_allows_ptr_shape: repsel_allows, - repsel_ptr_shape_context_denial: repsel_context_denial, + // #7109 split the FIELD out of `repsel_context_allows_canonical_i32`; + // #7128 split the VALUE, which is what the knob actually reads. Until + // then this was still `repsel_allows`, so `PERRY_CANONICAL_I32_LOCALS=0` + // disabled every Ptr consumption in the program. + repsel_context_allows_ptr_shape: repsel_flags.allows_ptr_shape, + repsel_ptr_shape_context_denial: repsel_flags.ptr_shape_denial, repsel_context_denial, repsel_closure_ref_locals: repsel_closure_refs, repsel_context_allows_canonical_str: repsel_str_allows, @@ -1449,20 +1444,14 @@ pub(super) fn compile_static_method( &cross_module.module_dispatch, ); - // Representation-selection Phase 1 context gate (see codegen/function.rs). - let repsel_allows = crate::expr::canonical_i32_locals_enabled() - && !f.is_async - && !f.is_generator - && !f.was_plain_async; - // Phase 3a: same context restrictions, independent env gate. - let repsel_str_allows = crate::expr::canonical_str_locals_enabled() - && !f.is_async - && !f.is_generator - && !f.was_plain_async; + // Representation-selection context gates (see codegen/function.rs). + let repsel_flags = + crate::expr::RepselContextFlags::for_body(f.is_async, f.is_generator, f.was_plain_async); + let repsel_allows = repsel_flags.allows_canonical_i32; + let repsel_str_allows = repsel_flags.allows_canonical_str; // #7106: report the structural context exclusion at the `Stmt::Let` site. - let repsel_context_denial = - crate::expr::body_context_denial(f.is_async, f.is_generator, f.was_plain_async); - let report_denial = crate::expr::report_context_denial(repsel_context_denial); + let repsel_context_denial = repsel_flags.canonical_denial; + let report_denial = repsel_flags.report_denial(); let repsel_closure_refs = if repsel_allows || repsel_str_allows || report_denial { crate::expr::collect_closure_referenced_locals(&f.body) } else { @@ -1584,11 +1573,12 @@ pub(super) fn compile_static_method( i32_counter_slots: HashMap::new(), local_slot_reps: HashMap::new(), repsel_context_allows_canonical_i32: repsel_allows, - // #7109: Phase 5a's Ptr context gate, split out of - // `repsel_context_allows_canonical_i32`. Ordinary bodies keep the - // exact pre-split value; only entry bodies diverge. - repsel_context_allows_ptr_shape: repsel_allows, - repsel_ptr_shape_context_denial: repsel_context_denial, + // #7109 split the FIELD out of `repsel_context_allows_canonical_i32`; + // #7128 split the VALUE, which is what the knob actually reads. Until + // then this was still `repsel_allows`, so `PERRY_CANONICAL_I32_LOCALS=0` + // disabled every Ptr consumption in the program. + repsel_context_allows_ptr_shape: repsel_flags.allows_ptr_shape, + repsel_ptr_shape_context_denial: repsel_flags.ptr_shape_denial, repsel_context_denial, repsel_closure_ref_locals: repsel_closure_refs, repsel_context_allows_canonical_str: repsel_str_allows, diff --git a/crates/perry-codegen/src/collectors/mod.rs b/crates/perry-codegen/src/collectors/mod.rs index 90ebd15fd8..a411133a7e 100644 --- a/crates/perry-codegen/src/collectors/mod.rs +++ b/crates/perry-codegen/src/collectors/mod.rs @@ -68,7 +68,7 @@ pub(crate) use mutation::has_any_mutation; pub(crate) use pointer_locals::collect_pointer_typed_locals; pub(crate) use proven_this::{method_proven_this, prune_colliding_clones, pshape_method_name}; pub(crate) use ptr_numarray::{NumArrayDensity, NumArrayLocal}; -pub(crate) use ptr_shape::PtrShapeLocal; +pub(crate) use ptr_shape::{ptr_shape_locals_enabled, PtrShapeLocal}; pub(crate) use refs::{ collect_let_ids, collect_ref_ids_in_expr, collect_ref_ids_in_stmts, is_clamp_call, }; diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index 969aa96af3..b98988b0fa 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -131,18 +131,27 @@ pub(crate) use write_barrier::{ // under 2000 lines. Inherent methods (`record_value`) need no re-export. mod dispatch; mod record_value; +mod repsel_gates; mod scalar_slot_root; pub(crate) mod shadow_inline; mod shadow_slot; mod slot_rep; pub(crate) mod temp_root; +// #7128: the env-knob table and the pure `gates -> context flags` derivation. +// Every `FnCtx` construction site goes through `RepselContextFlags` so that a +// knob cannot silently acquire a second representation's sites again. +pub(crate) use repsel_gates::{static_string_lowering_enabled, RepselContextFlags}; +// `body_context_denial` / `report_context_denial` / `MODULE_INIT_CONTEXT` are +// deliberately NOT re-exported: since #7128 the only legitimate consumer is +// `repsel_gates::RepselContextFlags::derive`, and a `FnCtx` construction site +// that reaches for the structural rule directly is exactly how the two gates +// drifted back into one bool the last two times. pub(crate) use slot_rep::{ - body_context_denial, canonical_i32_locals_enabled, canonical_local_i32_slot, - canonical_str_locals_enabled, collect_canonical_str_ineligible_locals, - collect_closure_referenced_locals, deny_canonical_context, deny_canonical_i32, - load_canonical_local_boxed, local_is_canonical_str, local_rep_is_canonical_i32, - note_canonical_local, ptr_shape_context_rule_text, report_context_denial, - store_canonical_local_from_double, CanonicalI32Denial, SlotRep, MODULE_INIT_CONTEXT, + canonical_i32_locals_enabled, canonical_local_i32_slot, canonical_str_locals_enabled, + collect_canonical_str_ineligible_locals, collect_closure_referenced_locals, + deny_canonical_context, deny_canonical_i32, load_canonical_local_boxed, + local_is_canonical_str, local_rep_is_canonical_i32, note_canonical_local, + ptr_shape_context_rule_text, store_canonical_local_from_double, CanonicalI32Denial, SlotRep, PTR_SHAPE_SCALAR_REPLACED, }; diff --git a/crates/perry-codegen/src/expr/property_get.rs b/crates/perry-codegen/src/expr/property_get.rs index 9b3c6d21dd..fe54039b18 100644 --- a/crates/perry-codegen/src/expr/property_get.rs +++ b/crates/perry-codegen/src/expr/property_get.rs @@ -271,9 +271,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { return Ok(ctx.block().load(DOUBLE, &slot)); } } - // Representation-selection Phase 3a: `.length` on a statically- - // string receiver (canonical-Str local, `string[]` element, - // string-returning expression). The receiver bits are freshly + // `.length` on a statically-string receiver (`string`-typed local, + // `string[]` element, string-returning expression). #7128: this + // arrived in Phase 3a but keys on `is_string_expr` — the receiver's + // static TYPE — and never on a canonical-`Str` selection, so it is + // on `PERRY_STATIC_STRING_LOWERING`, not on the `Str` knob. + // The receiver bits are freshly // produced with no safepoint before the header read (no // forwarding hazard — evacuation rewrites slots/returns before // the mutator resumes), so the ~18-op generic tower below @@ -285,7 +288,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // receiver) → the same `js_value_length_f64` slow call the // generic tower's slow arm uses. { - if crate::expr::canonical_str_locals_enabled() + if crate::expr::static_string_lowering_enabled() && is_string_expr(ctx, object) && !is_array_expr(ctx, object) { diff --git a/crates/perry-codegen/src/expr/repsel_gates.rs b/crates/perry-codegen/src/expr/repsel_gates.rs new file mode 100644 index 0000000000..4ae4431d51 --- /dev/null +++ b/crates/perry-codegen/src/expr/repsel_gates.rs @@ -0,0 +1,374 @@ +//! Representation-selection env knobs, and the per-context flags derived from +//! them (#7128). +//! +//! ## Why the knobs need a module of their own +//! +//! Perry ships one bisection knob per unboxed representation. Their whole +//! value is A/B: build twice, flip one knob, attribute the difference to that +//! representation. **That attribution is only sound if the knob moves nothing +//! else** — and twice it did not, in ways that were invisible from the knob's +//! name and that retroactively weakened every measurement taken through it: +//! +//! * **`PERRY_CANONICAL_I32_LOCALS=0` also disabled every `Ptr` +//! consumption.** Phase 5a reused `repsel_context_allows_canonical_i32` as +//! *its* context gate. #7121 split the `FnCtx` FIELD, but left the four +//! ordinary-body construction sites initialising both fields from one +//! `repsel_allows` bool whose first conjunct was the canonical-i32 env read +//! — so the field split was a no-op for the knob. Census under that knob +//! read `ptr-shape: 7 selected, 0 consumed`, with all six consumption sites +//! printing `NEVER FIRES`. Two of the four workloads whose object moves +//! under the knob (`batch`, `suite_09_method_calls`) were therefore +//! measuring two representations at once. +//! * **`PERRY_CANONICAL_STR_LOCALS=0` also disabled three lowerings that never +//! consult a selected `Str` local at all** — the inline `StringRef` retag +//! (`native_value/materialize.rs`), the proven-heap operand arm of +//! `str_operand_handle_tag_dispatched` (`lower_string_method.rs`), and the +//! tag-dispatched `.length` on any statically-string expression +//! (`expr/property_get.rs`). All three key on a value's static string TYPE, +//! not on slot selection, so 24 of the 26 census workloads emitted +//! differently under the knob — including workloads whose `canonical-str` +//! count is zero. +//! +//! ## The rule +//! +//! **A knob may move only the sites of the representation it names.** Stated +//! mechanically, and checked by +//! `scripts/compiler_output_harness/repsel_knob_isolation.py` over the census +//! corpus: +//! +//! 1. with knob `X=0` and everything else default, no census key outside `X`'s +//! own keys may change; and +//! 2. a workload in which `X` promotes nothing must emit a **byte-identical** +//! object. +//! +//! ## The knob table +//! +//! | env knob | representation | census keys | +//! |---|---|---| +//! | `PERRY_CANONICAL_I32_LOCALS` | canonical `i32`/`u32` slots (Phase 1) | `canonical-i32`, `canonical-u32` | +//! | `PERRY_CANONICAL_STR_LOCALS` | canonical `Str` slots (Phase 3a) | `canonical-str` | +//! | `PERRY_PTR_SHAPE_LOCALS` | `Ptr` (Phase 3b/5a) | `ptr-shape`, `ptr-shape-consumed` | +//! | `PERRY_PTR_NUMARRAY_LOCALS` | `Ptr` (Phase 4a.3) | `ptr-numarray` | +//! | `PERRY_INT_VALUED_LOCALS` | int-valued TA residency (#6898) | `int-valued-ta` | +//! | `PERRY_SPECIALIZED_ABI` | specialized ABI entries (Phase 2) | `spec-abi-entry`, `spec-abi-taptr-slot` | +//! | `PERRY_STATIC_STRING_LOWERING` | *(not a representation)* string fast paths keyed on a value's static type | — | +//! +//! The last row is the residue of defect 2. Those three lowerings are real and +//! shipped, but they are not representation selection: nothing about them +//! depends on a local having been selected. They keep a kill switch of their +//! own rather than riding on `Str`'s, so that `PERRY_CANONICAL_STR_LOCALS` +//! means what it says. `PERRY_PTR_SHAPE_THIS` is a sub-knob of +//! `PERRY_PTR_SHAPE_LOCALS` (Phase 5a's `__pshape` clones only) and is honoured +//! by the same collector. + +use super::slot_rep::{ + body_context_denial, canonical_i32_locals_enabled, canonical_str_locals_enabled, + MODULE_INIT_CONTEXT, +}; + +/// `PERRY_STATIC_STRING_LOWERING` gate. Enabled by default; `=0`/`off`/`false` +/// reverts the three string fast paths that key on a value's **static string +/// type** rather than on a canonical-`Str` slot selection: +/// +/// * `native_value::materialize::nanbox_string_ref_boxed` — the inline +/// `or STRING_TAG; bitcast` retag of a raw `StringRef` handle, with the +/// null-handle case kept in a cold `js_nanbox_string` arm; +/// * `lower_string_method::str_operand_handle_tag_dispatched`'s +/// `proven_heap_string_operand` arm — inline `bitcast; and POINTER_MASK` for +/// a literal / `String(x)` operand; +/// * `expr::property_get`'s tag-dispatched `.length` on any `is_string_expr` +/// receiver. +/// +/// All three shipped in Phase 3a (#6909) behind `PERRY_CANONICAL_STR_LOCALS`, +/// which made that knob unusable for A/B: they fire on programs that select no +/// canonical-`Str` local at all (#7128, finding B). Splitting them out here is +/// **behaviour-preserving by construction** — both knobs default on, so the +/// default build emits the identical bytes; only the `=0` arms change meaning. +/// Keyed into the object cache (`object_cache.rs`). +pub(crate) fn static_string_lowering_enabled() -> bool { + use std::sync::OnceLock; + static CACHED: OnceLock = OnceLock::new(); + *CACHED.get_or_init(|| { + !matches!( + std::env::var("PERRY_STATIC_STRING_LOWERING").as_deref(), + Ok("0") | Ok("off") | Ok("false") + ) + }) +} + +/// The env knobs that gate a *context* decision, read once and passed by value. +/// +/// Passed explicitly rather than read inside [`RepselContextFlags::derive`] for +/// one reason: the readers are process-wide `OnceLock`s, so a unit test cannot +/// flip them. With the gates as a parameter, `derive` is a pure function and +/// the "one knob moves one flag" property is testable directly — which is what +/// the tests at the bottom of this file do, and what nothing checked before +/// #7128. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) struct RepselGates { + pub canonical_i32: bool, + pub canonical_str: bool, + pub ptr_shape: bool, +} + +impl RepselGates { + pub(crate) fn from_env() -> Self { + Self { + canonical_i32: canonical_i32_locals_enabled(), + canonical_str: canonical_str_locals_enabled(), + ptr_shape: crate::collectors::ptr_shape_locals_enabled(), + } + } +} + +/// Which kind of body a `FnCtx` is being constructed for. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum RepselBody { + /// An ordinary function, method or closure body. `is_generator` / + /// `was_plain_async` carry the closure spellings too + /// (`local_generator_funcs`, `async_step_closures`) — same conjunction, + /// different registry. + Body { + is_async: bool, + is_generator: bool, + was_plain_async: bool, + }, + /// A module-init or program-entry body (`codegen/entry.rs`). Canonical + /// i32/u32/Str are allowed here since #7109; `Ptr` is not — see + /// [`MODULE_INIT_CONTEXT`] for the audit and #6991 for the live rooting bug + /// that keeps it off. + Entry, +} + +/// The three context gates a `FnCtx` carries, plus the rule names the +/// `--opt-report` denial records use. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) struct RepselContextFlags { + pub allows_canonical_i32: bool, + pub allows_canonical_str: bool, + pub allows_ptr_shape: bool, + /// Why this context forbids canonical (i32/u32/Str) selection, or `None`. + /// Structural reasons only — never the env knobs (see + /// `slot_rep::body_context_denial`). + pub canonical_denial: Option<&'static str>, + /// Why this context forbids acting on a `Ptr` proof, or `None`. + pub ptr_shape_denial: Option<&'static str>, +} + +impl RepselContextFlags { + /// Pure derivation: `gates` in, flags out, no env reads. + /// + /// **Each output flag reads exactly one input gate.** That is the whole + /// point of this function existing, and `each_knob_moves_exactly_one_flag` + /// below is the assertion. Before #7128 the four ordinary-body `FnCtx` + /// construction sites computed `allows_ptr_shape` from the canonical-i32 + /// gate, so `PERRY_CANONICAL_I32_LOCALS=0` silently turned off a second + /// representation. + pub(crate) fn derive(gates: RepselGates, body: RepselBody) -> Self { + match body { + RepselBody::Body { + is_async, + is_generator, + was_plain_async, + } => { + let denial = body_context_denial(is_async, is_generator, was_plain_async); + let structural_ok = denial.is_none(); + Self { + allows_canonical_i32: gates.canonical_i32 && structural_ok, + allows_canonical_str: gates.canonical_str && structural_ok, + allows_ptr_shape: gates.ptr_shape && structural_ok, + canonical_denial: denial, + // Same structural rule, its own field: before #7109 this + // read `repsel_context_denial`, which no longer names a + // rule in entry bodies. + ptr_shape_denial: denial, + } + } + RepselBody::Entry => Self { + allows_canonical_i32: gates.canonical_i32, + allows_canonical_str: gates.canonical_str, + // Unconditionally off, regardless of `gates.ptr_shape`: the + // exclusion is structural (#6991), not a knob. Written as a + // literal so a future reader cannot mistake it for something + // `PERRY_PTR_SHAPE_LOCALS=1` could turn back on. + allows_ptr_shape: false, + canonical_denial: None, + ptr_shape_denial: Some(MODULE_INIT_CONTEXT), + }, + } + } + + /// Read the env gates and derive for an ordinary body. + pub(crate) fn for_body(is_async: bool, is_generator: bool, was_plain_async: bool) -> Self { + Self::derive( + RepselGates::from_env(), + RepselBody::Body { + is_async, + is_generator, + was_plain_async, + }, + ) + } + + /// Read the env gates and derive for a module-init / program-entry body. + pub(crate) fn for_entry() -> Self { + Self::derive(RepselGates::from_env(), RepselBody::Entry) + } + + /// Whether the value-level screens should be collected purely so + /// `--opt-report` can count the structural denial exactly (#7106). + pub(crate) fn report_denial(&self) -> bool { + super::slot_rep::report_context_denial(self.canonical_denial) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const ALL_ON: RepselGates = RepselGates { + canonical_i32: true, + canonical_str: true, + ptr_shape: true, + }; + + const SYNC: RepselBody = RepselBody::Body { + is_async: false, + is_generator: false, + was_plain_async: false, + }; + + fn allows(flags: &RepselContextFlags) -> (bool, bool, bool) { + ( + flags.allows_canonical_i32, + flags.allows_canonical_str, + flags.allows_ptr_shape, + ) + } + + /// #7128's acceptance property, at the smallest scale it can be stated: + /// turning off exactly one knob turns off exactly one context flag. + /// + /// Sabotage-checked: restoring the pre-#7128 coupling + /// (`allows_ptr_shape: gates.canonical_i32 && structural_ok`) takes the + /// `canonical_i32` row red with "knob canonical_i32 also moved + /// allows_ptr_shape", which is precisely finding A. + #[test] + fn each_knob_moves_exactly_one_flag() { + let baseline = RepselContextFlags::derive(ALL_ON, SYNC); + assert_eq!(allows(&baseline), (true, true, true)); + + let cases: [(&str, RepselGates, (bool, bool, bool)); 3] = [ + ( + "canonical_i32", + RepselGates { + canonical_i32: false, + ..ALL_ON + }, + (false, true, true), + ), + ( + "canonical_str", + RepselGates { + canonical_str: false, + ..ALL_ON + }, + (true, false, true), + ), + ( + "ptr_shape", + RepselGates { + ptr_shape: false, + ..ALL_ON + }, + (true, true, false), + ), + ]; + for (name, gates, want) in cases { + let got = RepselContextFlags::derive(gates, SYNC); + assert_eq!( + allows(&got), + want, + "knob {name} moved a flag it does not own: \ + (canonical_i32, canonical_str, ptr_shape) = {:?}, expected {want:?}", + allows(&got) + ); + // A bisection knob is not a structural rule: it must never invent a + // `--opt-report` denial the default build cannot emit. + assert_eq!(got.canonical_denial, None, "knob {name}"); + assert_eq!(got.ptr_shape_denial, None, "knob {name}"); + } + } + + /// The same property for the entry context, where `Ptr` is off for a + /// structural reason: the two canonical knobs must still move only + /// themselves, and the `Ptr` knob must move nothing (it is already + /// off). + #[test] + fn entry_context_keeps_ptr_shape_off_and_names_the_rule() { + let entry = RepselContextFlags::derive(ALL_ON, RepselBody::Entry); + assert_eq!(allows(&entry), (true, true, false)); + assert_eq!(entry.canonical_denial, None); + assert_eq!(entry.ptr_shape_denial, Some(MODULE_INIT_CONTEXT)); + + for gates in [ + RepselGates { + canonical_i32: false, + ..ALL_ON + }, + RepselGates { + canonical_str: false, + ..ALL_ON + }, + RepselGates { + ptr_shape: false, + ..ALL_ON + }, + ] { + let got = RepselContextFlags::derive(gates, RepselBody::Entry); + assert!(!got.allows_ptr_shape); + assert_eq!(got.ptr_shape_denial, Some(MODULE_INIT_CONTEXT)); + assert_eq!(got.allows_canonical_i32, gates.canonical_i32); + assert_eq!(got.allows_canonical_str, gates.canonical_str); + } + } + + /// The structural denials still apply to all three representations, and + /// still name a rule. This is the part #7128 must NOT change: an async body + /// is excluded because the async-to-generator transform boxes body locals, + /// which is a hazard, not a knob. + #[test] + fn structural_denials_apply_to_every_representation() { + for (body, rule) in [ + ( + RepselBody::Body { + is_async: true, + is_generator: false, + was_plain_async: false, + }, + "async_body", + ), + ( + RepselBody::Body { + is_async: false, + is_generator: true, + was_plain_async: false, + }, + "generator_body", + ), + ( + RepselBody::Body { + is_async: false, + is_generator: false, + was_plain_async: true, + }, + "was_plain_async_body", + ), + ] { + let got = RepselContextFlags::derive(ALL_ON, body); + assert_eq!(allows(&got), (false, false, false), "{rule}"); + assert_eq!(got.canonical_denial, Some(rule)); + assert_eq!(got.ptr_shape_denial, Some(rule)); + } + } +} diff --git a/crates/perry-codegen/src/lower_string_method.rs b/crates/perry-codegen/src/lower_string_method.rs index 9bceb71729..b3272193bb 100644 --- a/crates/perry-codegen/src/lower_string_method.rs +++ b/crates/perry-codegen/src/lower_string_method.rs @@ -1462,18 +1462,20 @@ fn proven_heap_string_operand(_ctx: &FnCtx<'_>, e: &Expr) -> bool { /// annotation lie) → the legacy `js_get_string_pointer_unified` (which /// materializes SSO — cold); /// - everything else (or flag off) → the legacy unified call, unchanged. +/// +/// #7128: the two arms are on separate knobs, because only the second one is +/// about a selected representation. The proven-heap arm keys on the operand's +/// static type and fires with zero canonical-`Str` locals in the program. fn str_operand_handle_tag_dispatched(ctx: &mut FnCtx<'_>, object: &Expr, recv_box: &str) -> String { use crate::nanbox::POINTER_MASK_I64; - if !crate::expr::canonical_str_locals_enabled() { - return unbox_str_handle(ctx.block(), recv_box); - } - if proven_heap_string_operand(ctx, object) { + if crate::expr::static_string_lowering_enabled() && proven_heap_string_operand(ctx, object) { let bits = ctx.block().bitcast_double_to_i64(recv_box); return ctx.block().and(I64, &bits, POINTER_MASK_I64); } - let canonical = matches!( - object, Expr::LocalGet(id) if crate::expr::local_is_canonical_str(ctx, *id) - ); + let canonical = crate::expr::canonical_str_locals_enabled() + && matches!( + object, Expr::LocalGet(id) if crate::expr::local_is_canonical_str(ctx, *id) + ); if !canonical { return unbox_str_handle(ctx.block(), recv_box); } diff --git a/crates/perry-codegen/src/native_value/materialize.rs b/crates/perry-codegen/src/native_value/materialize.rs index a3e8e91cce..9c8aae063a 100644 --- a/crates/perry-codegen/src/native_value/materialize.rs +++ b/crates/perry-codegen/src/native_value/materialize.rs @@ -434,14 +434,21 @@ fn materialize_js_value_bits_to_js_value( } /// NaN-box a raw `StringRef` handle (i64 `StringHeader*`) as a boxed string -/// value. Repsel Phase 3a: with `PERRY_CANONICAL_STR_LOCALS` on (the -/// default), the hot non-null path is the inline `or STRING_TAG; bitcast` -/// pair (`expr/nanbox_inline.rs` shape) instead of the opaque -/// `js_nanbox_string` call; the helper's one semantic addition — a null -/// handle allocates an empty string — is preserved in a cold arm that still -/// calls it. Flag off reverts to the pre-phase unconditional call. +/// value. The hot non-null path is the inline `or STRING_TAG; bitcast` pair +/// (`expr/nanbox_inline.rs` shape) instead of the opaque `js_nanbox_string` +/// call; the helper's one semantic addition — a null handle allocates an empty +/// string — is preserved in a cold arm that still calls it. Flag off reverts +/// to the pre-phase unconditional call. +/// +/// #7128: this shipped in Phase 3a behind `PERRY_CANONICAL_STR_LOCALS`, but it +/// consults **no** canonical-`Str` local — it fires wherever a `StringRef` +/// materializes, which is most programs. That made the canonical-`Str` knob +/// useless as a bisection instrument (24 of 26 census workloads emitted +/// differently under it, including ones that select zero `Str` locals). It is +/// on `PERRY_STATIC_STRING_LOWERING` now; both default on, so the default +/// build is unchanged. fn nanbox_string_ref_boxed(ctx: &mut FnCtx<'_>, handle: &str) -> String { - if !crate::expr::canonical_str_locals_enabled() { + if !crate::expr::static_string_lowering_enabled() { return ctx .block() .call(DOUBLE, "js_nanbox_string", &[(I64, handle)]); diff --git a/crates/perry/src/commands/compile/object_cache.rs b/crates/perry/src/commands/compile/object_cache.rs index a4049bf63b..2194ce4fbb 100644 --- a/crates/perry/src/commands/compile/object_cache.rs +++ b/crates/perry/src/commands/compile/object_cache.rs @@ -978,17 +978,30 @@ fn compute_object_cache_key_with_env( .unwrap_or(""), ); // Representation-selection Phase 3a — canonical string locals - // (tagged-at-rest): `=0`/`off`/`false` reverts the Str-gated lowerings - // (`+=` tag-dispatch, inline `.length`, direct string compares, the - // char-access receiver fast arm, and the inline StringRef retag) back to - // the pre-phase sequences, which changes the emitted IR / .o bytes — a - // warm cache must not serve an object built under the other setting. + // (tagged-at-rest): `=0`/`off`/`false` reverts the lowerings that consult a + // SELECTED `Str` local (`+=` tag-dispatch, direct string compares, the + // char-access receiver fast arm) back to the pre-phase sequences, which + // changes the emitted IR / .o bytes — a warm cache must not serve an object + // built under the other setting. h.field( "env_canonical_str_locals", env_var("PERRY_CANONICAL_STR_LOCALS") .as_deref() .unwrap_or(""), ); + // #7128 — string fast paths that key on a value's STATIC string type and + // never on a canonical-`Str` selection: the inline `StringRef` retag, the + // proven-heap string operand handle, and the tag-dispatched `.length`. + // They shipped under `PERRY_CANONICAL_STR_LOCALS`, which made that knob + // move 24 of 26 census workloads and stop being evidence about the + // representation it names. `=0`/`off`/`false` reverts all three, which + // changes the emitted IR / .o bytes — so it keys the cache too. + h.field( + "env_static_string_lowering", + env_var("PERRY_STATIC_STRING_LOWERING") + .as_deref() + .unwrap_or(""), + ); // Representation-selection Phase 2 — specialized calling convention: // `PERRY_SPECIALIZED_ABI=0/off/false` removes the specialized entries and // their static/guarded dispatch sites; `PERRY_SPECIALIZED_ABI_MAX` diff --git a/crates/perry/src/commands/compile/object_cache/object_cache_tests.rs b/crates/perry/src/commands/compile/object_cache/object_cache_tests.rs index 5b0733e451..f8320f7af5 100644 --- a/crates/perry/src/commands/compile/object_cache/object_cache_tests.rs +++ b/crates/perry/src/commands/compile/object_cache/object_cache_tests.rs @@ -619,6 +619,9 @@ fn key_changes_with_codegen_env_vars() { "PERRY_CANONICAL_I32_LOCALS", // Representation-selection Phase 3a: canonical string locals. "PERRY_CANONICAL_STR_LOCALS", + // #7128: string fast paths keyed on a value's static string type, + // split off the Phase 3a knob so that knob isolates its own rep. + "PERRY_STATIC_STRING_LOWERING", // Representation-selection Phase 2: specialized calling convention. "PERRY_SPECIALIZED_ABI", "PERRY_SPECIALIZED_ABI_MAX", From 07a27587abed6d51872f2df0cad4129e5e63cc4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 31 Jul 2026 10:50:10 +0200 Subject: [PATCH 2/5] test(repsel): gate that each representation knob moves only its own representation (#7128) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `census-knob-isolation` runs the census corpus once per bisection knob and asserts, per knob: 1. no census key outside the knob's own may change; 2. a workload whose representation promotes nothing must emit a BYTE-IDENTICAL object; 3. the knob is still live — it must take a promotion away somewhere and change some object, or it has stopped being an instrument. Two controls guard the diff itself: the compiler must be deterministic (it is not on aarch64 Linux, where the LLVM module name embeds pid + nanotime — the emission half is skipped there rather than reporting phantoms), and both `K=1` and an env var the compiler does not read must reproduce the default object bit-for-bit. Rule 1 is what catches the canonical-i32/Ptr entanglement; rule 2 is what catches the canonical-Str one, which leaves every count untouched and still changes 24 of 26 objects. The self-test replays both defects as synthetic arm tables so the two branches are exercised without a compiler. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- scripts/compiler_output_harness/cli.py | 30 + .../compiler_output_harness/repsel_census.py | 50 +- .../repsel_knob_isolation.py | 597 ++++++++++++++++++ 3 files changed, 676 insertions(+), 1 deletion(-) create mode 100644 scripts/compiler_output_harness/repsel_knob_isolation.py diff --git a/scripts/compiler_output_harness/cli.py b/scripts/compiler_output_harness/cli.py index 65e9c044d8..3c410eb01f 100644 --- a/scripts/compiler_output_harness/cli.py +++ b/scripts/compiler_output_harness/cli.py @@ -7,6 +7,8 @@ from .common import DEFAULT_BENCHMARK_RUNS, HarnessError from .repsel_census import census from .repsel_census import self_test as census_self_test +from .repsel_knob_isolation import check_isolation +from .repsel_knob_isolation import self_test as isolation_self_test from .spec import WORKLOADS @@ -129,6 +131,34 @@ def build_parser() -> argparse.ArgumentParser: ) census_self_p.set_defaults(func=census_self_test) + # Knob isolation (#7128). Runs the census corpus once per bisection knob and + # asserts each knob moves only its own representation — the property every + # knob-based A/B silently assumes and that nothing checked until two knobs + # were caught moving two representations each. + iso_p = sub.add_parser( + "census-knob-isolation", + help="assert each representation knob moves only its own representation", + ) + iso_p.add_argument("--perry") + iso_p.add_argument("--baseline") + iso_p.add_argument("--workload", action="append", help="restrict to named workload(s)") + iso_p.add_argument("--knob", action="append", help="restrict to named knob(s)") + iso_p.add_argument("--compile-timeout", type=int, default=300) + iso_p.add_argument("--jobs", type=int, default=4, help="parallel compiles") + iso_p.add_argument( + "--require-emission", + action="store_true", + help="fail instead of skipping when the host cannot emit objects deterministically", + ) + iso_p.add_argument("--keep-objects", action="store_true") + iso_p.set_defaults(func=check_isolation) + + iso_self_p = sub.add_parser( + "census-knob-isolation-self-test", + help="check the knob-isolation verdict logic without compiling", + ) + iso_self_p.set_defaults(func=isolation_self_test) + return parser diff --git a/scripts/compiler_output_harness/repsel_census.py b/scripts/compiler_output_harness/repsel_census.py index 9150935910..2ae84b3db6 100644 --- a/scripts/compiler_output_harness/repsel_census.py +++ b/scripts/compiler_output_harness/repsel_census.py @@ -441,6 +441,8 @@ def compile_and_census( timeout: int, extra_env: dict[str, str] | None = None, keep_report: Path | None = None, + object_out: Path | None = None, + with_report: bool = False, ) -> dict[str, Any]: """Compile `source` with `--opt-report=json --no-link` and reduce it. @@ -448,6 +450,14 @@ def compile_and_census( census never needs `libperry_runtime.a` and cannot be fooled by a stale one. `--no-cache` is redundant (`--opt-report` forces it) but stated so the intent survives a change upstream. + + `object_out` keeps the emitted object instead of discarding it with the temp + directory — the knob-isolation gate (#7128) needs the bytes, because a knob + can leave every census count untouched and still change what ships (which is + exactly what `PERRY_CANONICAL_STR_LOCALS` did on 24 of 26 workloads). + `with_report` returns the raw payload alongside the counts for the same + reason: some representation sites (a specialized entry's `i32` parameter + slot, a proven-`this` receiver) are not counted by any census key. """ if not source.exists(): raise HarnessError(f"census source not found: {source}") @@ -458,11 +468,16 @@ def compile_and_census( env.update(extra_env) with tempfile.TemporaryDirectory(prefix="repsel-census-") as tmp: tmpdir = Path(tmp) + if object_out is not None: + object_out.parent.mkdir(parents=True, exist_ok=True) + out_path = object_out + else: + out_path = tmpdir / "census.o" cmd = perry + [ "compile", str(source), "-o", - str(tmpdir / "census.o"), + str(out_path), "--opt-report=json", "--no-link", "--no-cache", @@ -486,10 +501,43 @@ def compile_and_census( keep_report.parent.mkdir(parents=True, exist_ok=True) keep_report.write_text(json.dumps(payload, indent=2), encoding="utf-8") census = census_from_report(payload) + if with_report: + census["report"] = payload + if object_out is not None: + # Take the paths the compiler SAYS it wrote rather than assuming `-o` + # named them: a multi-module compile emits several, and an earlier + # version of this A/B hashed an empty directory and reported "all + # identical" (#7121). An arm that produces no object is a harness + # error, not a silent pass. + census["objects"] = _written_objects(result.stdout) census["source"] = str(source.relative_to(REPO_ROOT)) return census +#: `run_pipeline.rs` prints one of these per emitted artifact, on stdout. +_WROTE_OBJECT = re.compile(r"^(?:Wrote object file|Stored cached object): (.+)$", re.M) + + +def _written_objects(stdout: str) -> list[str]: + """Every object path the compiler reported writing, in emission order. + + Raises rather than returning `[]`: "no objects" and "identical objects" are + the same answer to an object-level A/B, and the empty one is always wrong. + """ + paths = [m.group(1).strip() for m in _WROTE_OBJECT.finditer(stdout)] + if not paths: + raise HarnessError( + "the compiler reported writing no object file. An object-level A/B " + "over zero objects reports 'identical' for every arm, which is the " + "vacuous comparison this harness exists to avoid.\n" + f"stdout tail:\n{stdout[-2000:]}" + ) + missing = [p for p in paths if not Path(p).is_file()] + if missing: + raise HarnessError(f"compiler reported objects that do not exist: {missing}") + return paths + + def _extract_json(stderr: str) -> dict[str, Any]: """Pull the report object out of the compiler's stderr. diff --git a/scripts/compiler_output_harness/repsel_knob_isolation.py b/scripts/compiler_output_harness/repsel_knob_isolation.py new file mode 100644 index 0000000000..1fd2395b7a --- /dev/null +++ b/scripts/compiler_output_harness/repsel_knob_isolation.py @@ -0,0 +1,597 @@ +"""Representation-selection knob isolation gate (#7128). + +Perry ships one bisection knob per unboxed representation, and every +representation-selection measurement to date has been read through them: build +twice, flip one knob, attribute the difference. **That attribution is a +non-sequitur unless the knob moves exactly one representation**, and twice it +did not: + +* `PERRY_CANONICAL_I32_LOCALS=0` also switched off every `Ptr` + consumption in the program (`ptr-shape` went 7 selected / 3 consumed → + 7 selected / **0** consumed, with all six consumption sites printing + `NEVER FIRES`). Two of the four workloads whose object moves under that knob + were therefore measuring two representations at once. +* `PERRY_CANONICAL_STR_LOCALS=0` also switched off three string lowerings that + never consult a selected `Str` local, so **24 of 26** census workloads + emitted differently under it — including workloads whose `canonical-str` + count is zero. + +Neither defect was visible from the knob's name, from the census table, or from +any test. Both were found by measurement, after a day of A/B runs had already +been taken through them. + +## What this gate asserts + +For each knob `K`, with `K=0` and every other knob at its default: + +1. **No cross-representation count leak.** Every census key *not* owned by `K` + reads exactly what the default build reads, on every workload. +2. **No cross-representation emission leak.** A workload in which `K`'s + representation promotes *nothing* must compile to a **byte-identical** + object. This is the half that catches defect 2: it leaves every census count + alone and still changes what ships. +3. **The knob is live** — corpus-wide it must take at least one owned count + down AND change at least one object. A knob that moves nothing has stopped + being an instrument (CLAUDE.md, "the gate runs but its subject never did"). + +Plus two controls, because a diff-based gate that cannot tell "different" from +"noisy" proves nothing: + +* **determinism** — the same compiler, same flags, twice, must produce the same + bytes. On aarch64 Linux it does not (the LLVM module name embeds pid + + nanotime), so this check refuses to run the emission half there rather than + reporting 26 phantom diffs; +* **inert-variable** — `K=1` and an unrelated `PERRY_TOTALLY_UNRELATED=0` must + both reproduce the default object bit-for-bit. If they do not, the diff + signal is not attributable to the knob at all. + +## Why the owned-signal table is not simply the census keys + +Two representation sites exist that no census key counts, and both would make +rule 2 fire spuriously: + +* a specialized-ABI entry's `i32` parameter slot is canonical-i32 storage + (`codegen/function.rs`), but it is a parameter, so it is never `select()`ed + as a canonical slot; +* a proven-`this` receiver is a `Ptr` consumption that was never + selected either (the census reports it as `consumed_receiver`). + +So [`KNOB_SIGNALS`] names, per knob, everything that means "this representation +has a site here" — census keys plus those two derived quantities. Getting this +table wrong makes the gate red on a correct compiler, which is the failure mode +that gets a gate deleted; it is derived from the report, not guessed. +""" + +from __future__ import annotations + +import argparse +import hashlib +import platform +import re +import shutil +import tempfile +from concurrent.futures import ThreadPoolExecutor +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any + +from .capture import resolve_perry +from .common import HarnessError, REPO_ROOT +from .repsel_census import ( + CENSUS_KEYS, + DEFAULT_BASELINE, + compile_and_census, + load_baseline, +) + + +#: `SpecParamRep::label()` spelling for a canonical-i32 parameter slot. +SPEC_I32_LABEL = re.compile(r"^i32$") + +#: Derived signals, computed here rather than added to the census, so +#: `benchmarks/repsel_census/baseline.json` (which has conflicted repeatedly) +#: does not have to move for an instrument fix. +DERIVED_SIGNALS = ("spec-abi-i32-slot", "consumed-receiver") + + +@dataclass(frozen=True) +class Knob: + """One bisection knob, and everything it is allowed to move.""" + + env: str + #: Census keys this knob owns. Empty for a knob that is not a + #: representation at all (see `PERRY_STATIC_STRING_LOWERING`). + keys: tuple[str, ...] + #: Derived signals from [`DERIVED_SIGNALS`] that also mean "a site of this + #: representation exists in this workload". + signals: tuple[str, ...] = () + #: What the knob is, for the report. + what: str = "" + + @property + def owned(self) -> tuple[str, ...]: + return self.keys + self.signals + + +#: The knob table. Kept HERE and not in the baseline JSON, for the same reason +#: `LIVENESS_FLOORS` is: it is an assertion about the compiler, and no +#: `--update` path may widen it. +KNOBS: tuple[Knob, ...] = ( + Knob( + "PERRY_CANONICAL_I32_LOCALS", + ("canonical-i32", "canonical-u32"), + ("spec-abi-i32-slot",), + "repsel Phase 1 — canonical unboxed i32/u32 slots", + ), + Knob( + "PERRY_CANONICAL_STR_LOCALS", + ("canonical-str",), + (), + "repsel Phase 3a — canonical (tagged-at-rest) Str slots", + ), + Knob( + "PERRY_PTR_SHAPE_LOCALS", + ("ptr-shape", "ptr-shape-consumed"), + ("consumed-receiver",), + "repsel Phase 3b/5a — Ptr receivers", + ), + Knob( + "PERRY_PTR_NUMARRAY_LOCALS", + ("ptr-numarray",), + (), + "repsel Phase 4a.3 — Ptr locals", + ), + Knob( + "PERRY_INT_VALUED_LOCALS", + ("int-valued-ta",), + (), + "native-i32 residency for int-TA-seeded locals (#6898)", + ), + Knob( + "PERRY_STATIC_STRING_LOWERING", + (), + (), + "#7128 — string fast paths keyed on a value's STATIC string type. Not " + "a representation: it owns no census key, so it must move no count at " + "all, and rule 2 does not apply to it", + ), +) + +#: An env var the compiler must not read. The default object has to reproduce +#: bit-for-bit under it, or the object diff is measuring the environment rather +#: than the knob. +INERT_VAR = "PERRY_TOTALLY_UNRELATED" + + +@dataclass +class Arm: + """One (workload, env) compile.""" + + counts: dict[str, int] + signals: dict[str, int] + digest: str + objects: list[str] = field(default_factory=list) + + +def _spec_abi_i32_slots(report: dict[str, Any]) -> int: + """Count `i32` parameter slots across selected specialized-ABI entries. + + `codegen/function.rs` allocates those as canonical-i32 storage under + `PERRY_CANONICAL_I32_LOCALS`, and reverts them to a boxed double slot when + the knob is off. Nothing in the census counts them, so without this the + gate would demand a byte-identical object for a workload that legitimately + has canonical-i32 sites. + """ + total = 0 + for entry in report.get("entries", []): + if entry.get("analysis") != "spec-abi" or entry.get("outcome") != "selected": + continue + total += sum(1 for label in (entry.get("rep") or "").split(",") if SPEC_I32_LABEL.match(label.strip())) + return total + + +def _digest(paths: list[str]) -> str: + h = hashlib.sha256() + for path in sorted(paths): + h.update(Path(path).read_bytes()) + return h.hexdigest() + + +def _compile_arm( + perry: list[str], + source: Path, + env: dict[str, str], + *, + timeout: int, + workdir: Path, +) -> Arm: + census = compile_and_census( + perry, + source, + timeout=timeout, + extra_env=env, + object_out=workdir / "out.o", + with_report=True, + ) + report = census["report"] + return Arm( + counts={key: int(census["counts"].get(key, 0)) for key in CENSUS_KEYS}, + signals={ + "spec-abi-i32-slot": _spec_abi_i32_slots(report), + "consumed-receiver": int(census.get("consumed_receiver", 0)), + }, + digest=_digest(census["objects"]), + objects=list(census["objects"]), + ) + + +def _resolve_source(rel: str) -> Path: + path = Path(rel) + return path if path.is_absolute() else REPO_ROOT / path + + +def _arm_env(var: str | None, value: str = "0") -> dict[str, str]: + return {} if var is None else {var: value} + + +def check_isolation(args: argparse.Namespace) -> int: + baseline = load_baseline(Path(args.baseline) if args.baseline else DEFAULT_BASELINE) + perry = resolve_perry(args.perry) + workloads = baseline["workloads"] + if args.workload: + wanted = set(args.workload) + workloads = [w for w in workloads if w["name"] in wanted] + unknown = wanted - {w["name"] for w in workloads} + if unknown: + raise HarnessError(f"unknown census workload(s): {sorted(unknown)}") + knobs = KNOBS + if args.knob: + wanted = set(args.knob) + knobs = tuple(k for k in KNOBS if k.env in wanted) + unknown = wanted - {k.env for k in knobs} + if unknown: + raise HarnessError(f"unknown knob(s): {sorted(unknown)}") + + print("Representation-selection knob isolation (#7128)") + print("==============================================\n") + print(f"compiler: {' '.join(perry)}") + print(f"host: {platform.system()} {platform.machine()}") + print(f"corpus: {len(workloads)} workload(s), {len(knobs)} knob(s)\n") + + tmp = Path(tempfile.mkdtemp(prefix="repsel-knob-iso-")) + try: + # ── arm plan ────────────────────────────────────────────────────── + # `default` twice: the second copy is the determinism control. + arm_names: list[tuple[str, dict[str, str]]] = [ + ("default", {}), + ("default#2", {}), + (f"inert:{INERT_VAR}=0", _arm_env(INERT_VAR)), + ] + for knob in knobs: + arm_names.append((f"{knob.env}=0", _arm_env(knob.env))) + arm_names.append((f"{knob.env}=1", _arm_env(knob.env, "1"))) + + jobs: list[tuple[str, str, Path, dict[str, str], Path]] = [] + for workload in workloads: + source = _resolve_source(workload["source"]) + for arm, env in arm_names: + slug = re.sub(r"[^A-Za-z0-9]+", "_", arm) + jobs.append( + (workload["name"], arm, source, env, tmp / workload["name"] / slug) + ) + + def run(job: tuple[str, str, Path, dict[str, str], Path]) -> tuple[tuple[str, str], Arm]: + name, arm, source, env, workdir = job + return (name, arm), _compile_arm( + perry, source, env, timeout=args.compile_timeout, workdir=workdir + ) + + with ThreadPoolExecutor(max_workers=max(1, args.jobs)) as pool: + results: dict[tuple[str, str], Arm] = dict(pool.map(run, jobs)) + + return _verdict(workloads, knobs, results, args) + finally: + if not args.keep_objects: + shutil.rmtree(tmp, ignore_errors=True) + else: + print(f"\n(objects kept in {tmp})") + + +def _verdict( + workloads: list[dict[str, Any]], + knobs: tuple[Knob, ...], + results: dict[tuple[str, str], Arm], + args: argparse.Namespace, +) -> int: + names = [w["name"] for w in workloads] + + # ── control 1: determinism ──────────────────────────────────────────── + nondeterministic = [ + n for n in names if results[(n, "default")].digest != results[(n, "default#2")].digest + ] + emission_checkable = not nondeterministic + if nondeterministic: + print( + "OBJECT EMISSION IS NONDETERMINISTIC on this host: " + f"{len(nondeterministic)}/{len(names)} workload(s) compiled twice with " + "identical flags produced different bytes " + f"({', '.join(nondeterministic[:4])}{'…' if len(nondeterministic) > 4 else ''}).\n" + " Known cause on aarch64 Linux: the LLVM module name embeds pid +\n" + " nanotime and lands in the object. The emission half of this gate is\n" + " SKIPPED — it would report a diff for every arm. Run it on a host\n" + " where the compiler is deterministic (macOS today).\n" + ) + if args.require_emission: + print( + "--require-emission was passed, so a host that cannot compare objects " + "is a failure rather than a partial run." + ) + return 1 + + failures: list[str] = [] + notes: list[str] = [] + + # ── control 2: an inert variable must not move the object ───────────── + if emission_checkable: + for n in names: + if results[(n, f"inert:{INERT_VAR}=0")].digest != results[(n, "default")].digest: + failures.append( + f"CONTROL: {n} compiled differently with {INERT_VAR}=0 set, an env var " + "the compiler does not read. The object diff below is not attributable " + "to any knob." + ) + for knob in knobs: + for n in names: + if results[(n, f"{knob.env}=1")].digest != results[(n, "default")].digest: + failures.append( + f"CONTROL: {n} compiled differently with {knob.env}=1, which is the " + "default. The knob is keyed into codegen beyond its documented " + "off-state." + ) + + # ── rule 1 / rule 2, per knob ───────────────────────────────────────── + rows: list[str] = [] + for knob in knobs: + moved_counts = 0 + moved_objects = 0 + for n in names: + base = results[(n, "default")] + off = results[(n, f"{knob.env}=0")] + + lost = False + for key in CENSUS_KEYS: + if key in knob.keys: + lost = lost or off.counts[key] < base.counts[key] + continue + if off.counts[key] != base.counts[key]: + failures.append( + f"COUNT LEAK: {knob.env}=0 changed {key} on {n} " + f"({base.counts[key]} -> {off.counts[key]}). That key belongs to a " + "different representation, so any A/B through this knob measures " + "more than one." + ) + for sig in DERIVED_SIGNALS: + if sig in knob.signals: + lost = lost or off.signals[sig] < base.signals[sig] + continue + if off.signals[sig] != base.signals[sig]: + failures.append( + f"COUNT LEAK: {knob.env}=0 changed {sig} on {n} " + f"({base.signals[sig]} -> {off.signals[sig]})." + ) + + moved_counts += int(lost) + if not emission_checkable: + continue + differs = off.digest != base.digest + moved_objects += int(differs) + promotes = sum(base.counts[k] for k in knob.keys) + sum( + base.signals[s] for s in knob.signals + ) + # A knob that owns no census key is not a representation, so + # "promotes nothing" is true of every workload and rule 2 would + # forbid the knob from doing anything at all. Rule 1 (no count may + # move) plus rule 3 (it must still be live) are what constrain it. + if differs and promotes == 0 and knob.owned: + failures.append( + f"EMISSION LEAK: {knob.env}=0 changed the emitted object on {n}, which " + f"promotes nothing this knob owns ({', '.join(knob.owned) or 'no census key'}" + " = 0). The knob is reaching sites outside its own representation." + ) + rows.append( + f" {knob.env:<30} {moved_counts:>3} workload(s) lose a promotion, " + f"{moved_objects:>3} change the object" + ) + + # ── rule 3: the knob must be an instrument ──────────────────────── + if knob.keys and moved_counts == 0: + failures.append( + f"DEAD KNOB: {knob.env}=0 took no promotion of {', '.join(knob.keys)} away " + "anywhere in the corpus. Either the representation stopped firing or the " + "knob no longer reaches it; both make every A/B through it vacuous." + ) + if emission_checkable and moved_objects == 0: + failures.append( + f"DEAD KNOB: {knob.env}=0 left every object in the corpus byte-identical. " + "An arm that emits the same bytes as the default cannot be evidence about " + "anything." + ) + + print("Per-knob effect") + print("---------------") + for row in rows: + print(row) + print() + + if notes: + for note in notes: + print(note) + + if failures: + print("FAILURES:") + for line in failures: + print(f" {line}") + print() + print( + "Knob isolation FAILED. A knob that moves a representation it does not name " + "silently invalidates every measurement taken through it — see #7128." + ) + return 1 + + if not emission_checkable: + print( + "Count isolation OK on every knob. EMISSION isolation was not checked " + "(nondeterministic host)." + ) + return 0 + print("Knob isolation OK: every knob moves its own representation and nothing else.") + return 0 + + +def self_test(_args: argparse.Namespace) -> int: + """Prove the verdict logic can go red, without compiling anything. + + Both defects #7128 fixed are replayed here as synthetic arm tables, so the + two branches that catch them are exercised on every run rather than only on + a host with a compiler. + """ + ns = argparse.Namespace(require_emission=False) + workloads = [{"name": "w", "source": "x.ts"}, {"name": "v", "source": "y.ts"}] + + def arm(counts: dict[str, int], digest: str, **signals: int) -> Arm: + full = {key: 0 for key in CENSUS_KEYS} + full.update(counts) + sig = {s: 0 for s in DERIVED_SIGNALS} + sig.update(signals) + return Arm(counts=full, signals=sig, digest=digest) + + i32 = next(k for k in KNOBS if k.env == "PERRY_CANONICAL_I32_LOCALS") + strk = next(k for k in KNOBS if k.env == "PERRY_CANONICAL_STR_LOCALS") + + def table(default: dict[str, Arm], off: dict[str, Arm], knob: Knob) -> dict[tuple[str, str], Arm]: + out: dict[tuple[str, str], Arm] = {} + for name, a in default.items(): + out[(name, "default")] = a + out[(name, "default#2")] = a + out[(name, f"inert:{INERT_VAR}=0")] = a + out[(name, f"{knob.env}=1")] = a + for name, a in off.items(): + out[(name, f"{knob.env}=0")] = a + return out + + # Defect A, exactly as measured: the i32 knob takes `ptr-shape-consumed` + # from 1 to 0 while doing its own job on `canonical-i32`. + leak = table( + { + "w": arm({"canonical-i32": 5, "ptr-shape": 2, "ptr-shape-consumed": 1}, "aa"), + "v": arm({"canonical-i32": 2}, "bb"), + }, + { + "w": arm({"canonical-i32": 0, "ptr-shape": 2, "ptr-shape-consumed": 0}, "cc"), + "v": arm({"canonical-i32": 0}, "dd"), + }, + i32, + ) + verdict = _capture(_verdict, workloads, (i32,), leak, ns) + assert verdict.code == 1, verdict.out + assert "COUNT LEAK" in verdict.out and "ptr-shape-consumed" in verdict.out, verdict.out + + # Defect B: every count is untouched and the object still moves on a + # workload that selects no Str local at all. + emission = table( + {"w": arm({"canonical-str": 1}, "aa"), "v": arm({}, "bb")}, + {"w": arm({"canonical-str": 0}, "cc"), "v": arm({}, "zz")}, + strk, + ) + verdict = _capture(_verdict, workloads, (strk,), emission, ns) + assert verdict.code == 1, verdict.out + assert "EMISSION LEAK" in verdict.out, verdict.out + + # The fixed shape: same counts elsewhere, object changes only where the + # representation actually promotes. + clean = table( + {"w": arm({"canonical-str": 1}, "aa"), "v": arm({}, "bb")}, + {"w": arm({"canonical-str": 0}, "cc"), "v": arm({}, "bb")}, + strk, + ) + verdict = _capture(_verdict, workloads, (strk,), clean, ns) + assert verdict.code == 0, verdict.out + + # A knob that moves nothing is dead, not clean. + dead = table( + {"w": arm({"canonical-str": 1}, "aa"), "v": arm({}, "bb")}, + {"w": arm({"canonical-str": 1}, "aa"), "v": arm({}, "bb")}, + strk, + ) + verdict = _capture(_verdict, workloads, (strk,), dead, ns) + assert verdict.code == 1 and "DEAD KNOB" in verdict.out, verdict.out + + # An inert variable that moves the object means the diff is not the knob's. + contaminated = table( + {"w": arm({"canonical-str": 1}, "aa"), "v": arm({}, "bb")}, + {"w": arm({"canonical-str": 0}, "cc"), "v": arm({}, "bb")}, + strk, + ) + contaminated[("v", f"inert:{INERT_VAR}=0")] = arm({}, "qq") + verdict = _capture(_verdict, workloads, (strk,), contaminated, ns) + assert verdict.code == 1 and "CONTROL" in verdict.out, verdict.out + + # A nondeterministic host must skip the emission half, not fail it — and + # must still run the count half. + flaky = table( + {"w": arm({"canonical-str": 1}, "aa"), "v": arm({}, "bb")}, + {"w": arm({"canonical-str": 0}, "cc"), "v": arm({}, "zz")}, + strk, + ) + flaky[("w", "default#2")] = arm({"canonical-str": 1}, "AA") + verdict = _capture(_verdict, workloads, (strk,), flaky, ns) + assert verdict.code == 0 and "NONDETERMINISTIC" in verdict.out.upper(), verdict.out + strict = argparse.Namespace(require_emission=True) + verdict = _capture(_verdict, workloads, (strk,), flaky, strict) + assert verdict.code == 1, verdict.out + + # `PERRY_STATIC_STRING_LOWERING` owns no census key: it must move no count, + # and rule 2 must NOT demand a byte-identical object of it. + static = next(k for k in KNOBS if k.env == "PERRY_STATIC_STRING_LOWERING") + assert static.keys == () and static.signals == () + ok = table( + {"w": arm({"canonical-str": 1}, "aa"), "v": arm({}, "bb")}, + {"w": arm({"canonical-str": 1}, "cc"), "v": arm({}, "dd")}, + static, + ) + verdict = _capture(_verdict, workloads, (static,), ok, ns) + assert verdict.code == 0, verdict.out + moved = table( + {"w": arm({"canonical-str": 1}, "aa"), "v": arm({}, "bb")}, + {"w": arm({"canonical-str": 0}, "cc"), "v": arm({}, "dd")}, + static, + ) + verdict = _capture(_verdict, workloads, (static,), moved, ns) + assert verdict.code == 1 and "COUNT LEAK" in verdict.out, verdict.out + + print("repsel knob-isolation self-test OK") + return 0 + + +@dataclass +class _Captured: + code: int + out: str + + +def _capture(fn: Any, *fn_args: Any) -> _Captured: + import contextlib + import io + + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + code = fn(*fn_args) + return _Captured(code=code, out=buf.getvalue()) + + +__all__ = [ + "KNOBS", + "Knob", + "check_isolation", + "self_test", +] From 201b852976f080b863138ccd815d913d882cee77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 31 Jul 2026 10:57:45 +0200 Subject: [PATCH 3/5] test(repsel): model documented proof dependencies in the knob-isolation gate (#7128) PERRY_INT_VALUED_LOCALS=0 lowers canonical-i32 on fixture_int_valued_ta (3 -> 2), and that is legitimate: int_valued_ta_locals is merged into integer_locals, which is the candidate set canonical-i32 admission draws from, so turning the proof off removes the selection. Encoded as a documented, DOWNWARD-ONLY dependency rather than an exemption -- a knob that raises another representation's count is still a leak. Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- .../repsel_knob_isolation.py | 92 ++++++++++++++++++- 1 file changed, 89 insertions(+), 3 deletions(-) diff --git a/scripts/compiler_output_harness/repsel_knob_isolation.py b/scripts/compiler_output_harness/repsel_knob_isolation.py index 1fd2395b7a..8123bdece7 100644 --- a/scripts/compiler_output_harness/repsel_knob_isolation.py +++ b/scripts/compiler_output_harness/repsel_knob_isolation.py @@ -105,12 +105,22 @@ class Knob: #: Derived signals from [`DERIVED_SIGNALS`] that also mean "a site of this #: representation exists in this workload". signals: tuple[str, ...] = () + #: Keys this knob may legitimately take DOWN but never up, because the + #: analysis it gates FEEDS them. Each entry carries the reason; an + #: undocumented one is a leak, not a dependency. + #: + #: This is the one place the "a knob owns exactly one representation" rule + #: bends, and it bends for a real reason: a representation whose proof is + #: withdrawn cannot be selected. Allowing only the downward direction keeps + #: it from becoming a licence — a knob that ADDS promotions of another + #: representation is still a leak. + downstream: tuple[tuple[str, str], ...] = () #: What the knob is, for the report. what: str = "" @property def owned(self) -> tuple[str, ...]: - return self.keys + self.signals + return self.keys + self.signals + tuple(k for k, _ in self.downstream) #: The knob table. Kept HERE and not in the baseline JSON, for the same reason @@ -121,36 +131,52 @@ def owned(self) -> tuple[str, ...]: "PERRY_CANONICAL_I32_LOCALS", ("canonical-i32", "canonical-u32"), ("spec-abi-i32-slot",), + (), "repsel Phase 1 — canonical unboxed i32/u32 slots", ), Knob( "PERRY_CANONICAL_STR_LOCALS", ("canonical-str",), (), + (), "repsel Phase 3a — canonical (tagged-at-rest) Str slots", ), Knob( "PERRY_PTR_SHAPE_LOCALS", ("ptr-shape", "ptr-shape-consumed"), ("consumed-receiver",), + (), "repsel Phase 3b/5a — Ptr receivers", ), Knob( "PERRY_PTR_NUMARRAY_LOCALS", ("ptr-numarray",), (), + (), "repsel Phase 4a.3 — Ptr locals", ), Knob( "PERRY_INT_VALUED_LOCALS", ("int-valued-ta",), (), + ( + ( + "canonical-i32", + "`int_valued_ta_locals` is merged into `integer_locals` " + "(`collectors/hir_facts.rs`), which is the candidate set canonical-i32 " + "admission draws from. With the knob off the local is no longer PROVEN " + "integer, so canonical-i32 cannot select it — a withdrawn proof, not a " + "second representation being switched off. Measured on " + "`fixture_int_valued_ta`: canonical-i32 3 -> 2.", + ), + ), "native-i32 residency for int-TA-seeded locals (#6898)", ), Knob( "PERRY_STATIC_STRING_LOWERING", (), (), + (), "#7128 — string fast paths keyed on a value's STATIC string type. Not " "a representation: it owns no census key, so it must move no count at " "all, and rule 2 does not apply to it", @@ -359,10 +385,23 @@ def _verdict( off = results[(n, f"{knob.env}=0")] lost = False + downstream = dict(knob.downstream) for key in CENSUS_KEYS: if key in knob.keys: lost = lost or off.counts[key] < base.counts[key] continue + if key in downstream: + # Only the downward direction, and only with a reason on + # record. An UPWARD move means the knob is creating + # promotions of another representation, which no proof + # dependency can explain. + if off.counts[key] > base.counts[key]: + failures.append( + f"COUNT LEAK: {knob.env}=0 RAISED {key} on {n} " + f"({base.counts[key]} -> {off.counts[key]}). A withdrawn proof " + "can only remove promotions; this knob is adding them." + ) + continue if off.counts[key] != base.counts[key]: failures.append( f"COUNT LEAK: {knob.env}=0 changed {key} on {n} " @@ -385,8 +424,10 @@ def _verdict( continue differs = off.digest != base.digest moved_objects += int(differs) - promotes = sum(base.counts[k] for k in knob.keys) + sum( - base.signals[s] for s in knob.signals + promotes = ( + sum(base.counts[k] for k in knob.keys) + + sum(base.signals[s] for s in knob.signals) + + sum(base.counts[k] for k, _ in knob.downstream) ) # A knob that owns no census key is not a representation, so # "promotes nothing" is true of every workload and rule 2 would @@ -423,6 +464,16 @@ def _verdict( print(row) print() + documented = [(k, key, why) for k in knobs for key, why in k.downstream] + if documented: + print("Documented proof dependencies (a knob may only LOWER these)") + print("----------------------------------------------------------") + for knob, key, why in documented: + print(f" {knob.env}=0 may lower {key}:") + for line in _wrap(why): + print(f" {line}") + print() + if notes: for note in notes: print(note) @@ -448,6 +499,12 @@ def _verdict( return 0 +def _wrap(text: str, width: int = 74) -> list[str]: + import textwrap + + return textwrap.wrap(" ".join(text.split()), width=width) + + def self_test(_args: argparse.Namespace) -> int: """Prove the verdict logic can go red, without compiling anything. @@ -569,6 +626,35 @@ def table(default: dict[str, Arm], off: dict[str, Arm], knob: Knob) -> dict[tupl verdict = _capture(_verdict, workloads, (static,), moved, ns) assert verdict.code == 1 and "COUNT LEAK" in verdict.out, verdict.out + # A documented proof dependency may lower the downstream key and only that. + intk = next(k for k in KNOBS if k.env == "PERRY_INT_VALUED_LOCALS") + assert dict(intk.downstream).get("canonical-i32"), "the dependency must carry a reason" + down_ok = table( + {"w": arm({"int-valued-ta": 1, "canonical-i32": 3}, "aa"), "v": arm({}, "bb")}, + {"w": arm({"int-valued-ta": 0, "canonical-i32": 2}, "cc"), "v": arm({}, "bb")}, + intk, + ) + verdict = _capture(_verdict, workloads, (intk,), down_ok, ns) + assert verdict.code == 0, verdict.out + # …but never raise it. A knob that ADDS another representation's promotions + # is a leak no withdrawn proof can explain. + down_bad = table( + {"w": arm({"int-valued-ta": 1, "canonical-i32": 3}, "aa"), "v": arm({}, "bb")}, + {"w": arm({"int-valued-ta": 0, "canonical-i32": 4}, "cc"), "v": arm({}, "bb")}, + intk, + ) + verdict = _capture(_verdict, workloads, (intk,), down_bad, ns) + assert verdict.code == 1 and "RAISED" in verdict.out, verdict.out + # An UNDOCUMENTED cross-representation move is still a leak: the Str knob + # has no dependency on canonical-i32, so the identical shape must go red. + undocumented = table( + {"w": arm({"canonical-str": 1, "canonical-i32": 3}, "aa"), "v": arm({}, "bb")}, + {"w": arm({"canonical-str": 0, "canonical-i32": 2}, "cc"), "v": arm({}, "bb")}, + strk, + ) + verdict = _capture(_verdict, workloads, (strk,), undocumented, ns) + assert verdict.code == 1 and "COUNT LEAK" in verdict.out, verdict.out + print("repsel knob-isolation self-test OK") return 0 From e3f6f90464fa28121c5df3628ce35bd91f32b5d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 31 Jul 2026 11:01:50 +0200 Subject: [PATCH 4/5] docs(repsel): document the knob-isolation gate and exercise the new knob in the matrix (#7128) Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- benchmarks/repsel_census/README.md | 45 ++++++++++++++++++++++++++++ crates/perry-codegen/src/expr/mod.rs | 7 ++--- scripts/gc_repsel_matrix.sh | 1 + 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/benchmarks/repsel_census/README.md b/benchmarks/repsel_census/README.md index 6144a4a2b3..b52fe7e2f6 100644 --- a/benchmarks/repsel_census/README.md +++ b/benchmarks/repsel_census/README.md @@ -183,6 +183,51 @@ removing either mechanism recorder, counting per access site, folding proven-`this` consumption into the local column, and deleting the consumed liveness minimum each turn the gate red. +## Knob isolation (#7128) + +The census answers "how much does each representation promote". It cannot, on +its own, answer "is knob X evidence about representation X" — and for two of the +five knobs it was not: + +- `PERRY_CANONICAL_I32_LOCALS=0` also turned off **every `Ptr` + consumption**, because the four ordinary-body `FnCtx` construction sites + computed the `Ptr` context flag from the canonical-i32 env read. Census + under that knob read `ptr-shape: 7 selected, 0 consumed`. On this corpus it + moved the object on `batch`, `suite_09_method_calls` and + `fixture_ptr_shape_sites` for `Ptr` reasons alone. +- `PERRY_CANONICAL_STR_LOCALS=0` also turned off three lowerings that never + consult a selected `Str` local, so it changed the emitted object on **23 of + the 26** workloads — 20 of which promote no `canonical-str` at all. + +```bash +python3 scripts/compiler_output_regression.py census-knob-isolation \ + --perry --jobs 4 +``` + +Per knob, with that knob at `0` and every other at its default: + +1. no census key outside the knob's own may change; +2. a workload whose representation promotes nothing must emit a + **byte-identical** object; +3. the knob must still be live — take a promotion away somewhere, and change + some object. + +Rule 1 catches the first defect, rule 2 the second (it leaves every count +untouched). Two controls guard the diff: the compiler must be deterministic +(**it is not on aarch64 Linux** — the LLVM module name embeds pid + nanotime, so +the emission half is skipped there rather than reporting 26 phantoms), and both +`X=1` and an env var the compiler does not read must reproduce the default +object bit-for-bit. + +One documented exception, downward only: `PERRY_INT_VALUED_LOCALS=0` lowers +`canonical-i32` on `fixture_int_valued_ta` (3 → 2), because +`int_valued_ta_locals` is merged into `integer_locals`, the candidate set +canonical-i32 draws from. A withdrawn proof cannot be selected. A knob that +*raises* another representation's count is still a leak. + +`--jobs` compiles arms in parallel; the whole corpus × 6 knobs × 4 arms is about +20 s on an M1. + ## Editing the fixtures Don't tidy them. Every one is written against a specific collector's rules and diff --git a/crates/perry-codegen/src/expr/mod.rs b/crates/perry-codegen/src/expr/mod.rs index b98988b0fa..279f205ccd 100644 --- a/crates/perry-codegen/src/expr/mod.rs +++ b/crates/perry-codegen/src/expr/mod.rs @@ -149,10 +149,9 @@ pub(crate) use repsel_gates::{static_string_lowering_enabled, RepselContextFlags pub(crate) use slot_rep::{ canonical_i32_locals_enabled, canonical_local_i32_slot, canonical_str_locals_enabled, collect_canonical_str_ineligible_locals, collect_closure_referenced_locals, - deny_canonical_context, deny_canonical_i32, load_canonical_local_boxed, - local_is_canonical_str, local_rep_is_canonical_i32, note_canonical_local, - ptr_shape_context_rule_text, store_canonical_local_from_double, CanonicalI32Denial, SlotRep, - PTR_SHAPE_SCALAR_REPLACED, + deny_canonical_context, deny_canonical_i32, load_canonical_local_boxed, local_is_canonical_str, + local_rep_is_canonical_i32, note_canonical_local, ptr_shape_context_rule_text, + store_canonical_local_from_double, CanonicalI32Denial, SlotRep, PTR_SHAPE_SCALAR_REPLACED, }; pub(crate) use dispatch::{lower_expr, lower_math_operand}; diff --git a/scripts/gc_repsel_matrix.sh b/scripts/gc_repsel_matrix.sh index 2377a52306..c84d478e78 100755 --- a/scripts/gc_repsel_matrix.sh +++ b/scripts/gc_repsel_matrix.sh @@ -179,6 +179,7 @@ ARMS=( "loop_polls|PERRY_GC_MOVING_LOOP_POLLS=1|%P% %E% PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_FORCE_EVACUATE=1|move|defer the alloc-point collection to a loop back-edge precise-root safepoint, where the copying minor may MOVE survivors" "rep_i32_off|PERRY_CANONICAL_I32_LOCALS=0|%P% %E% PERRY_GC_FORCE_EVACUATE=1|move|repsel Phase 1 OFF x evacuation" "rep_str_off|PERRY_CANONICAL_STR_LOCALS=0|%P% %E% PERRY_GC_FORCE_EVACUATE=1|move|repsel Phase 3a OFF x evacuation" +"rep_str_static_off|PERRY_STATIC_STRING_LOWERING=0|%P% %E% PERRY_GC_FORCE_EVACUATE=1|move|#7128 static-string lowerings OFF x evacuation -- the inline StringRef retag, the proven-heap operand handle and the tag-dispatched .length. Split off PERRY_CANONICAL_STR_LOCALS because they key on a value's static string type, not on a selected Str local; this arm is what keeps the off-state exercised." "rep_ptr_shape_off|PERRY_PTR_SHAPE_LOCALS=0|%P% %E% PERRY_GC_FORCE_EVACUATE=1|move|repsel Phase 3b OFF x evacuation" "rep_ptr_numarray_off|PERRY_PTR_NUMARRAY_LOCALS=0|%P% %E% PERRY_GC_FORCE_EVACUATE=1|move|repsel Phase 4a.3 OFF x evacuation" "rep_spec_abi_off|PERRY_SPECIALIZED_ABI=0|%P% %E% PERRY_GC_FORCE_EVACUATE=1|move|repsel Phase 2 OFF x evacuation" From 6955e13a18627fd263d7608d1f7c8daccab804fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 31 Jul 2026 11:13:42 +0200 Subject: [PATCH 5/5] docs: changelog fragment for #7128 knob isolation (#7133) Claude-Session: https://claude.ai/code/session_018ZFER8EEg8K7ez2n6oDrT9 --- changelog.d/7133-repsel-knob-isolation.md | 61 +++++++++++++++++++ crates/perry-codegen/src/expr/repsel_gates.rs | 4 +- 2 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 changelog.d/7133-repsel-knob-isolation.md diff --git a/changelog.d/7133-repsel-knob-isolation.md b/changelog.d/7133-repsel-knob-isolation.md new file mode 100644 index 0000000000..0ea878e791 --- /dev/null +++ b/changelog.d/7133-repsel-knob-isolation.md @@ -0,0 +1,61 @@ +### Fixed + +- **Representation-selection bisection knobs now move exactly one representation + each (#7128).** Two of them did not, which is worse than an ordinary bug: + every knob-based A/B taken through them measured two things and attributed the + sum to one. + + - `PERRY_CANONICAL_I32_LOCALS=0` **also disabled every `Ptr` + consumption.** Phase 5a reused `repsel_context_allows_canonical_i32` as its + context gate; #7121 split the `FnCtx` field but left the four ordinary-body + construction sites (`codegen/function.rs`, `method.rs` ×2, `closure.rs`) + initialising both fields from one `repsel_allows` bool whose first conjunct + was the canonical-i32 env read. Census under that knob read `ptr-shape: 7 + selected, 0 consumed`, with all six consumption sites printing + `NEVER FIRES`. + - `PERRY_CANONICAL_STR_LOCALS=0` **was not scoped to `Str` locals.** Three + Phase 3a lowerings key on a value's static string type and never on a + selected local — the inline `StringRef` retag + (`native_value/materialize.rs`), the proven-heap operand arm of + `str_operand_handle_tag_dispatched` (`lower_string_method.rs`), and the + tag-dispatched `.length` (`expr/property_get.rs`). They changed the emitted + object on 23 of the 26 census workloads, 20 of which promote no + `canonical-str` at all. + + New `expr::repsel_gates` holds the knob table and a pure + `RepselGates -> RepselContextFlags` derivation that all six `FnCtx` + construction sites go through, so "one knob moves one flag" is a unit-testable + property instead of a convention. The three static-string lowerings move to + their own `PERRY_STATIC_STRING_LOWERING` (keyed into the object cache, with a + `gc_repsel_matrix.sh` arm keeping its off-state exercised). + + **The default build is byte-identical**: 26/26 census workloads, same census + counts, on both compilers. The `Str` split is an exact partition — base with + `PERRY_CANONICAL_STR_LOCALS=0` and this build with that knob *plus* + `PERRY_STATIC_STRING_LOWERING=0` emit byte-identical objects on 26/26. The + `Ptr`, `Ptr` and int-valued-TA knob arms are 26/26 identical + too. + +### Added + +- **`census-knob-isolation`** (`scripts/compiler_output_regression.py`) — a gate + for the property every knob-based A/B silently assumes. Per knob, with that + knob at `0`: no census key outside its own may change; a workload whose + representation promotes nothing must emit a **byte-identical** object; and the + knob must still be live (take a promotion away somewhere, change some object). + Two controls guard the diff — the compiler must be deterministic, and both + `X=1` and an env var the compiler does not read must reproduce the default + object bit-for-bit. + + It fails on `main` (7 count leaks, 20 emission leaks) and passes here. One + documented, downward-only exception: `PERRY_INT_VALUED_LOCALS=0` lowers + `canonical-i32` by one on `fixture_int_valued_ta`, because + `int_valued_ta_locals` is merged into `integer_locals` — a withdrawn proof + cannot be selected. A knob that *raises* another representation's count is + still a leak. + + Object emission is **nondeterministic on aarch64 Linux** (the LLVM temp module + name embeds pid + nanotime and lands in the ELF object — filed as #7131), so + the emission half detects the host and skips rather than reporting 26 phantom + diffs. `--require-emission` turns that into a failure where determinism is + expected. diff --git a/crates/perry-codegen/src/expr/repsel_gates.rs b/crates/perry-codegen/src/expr/repsel_gates.rs index 4ae4431d51..238914e954 100644 --- a/crates/perry-codegen/src/expr/repsel_gates.rs +++ b/crates/perry-codegen/src/expr/repsel_gates.rs @@ -58,8 +58,8 @@ //! depends on a local having been selected. They keep a kill switch of their //! own rather than riding on `Str`'s, so that `PERRY_CANONICAL_STR_LOCALS` //! means what it says. `PERRY_PTR_SHAPE_THIS` is a sub-knob of -//! `PERRY_PTR_SHAPE_LOCALS` (Phase 5a's `__pshape` clones only) and is honoured -//! by the same collector. +//! `PERRY_PTR_SHAPE_LOCALS` (Phase 5a's proven-`this` method clones only) and is +//! honoured by the same collector. use super::slot_rep::{ body_context_denial, canonical_i32_locals_enabled, canonical_str_locals_enabled,