From 9e7001f13f06c3c02311c139b82cbdd83e1fa9e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 30 Aug 2026 21:56:00 +0200 Subject: [PATCH 1/2] codegen: name the packed-loop admission's rejection reason under a trace flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whether a counted loop gets its packed-f64 clone is a chain of independent conditions, and when one unexpectedly stays on the generic path there is no way to tell WHICH condition declined it except by reading the code and guessing. That is not hypothetical. Three separate attempts on #9151 each guessed a different gate and each was wrong in a different way: the HIR statement predicate, then the emitted-block call scan, then a block-region property. Two of those were built, measured as no-ops, and reverted. The information needed to skip all three was a single string. `PERRY_PACKED_LOOP_TRACE=1` prints it: the eight rejection points in `match_packed_f64_versioned_loop` each name themselves, and the post-emission `fast_call_free` check gets its own line — that one matters because it is reached when the matcher ADMITTED the loop and an emitted block carries a call anyway, so the clone is built and never entered. That case is invisible in the matcher's own reasons, which is exactly the confusion that cost the three attempts. Off by default and read once through a `OnceLock`, so it costs a branch on an already-cold path and nothing at all when unset. It has already earned its place. On the `#9151` shape it fires `body_not_admissible` for a loop containing `try` (correct), and stays SILENT for `throw new Error(…)` — which proves both the matcher and the array-loop call-free scan innocent for that case and moves the search to whatever decides it later. A silence that rules out two suspects is worth more than three more guesses, but only because the control run proves the trace fires when it should. perry-codegen 1842/0, unchanged from main; `-D warnings` clean. Claude-Session: https://claude.ai/code/session_01F1dt1jfzK2cheMZyus6y6p --- crates/perry-codegen/src/stmt/loops.rs | 39 ++++++++++++++++++++------ 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index 9193117f97..29b69f49f4 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -4103,6 +4103,13 @@ fn lower_object_array_write_versioned_for( let fast_call_free = (fast_scan_start..ctx.func.num_blocks()) .all(|idx| !ctx.func.blocks()[idx].contains_gc_unsafe_call()); + if !fast_call_free { + // Reached when the matcher ADMITTED the loop but an emitted block in + // the clone carries a call, so the clone is built and never entered. + // That is invisible from the matcher's own rejection reasons, which is + // why it gets its own trace line. + let _ = packed_loop_reject("fast_clone_not_call_free"); + } ctx.current_block = preheader_idx; let mut guard_ok = ctx.block().icmp_ne(I64, &packed_slots, "0"); for (g_packed, _) in &extra_guards { @@ -4753,6 +4760,22 @@ fn record_loop_array_length_effect( ); } +/// Diagnostic for `match_packed_f64_versioned_loop`'s rejection points. +/// +/// The admission decision is a chain of independent conditions, and when a loop +/// unexpectedly stays on the generic path there is no way to tell WHICH one +/// declined it by reading the code — three separate attempts on #9151 each +/// found a different gate by guessing. `PERRY_PACKED_LOOP_TRACE=1` prints the +/// reason instead, turning that into one run. +fn packed_loop_reject(reason: &'static str) -> Option { + use std::sync::OnceLock; + static ON: OnceLock = OnceLock::new(); + if *ON.get_or_init(|| std::env::var("PERRY_PACKED_LOOP_TRACE").as_deref() == Ok("1")) { + eprintln!("[packed-loop] rejected: {reason}"); + } + None +} + fn match_packed_f64_versioned_loop( ctx: &FnCtx<'_>, init: Option<&perry_hir::Stmt>, @@ -4761,7 +4784,7 @@ fn match_packed_f64_versioned_loop( body: &[Stmt], ) -> Option { if !ctx.pending_labels.is_empty() { - return None; + return packed_loop_reject("pending_labels"); } let ordinary_hoist = condition.and_then(|cond| classify_for_length_hoist(ctx, cond, update, body)); @@ -4769,13 +4792,13 @@ fn match_packed_f64_versioned_loop( condition.and_then(|cond| classify_for_length_hoist_impl(ctx, cond, update, body, true)) })?; if !matches!(hoist.op, perry_hir::CompareOp::Lt) || hoist.lhs_addend != 0 { - return None; + return packed_loop_reject("compare_op_not_lt"); } if !ctx.integer_locals.contains(&hoist.counter_id) || !loop_counter_bounds_are_safe(ctx, hoist.counter_id, update, body) || !loop_counter_entry_i32_range_is_safe(init, hoist.counter_id) { - return None; + return packed_loop_reject("counter_not_integer"); } let store_array_kind = supported_packed_numeric_loop_store_kind(ctx, body, hoist.arr_id, hoist.counter_id); @@ -4794,7 +4817,7 @@ fn match_packed_f64_versioned_loop( // loop; call-free read bodies now qualify by the argument above. Every // other body keeps the ordinary materialization-hazard gate. if ordinary_hoist.is_none() && store_array_kind.is_none() && !read_body_is_safe { - return None; + return packed_loop_reject("body_not_admissible"); } let binding_is_eligible = if store_array_kind.is_some() || read_body_is_safe { // A helper call that produced the binding marks it with the @@ -4809,7 +4832,7 @@ fn match_packed_f64_versioned_loop( packed_loop_array_binding_is_eligible(ctx, hoist.arr_id) }; if !binding_is_eligible { - return None; + return packed_loop_reject("binding_not_eligible"); } let array_kind = if let Some(store_array_kind) = store_array_kind { // The accepted store body is exactly `arr[i] = ` @@ -4837,14 +4860,14 @@ fn match_packed_f64_versioned_loop( // non-packed array fails the guard into the slow clone.) PackedNumericLoopKind::F64 } else { - return None; + return packed_loop_reject("array_kind_unknown"); }; if !local_is_number_array(ctx, hoist.arr_id) { - return None; + return packed_loop_reject("guard_emit_declined"); } let body_is_supported = store_array_kind.is_some() || read_body_is_safe; if !body_is_supported { - return None; + return packed_loop_reject("clone_not_call_free"); } Some(PackedF64VersionedLoop { counter_id: hoist.counter_id, From 9962eaad3047dbac516a6fea0854907af92c5969 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 30 Aug 2026 23:13:38 +0200 Subject: [PATCH 2/2] chore: exclude PERRY_PACKED_LOOP_TRACE from the build cache key packed_loop_reject prints and returns None either way, so emitted code is identical with the flag on and off. codegen_env_vars_are_build_cache_inputs requires every perry-codegen PERRY_* read to be registered; an exclusion is correct here because an input would cost every trace run a rebuild. --- changelog.d/9204-packed-loop-trace.md | 11 +++++++++++ crates/perry/src/commands/compile/build_cache.rs | 5 +++++ 2 files changed, 16 insertions(+) create mode 100644 changelog.d/9204-packed-loop-trace.md diff --git a/changelog.d/9204-packed-loop-trace.md b/changelog.d/9204-packed-loop-trace.md new file mode 100644 index 0000000000..27cd23673e --- /dev/null +++ b/changelog.d/9204-packed-loop-trace.md @@ -0,0 +1,11 @@ +`match_packed_f64_versioned_loop`'s admission decision is a chain of independent +conditions, and when a loop unexpectedly stayed on the generic path there was no +way to tell which one declined it without reading the code and guessing — three +separate attempts on #9151 each found a different gate that way. +`PERRY_PACKED_LOOP_TRACE=1` names the rejection reason instead. + +`packed_loop_reject` prints and returns `None` either way, so the emitted code +is identical with the flag on and off. It is registered in +`BUILD_CACHE_ENV_EXCLUSIONS` rather than `BUILD_CACHE_ENV_VARS` for that +reason: making it a cache input would cost every trace run a rebuild and buy +nothing. diff --git a/crates/perry/src/commands/compile/build_cache.rs b/crates/perry/src/commands/compile/build_cache.rs index f6f446b5a2..f2270fed8a 100644 --- a/crates/perry/src/commands/compile/build_cache.rs +++ b/crates/perry/src/commands/compile/build_cache.rs @@ -206,6 +206,11 @@ const BUILD_CACHE_ENV_EXCLUSIONS: &[&str] = &[ // Human-facing telemetry only; never changes IR or object bytes. "PERRY_CODEGEN_PROGRESS", "PERRY_CODEGEN_UNIT_TIMINGS", + // `packed_loop_reject` prints the admission chain's declining condition and + // returns `None` either way — the rejection is what the caller already got + // without the flag, so the emitted code is identical. An input, rather than + // an exclusion, would make every trace run miss the cache for nothing. + "PERRY_PACKED_LOOP_TRACE", // Entry outlining report output is observational only. "PERRY_OUTLINE_ENTRY_REPORT", // Only read on an already-fatal dialect-construction failure (a unit that