jit: fix the jitstats bimodality root and its follow-ups

majit/majit-backend-wasm/src/codegen.rs

@@ -3168,24 +3168,6 @@ fn build_function(
     let region_spans = InlinedRegionSpan::collect(ops.len(), inlined_bridges);
     let liveness = HomeLiveness::collect_with_regions(inputargs, ops, &region_spans);
 
-    // `LOAD_FROM_GC_TABLE` is the backend form of a ConstPtr.  Native PyPy
-    // keeps such loop-invariant references in their allocated location across
-    // a loop; reloading the GC-table slot on every iteration is not part of
-    // the operation's semantics.  Do the same for a trace with no collecting
-    // operation: eager loads are safe, and no moving collection can stale the
-    // local before the trace exits.  Traces containing a call/allocation keep
-    // the original program points and the ordinary home/reload machinery.
-    let hoisted_gc_table_loads: indexmap::IndexSet<OpRef> =
-        if has_loop && !ops.iter().any(|op| op.opcode.can_malloc()) {
-            ops.iter()
-                .filter(|op| op.opcode == OpCode::LoadFromGcTable)
-                .map(|op| op.pos.get())
-                .filter(|result| *result != OpRef::NONE && !result.is_constant())
-                .collect()
-        } else {
-            indexmap::IndexSet::new()
-        };
-
     // Resume-at-LABEL: a peeled loop wraps its preamble in a dispatch so a
     // loop-closing bridge can re-enter AT any LABEL — key = label ordinal + 1
     // — skipping the code before it, in-module instead of round-tripping
@@ -3331,23 +3313,6 @@ fn build_function(
         }
     }
 
-    // Seed loop-invariant GC-table references after the fresh-entry home clear
-    // and input setup.  Store-on-def is mirrored here because the original op
-    // arm and its common tail are skipped below.
-    for result in &hoisted_gc_table_loads {
-        emit_seed_gc_table_ref(
-            &mut sink,
-            ops,
-            constants,
-            value_types,
-            ref_homes,
-            frame,
-            gc_table_base,
-            gc_table_bases,
-            *result,
-        );
-    }
-
     // Seed with the fail-index base so each guard/finish exit writes
     // `base + local` into `frame[0]` (every trace passes the next free index
     // of the global fail-index space, `failguard::fail_descr_base`). The local
@@ -3427,25 +3392,6 @@ fn build_function(
                     sink.i64_store(mem64(frame.home_slot_base + h as u64 * SLOT_SIZE));
                 }
             }
-            // The dispatch branch skipped the eager ConstPtr loads emitted on
-            // key 0.  Their ordinary homes are the low prefix a chained bridge
-            // clears and remaps for its own Refs, so what sits there on resume
-            // may be zero or another trace's object; the gc_table slot is the
-            // root the collector forwards in place, so read it again exactly
-            // as fresh entry does.  LABEL entry is cold enough to pay for it.
-            for result in &hoisted_gc_table_loads {
-                emit_seed_gc_table_ref(
-                    &mut sink,
-                    ops,
-                    constants,
-                    value_types,
-                    ref_homes,
-                    frame,
-                    gc_table_base,
-                    gc_table_bases,
-                    *result,
-                );
-            }
             sink.end(); // end B_j $past_loader
             labels_passed += 1;
         }
@@ -4846,7 +4792,7 @@ fn build_function(
                 // address; the collector forwards the slot in place, so the load
                 // reads the reference at its current address.
                 let vi = op.pos.get().raw();
-                if !OpRef::raw_is_constant(vi) && !hoisted_gc_table_loads.contains(&op.pos.get()) {
+                if !OpRef::raw_is_constant(vi) {
                     let index = resolve_const_bits(constants, op.arg(0).to_opref());
                     let base = gc_table_bases.get(&vi).copied().unwrap_or(gc_table_base);
                     let slot =
@@ -6174,9 +6120,7 @@ fn build_function(
         // skipped. Each value-producing arm is operand-stack-neutral, so this
         // appended store is balanced.
         let result = op.pos.get();
-        if !hoisted_gc_table_loads.contains(&result)
-            && let Some(h) = ref_homes.home(result)
-        {
+        if let Some(h) = ref_homes.home(result) {
             sink.local_get(0);
             sink.local_get(value_types.local(result.raw()));
             sink.i64_store(mem64(frame.home_slot_base + h as u64 * SLOT_SIZE));
@@ -6449,49 +6393,6 @@ fn resolve_const_bits(constants: &indexmap::IndexMap<u32, i64>, opref: OpRef) ->
     })
 }
 
-/// Load one hoisted `LoadFromGcTable` result from its table slot into its
-/// local and refresh the ordinary Ref home the in-loop reload path reads.
-///
-/// The table slot is the root the collector forwards in place
-/// (`assembler.py:1545 genop_load_from_gc_table`), which is why both the
-/// fresh-entry seeding and the LABEL resume loader read it rather than a home.
-#[allow(clippy::too_many_arguments)]
-fn emit_seed_gc_table_ref(
-    sink: &mut PeepSink<'_, '_>,
-    ops: &[Op],
-    constants: &indexmap::IndexMap<u32, i64>,
-    value_types: &ValueLocals,
-    ref_homes: &RefHomes,
-    frame: FrameGeometry,
-    gc_table_base: u32,
-    gc_table_bases: &HashMap<u32, u32>,
-    result: OpRef,
-) {
-    let producer = ops
-        .iter()
-        .find(|op| op.pos.get() == result)
-        .expect("hoisted GC-table result must have a producer");
-    let index = resolve_const_bits(constants, producer.arg(0).to_opref());
-    let base = gc_table_bases
-        .get(&result.raw())
-        .copied()
-        .unwrap_or(gc_table_base);
-    let slot = base as u64 + index as u64 * std::mem::size_of::<majit_ir::GcRef>() as u64;
-    sink.i32_const(slot as i32);
-    sink.i32_load(MemArg {
-        offset: 0,
-        align: 2,
-        memory_index: 0,
-    });
-    sink.i64_extend_i32_u();
-    sink.local_set(value_types.local(result.raw()));
-    if let Some(h) = ref_homes.home(result) {
-        sink.local_get(0);
-        sink.local_get(value_types.local(result.raw()));
-        sink.i64_store(mem64(frame.home_slot_base + h as u64 * SLOT_SIZE));
-    }
-}
-
 fn emit_resolve(
     sink: &mut PeepSink<'_, '_>,
     constants: &indexmap::IndexMap<u32, i64>,

majit/majit-backend-wasm/tests/codegen_test.rs

@@ -2342,8 +2342,16 @@ fn test_single_label_peeled_loop_validates() {
     assert!(!guards[0].is_finish);
 }
 
+/// A `LoadFromGcTable` placed inside the loop body is emitted inside the loop.
+///
+/// `rewrite.py:1100-1115 remove_constptr` caches one load per gc-table index,
+/// but `rewrite.py:1003-1006 emit_label` clears `gcrefs_recently_loaded` at
+/// every LABEL, so a reference constant used after the LABEL is loaded again on
+/// each iteration. The comment there rejects keeping the value alive across the
+/// label ("don't spill it") as "the wrong level" — the backend emits the op
+/// where the trace puts it and leaves that decision to the optimizer.
 #[test]
-fn loop_invariant_gc_table_load_stays_outside_non_collecting_loop() {
+fn gc_table_load_inside_a_loop_body_is_emitted_inside_the_loop() {
     let inputargs = vec![InputArg::from_type(Type::Int, 0)];
     let ops = vec![
         make_op(
@@ -2435,16 +2443,16 @@ fn loop_invariant_gc_table_load_stays_outside_non_collecting_loop() {
             }
         }
     }
-    // Without these two, the zero above also holds for a body that emitted no
-    // loop at all, or dropped the table load entirely.
+    // Without this, the counts below also hold for a body that emitted no loop
+    // at all.
     assert!(saw_loop, "codegen emitted no loop for a looping trace");
-    assert!(
-        loads_outside_loop > 0,
-        "the hoisted ConstPtr table slot is never loaded"
+    assert_eq!(
+        loads_inside_loop, 1,
+        "the in-loop LoadFromGcTable must be emitted inside the loop"
     );
     assert_eq!(
-        loads_inside_loop, 0,
-        "ConstPtr table slot was reloaded on the hot backedge"
+        loads_outside_loop, 0,
+        "no gc-table load belongs outside the loop for this trace"
     );
 }
 

majit/majit-metainterp/src/blackhole.rs

@@ -6066,7 +6066,15 @@ fn bhimpl_hint_force_virtualizable(_r: i64) {}
 /// because the value is a distinct compile-time constant per instruction and
 /// `check_result`'s 256-entry per-kind cap rejects one pool entry per
 /// instruction — registers this hook and writes the field itself.
-pub type LiveMarkerHook = fn(&BlackholeInterpreter, usize);
+/// The hook also owns the register file, because the marker names the live
+/// set: `cleanup_registers` (`blackhole.py:385`) clears `registers_r` "to
+/// avoid keeping references alive", but it only runs at `release_interp`
+/// (`blackhole.py:253`), so a register whose live range ended keeps its
+/// object for the rest of the run. RPython is insulated by liverange-based
+/// colouring reusing that register almost immediately
+/// (`rpython/tool/algo/regalloc.py:28-75`); a codewriter whose colours are
+/// not reused that densely needs the same clear at marker granularity.
+pub type LiveMarkerHook = fn(&mut BlackholeInterpreter, usize);
 
 static LIVE_MARKER_HOOK: std::sync::OnceLock<LiveMarkerHook> = std::sync::OnceLock::new();
 

majit/majit-translate/src/codewriter/assembler.rs

@@ -4427,6 +4427,18 @@ fn vable_arraydescrof(
         base_size: crate::layout::target_word_size(),
         itemsize,
         len_offset: Some(0),
+        // No layout identity, and neither zero is a placeholder to fill in.
+        // `PyFrame.locals_cells_stack_w` is reached through two allocators —
+        // `alloc_frame_locals_array`'s GC arm stamps the object-array tid into
+        // the header, its `alloc_fixed_array_with_header` arm (taken for a
+        // frame the collector does not own, and as the GC arm's own
+        // out-of-memory fallback) leaves the prepended header zeroed — so one
+        // tid cannot describe every block a trace will meet. Stamping either
+        // slot would put a `GUARD_GC_TYPE` on the short-preamble entry that is
+        // false for the other arm's blocks. `ArrayPtrInfo::make_guards`
+        // (`optimizeopt/info.rs`) instead refuses to build the entry, which
+        // costs the one unrolled attempt `unroll_free_retry_rescued` counts and
+        // keeps the guard honest.
         type_id: 0,
         gc_type_id: 0,
         item_type,

pyre/bench/synth/nested_for_outer_local_postread.cranelift.jitstats

@@ -1,4 +1,4 @@
-bridges_compiled=9
+bridges_compiled=8
 descr_set_absent=0
 descr_set_ambiguous=0
 descr_set_stale_absent=0
@@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0
 fbw_store_journal_rollback_failed=0
 field_pos_attached_misplaced=0
 field_pos_spec_misplaced=0
-guard_failures=2141
+guard_failures=2113
 internal_compile_panics=0
 loops_aborted=0
 loops_compiled=5
+retraces_compiled=0

pyre/bench/synth/nested_for_outer_local_postread.dynasm.jitstats

@@ -1,4 +1,4 @@
-bridges_compiled=9
+bridges_compiled=8
 descr_set_absent=0
 descr_set_ambiguous=0
 descr_set_stale_absent=0
@@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0
 fbw_store_journal_rollback_failed=0
 field_pos_attached_misplaced=0
 field_pos_spec_misplaced=0
-guard_failures=2141
+guard_failures=2113
 internal_compile_panics=0
 loops_aborted=0
 loops_compiled=5
+retraces_compiled=0

pyre/bench/synth/nested_for_outer_local_postread.wasm.jitstats

@@ -1,4 +1,4 @@
-bridges_compiled=9
+bridges_compiled=8
 descr_set_absent=0
 descr_set_ambiguous=0
 descr_set_stale_absent=0
@@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0
 fbw_store_journal_rollback_failed=0
 field_pos_attached_misplaced=0
 field_pos_spec_misplaced=0
-guard_failures=2141
+guard_failures=2113
 internal_compile_panics=0
 loops_aborted=0
 loops_compiled=5
+retraces_compiled=0

pyre/pyre-interpreter/src/jit_fnaddr.rs

@@ -3563,17 +3563,27 @@ mod tests {
         for (path, addr) in jit_trace_fnaddrs() {
             by_addr.entry(addr).or_default().push(path);
         }
+        // Collect every colliding address before failing. Asserting inside
+        // the loop reports whichever collision the hash order reached first
+        // and hides the rest, so each repair looks complete and the next run
+        // names a different pair.
+        let mut collisions: Vec<String> = Vec::new();
         for (addr, paths) in &by_addr {
             let leaves: std::collections::BTreeSet<&str> = paths
                 .iter()
                 .map(|p| p.rsplit("::").next().unwrap_or(p))
                 .collect();
-            assert_eq!(
-                leaves.len(),
-                1,
-                "fnaddr {addr:#x} is claimed by unrelated functions {paths:?}",
-            );
+            if leaves.len() > 1 {
+                collisions.push(format!("{addr:#x} {leaves:?}"));
+            }
         }
+        collisions.sort();
+        assert!(
+            collisions.is_empty(),
+            "{} fnaddr(s) claimed by unrelated functions:\n  {}",
+            collisions.len(),
+            collisions.join("\n  "),
+        );
     }
 
     #[test]

pyre/pyre-interpreter/src/pyopcode.rs

@@ -1818,6 +1818,7 @@ pub fn label_arg_to_usize(
     delta: crate::bytecode::Arg<crate::bytecode::oparg::Label>,
     op_arg: OpArg,
 ) -> usize {
+    keep_fnaddr_distinct(2);
     delta.get(op_arg).as_usize()
 }
 
@@ -1844,6 +1845,7 @@ pub fn jump_target_forward_from_oparg(
     next_instr: usize,
     op_arg: OpArg,
 ) -> usize {
+    keep_fnaddr_distinct(3);
     jump_target_forward(&code.instructions, next_instr, op_arg_as_usize(op_arg))
 }
 
@@ -1919,13 +1921,33 @@ pub fn convert_value_arg(
     conv.get(op_arg)
 }
 
+/// Materialise `tag` behind an optimisation barrier so the caller's machine
+/// code carries an immediate no sibling shares.
+///
+/// The decode helpers below differ only in the phantom type of their `Arg<T>`
+/// parameter, so several of them compile to byte-identical bodies. Each is a
+/// residual-call target whose address `jit_fnaddr.rs` registers, and
+/// `runtime_fnaddr_patch` re-pairs a build-time address with the runtime one
+/// by that address alone — two functions folded onto a single address make
+/// that pairing ambiguous and can send one callee's call to the other. A
+/// linker that folds identical code (MSVC `/OPT:ICF`, on by default) is what
+/// performs the fold, and which pair it picks moves with unrelated layout
+/// changes, so the bodies have to differ by construction rather than by luck.
+/// `drain_list_append` keeps its `#[inline(never)]` forwarding call for the
+/// same reason; these have no callee to forward to, so they carry a datum.
+#[inline(always)]
+fn keep_fnaddr_distinct(tag: u32) {
+    std::hint::black_box(tag);
+}
+
 /// Decode `LOAD_SPECIAL`'s enum oparg behind a first-party helper.
 #[inline]
 #[majit_macros::dont_look_inside]
 pub fn special_method_arg(
     method: crate::bytecode::Arg<crate::bytecode::oparg::SpecialMethod>,
     op_arg: OpArg,
 ) -> SpecialMethod {
+    keep_fnaddr_distinct(1);
     method.get(op_arg)
 }
 

pyre/pyre-jit-trace/src/descr.rs

@@ -1907,6 +1907,19 @@ static SPECIALISED_TUPLE_OO_DESCR_GROUP: LazyLock<PyreObjectDescrGroup> = LazyLo
     )
 });
 
+/// The `[capacity][items…]` header shared by every list/tuple backing block.
+///
+/// The `0` type id is load-bearing, not a slot waiting to be filled:
+/// `items_block_capacity_descr()` is the capacity read for all three list
+/// strategies, and their blocks carry three different runtime tids
+/// (`GC_INT_ARRAY_GC_TYPE_ID`, `GC_FLOAT_ARRAY_GC_TYPE_ID`,
+/// `PY_OBJECT_ARRAY_GC_TYPE_ID` — see the three arms of
+/// `helpers::emit_promote_empty_list_inline`). One descr fronting three tids
+/// can name none of them, so `StructPtrInfo::make_guards` (`optimizeopt/info.rs`)
+/// declines the short-preamble entry rather than pin a layout that holds for
+/// one strategy and not the other two; `unroll_free_retry_rescued` counts the
+/// unrolled attempt that costs. Stamping any single tid here makes the guard
+/// false for the other two block kinds.
 static ITEMS_BLOCK_DESCR_GROUP: LazyLock<PyreObjectDescrGroup> = LazyLock::new(|| {
     build_object_descr_group_with_def_path(
         pyre_object::object_array::ITEMS_BLOCK_ITEMS_OFFSET,