From 5f8c592314bd7bd99714ef2f2c728a8f63966d92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 28 Sep 2026 03:46:42 +0200 Subject: [PATCH 1/3] fix(gc-root-dominance): a phi edge that replaces the value is not in its window The 96 js_jsvalue_to_string_method_box violations on the dependency-scale corpus (#11604) are a checker false positive, not a codegen hazard. S2 (#11554) lowers `${v}` as br i1 %is_str, label %tmpl_coerce.merge, label %tmpl_coerce.slow tmpl_coerce.slow: %c = call double @js_template_string_coerce_box(double %v) tmpl_coerce.merge: %p = phi double [ %v, %entry ], [ %c, %tmpl_coerce.slow ] store %p -> slot; js_shadow_slot_bind On the only path through the collecting call the slot receives %c, and %v is dead after being passed to the helper. The bind-anchored window walk followed the phi back to %v and then counted collectors on every path, including the one that replaces %v. check_func now drops, per origin, the phi edges on the bound register's chain whose operand is not derived from the origin (the shadow-mode twin of #7664's --statepoints refinement). A collector on an edge that carries the value, or between the join and the bind, is still reported; --self-test asserts both directions plus the safe join. test_gap_gc_template_coerce_join puts the shape in the curated corpus (4 violations under the old checker, 0 now). Closes #11604 --- docs/src/internals/gc-rooting-invariant.md | 13 ++ scripts/gc_root_dominance_check.py | 195 ++++++++++++++++-- .../test_gap_gc_template_coerce_join.ts | 60 ++++++ test-parity/gc_repsel_corpus.txt | 6 + 4 files changed, 262 insertions(+), 12 deletions(-) create mode 100644 test-files/test_gap_gc_template_coerce_join.ts diff --git a/docs/src/internals/gc-rooting-invariant.md b/docs/src/internals/gc-rooting-invariant.md index 17d03d584a..529099b999 100644 --- a/docs/src/internals/gc-rooting-invariant.md +++ b/docs/src/internals/gc-rooting-invariant.md @@ -252,6 +252,19 @@ dominate the bind, so the register being rooted really is the one that instruction produced on every path. It is one-sided: an unrecognised call counts as collecting, so a gap in its model costs a false positive, never a missed bug. +**Phi edges that replace the value are not part of its window (#11604).** When +the bound register reaches the producing instruction through a `phi`, a path +that enters the join through an edge whose operand is something *else* (another +register or a constant) delivers that other value to the slot, not the one being +checked. So the window walk does not take such an edge: a collector reachable +from the producer only through it is not reported. This is the shadow-mode twin +of #7664's `--statepoints` refinement. The shape that needed it is S2's +template coercion (#11554): `phi [ %v, %entry ], [ %coerced, %tmpl_coerce.slow ]`, +where the only collecting call is on the arm that replaces `%v`. Before the +refinement that one join read as 96 violations. A collector on an edge that +*carries* the value, or between the join and the bind, is still reported, and +`--self-test` asserts both (`_SELFTEST_PHI_SAFE_EDGE` / `_SELFTEST_PHI_HAZARD`). + For a single file you are iterating on: ```bash diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 8852f5b6e5..fbdc3eef94 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -1785,14 +1785,20 @@ def dominates(idom, a, b): return False -def between_blocks(f, a_blk, b_blk): +def between_blocks(f, a_blk, b_blk, killed=frozenset()): """Blocks strictly between a_blk and b_blk on some path that does NOT re-enter a_blk (so a loop back-edge round trip is not counted -- that is a - different dynamic instance of the value).""" + different dynamic instance of the value). + + `killed` is a set of `(pred, succ)` CFG edges no path may take: the + phi edges along which the value being checked is REPLACED rather than + carried (#11604, see `phi_replacing_edges`). Empty by default, which is + every caller except the bind-anchored window.""" if a_blk == b_blk: return set() fwd = set() - q = deque(s for s in f.succs[a_blk] if s in f.insns and s != a_blk) + q = deque(s for s in f.succs[a_blk] + if s in f.insns and s != a_blk and (a_blk, s) not in killed) while q: x = q.popleft() if x in fwd: @@ -1801,10 +1807,11 @@ def between_blocks(f, a_blk, b_blk): if x == b_blk: continue # sink: do not expand past the bind for s in f.succs[x]: - if s in f.insns and s != a_blk: + if s in f.insns and s != a_blk and (x, s) not in killed: q.append(s) bwd = set() - q = deque(p for p in f.preds[b_blk] if p in f.insns and p != a_blk) + q = deque(p for p in f.preds[b_blk] + if p in f.insns and p != a_blk and (p, b_blk) not in killed) while q: x = q.popleft() if x in bwd: @@ -1813,11 +1820,62 @@ def between_blocks(f, a_blk, b_blk): if x == b_blk: continue for p in f.preds[x]: - if p in f.insns and p != a_blk: + if p in f.insns and p != a_blk and (p, x) not in killed: q.append(p) return (fwd & bwd) - {a_blk, b_blk} +def phi_replacing_edges(f, def_of, origin_reg, chain): + """CFG edges along which the value `origin_reg` produced is REPLACED on + its way to the bound register, rather than carried to it (#11604). + + `chain` is the bound register's backward transparent closure. A `phi` on + it merges several values; on an incoming edge whose operand is NOT + derived from `origin_reg` (another register, or a constant), the join + yields that other value, so a collection on a path that enters the join + only through such an edge happens while the slot is about to receive + something else -- not `origin_reg`'s pointer. Counting it anyway is the + loose-direction over-approximation #7664 already removed from the + `--statepoints` mode (`_cast_closure`'s `phi_all_edges` and + `_phi_edge_hazard`), and it is exactly the S2 template-coercion join + (#11554): `phi [ %v, %entry ], [ %coerced, %tmpl_coerce.slow ]`, where the + only collecting call is on the arm that replaces `%v`. + + An edge is killed only when NO chain phi in the join block takes an + origin-derived operand from it, so two phis that disagree about an edge + keep it (conservative). A collector on any edge that does carry the + value, or between the join and the bind, is still reported -- the + `_SELFTEST_PHI_*` fixtures assert both. + """ + tainted = {origin_reg} + changed = True + while changed: + changed = False + for r in chain: + if r in tainted: + continue + d = def_of.get(r) + if d is None or not is_transparent(d): + continue + if operand_regs(d.text) & tainted: + tainted.add(r) + changed = True + carry = {} # join block -> preds that carry an origin-derived operand + replace = {} # join block -> preds whose operand is something else + for r in tainted: + d = def_of.get(r) + if d is None or not _is_phi(d): + continue + for val, pred in phi_incoming(d): + val = val.strip() + if val.startswith("%") and val[1:] in tainted: + carry.setdefault(d.block, set()).add(pred) + else: + replace.setdefault(d.block, set()).add(pred) + return frozenset((pred, blk) for blk, preds in replace.items() + for pred in preds - carry.get(blk, set())) + + def loop_carried_blocks(f, a_blk, b_blk, barrier_blks=()): """Blocks on a cycle b_blk -> ... -> b_blk that enters neither a_blk nor any of `barrier_blks` (blocks that re-store the slot), or None when there @@ -2004,7 +2062,7 @@ def check_func(module, f, want_moving_only=False, poll_reaching=frozenset(), idom = dominators(f) violations = [] - def window_hits(A, B): + def window_hits(A, B, killed=frozenset()): """Collecting calls on some CFG path from just after A to B.""" hits = [] if A.block == B.block: @@ -2018,13 +2076,13 @@ def window_hits(A, B): for c in f.insns[B.block]: if is_collecting(c.callee) and c.idx < B.idx: hits.append(c) - for m_blk in between_blocks(f, A.block, B.block): + for m_blk in between_blocks(f, A.block, B.block, killed): for c in f.insns[m_blk]: if is_collecting(c.callee): hits.append(c) return hits - def protected(A, B, chain): + def protected(A, B, chain, killed=frozenset()): """Is the value rooted some other way inside the window? A temp-root push or a mutable-capture box store of any register in the value's provenance chain roots it (both are scanned AND rewritten).""" @@ -2039,7 +2097,7 @@ def scan(blk, lo, hi): return True if scan(B.block, 0, B.idx): return True - for m_blk in between_blocks(f, A.block, B.block): + for m_blk in between_blocks(f, A.block, B.block, killed): if scan(m_blk, 0, len(f.insns[m_blk])): return True return False @@ -2088,10 +2146,12 @@ def scan(blk, lo, hi): continue if origin.block == bind_ins.block and origin.idx >= bind_ins.idx: continue - hits = window_hits(origin, bind_ins) + killed = (phi_replacing_edges(f, def_of, origin.result, chain) + if origin.result else frozenset()) + hits = window_hits(origin, bind_ins, killed) if not hits: continue - if protected(origin, bind_ins, chain): + if protected(origin, bind_ins, chain, killed): continue v = Violation(module, f.name, origin, store_ins, bind_ins, hits, slot, poll_reaching) @@ -2494,6 +2554,86 @@ def seeded_violation_test(paths, moving_only, anchor, want_sites, verbose=False) """ +# #11604: the S2 template-coercion join (#11554). `%v` is a heap value; the +# only collecting call is on the arm that REPLACES it, so on every path that +# delivers `%v` to the store nothing collects. Before #11604 this read as a +# violation (96 of them on the corpus): the window walk followed the phi back +# to `%v` and then counted the slow arm's call, a path on which the slot +# receives `%c`, not `%v`. +_SELFTEST_PHI_SAFE_EDGE = """\ +define double @perry_fn_selftest__tmpl_join(double %a) { +entry.0: + %slot = alloca i64 + call void @js_shadow_frame_enter(i32 1) + %v = call double @js_jsvalue_to_string_method_box(double %a) + %bits = bitcast double %v to i64 + %top = lshr i64 %bits, 48 + %is_str = icmp eq i64 %top, 32767 + br i1 %is_str, label %tmpl_coerce.merge.2, label %tmpl_coerce.slow.1 + +tmpl_coerce.slow.1: + %c = call double @js_template_string_coerce_box(double %v) + br label %tmpl_coerce.merge.2 + +tmpl_coerce.merge.2: + %p = phi double [ %v, %entry.0 ], [ %c, %tmpl_coerce.slow.1 ] + %pb = bitcast double %p to i64 + store i64 %pb, ptr %slot + call void @js_shadow_slot_bind(i32 0, ptr %slot) + ret double %p +} +""" + +# The two ways that join CAN still be a hazard, which the edge refinement must +# keep reporting: a collector on an edge that CARRIES `%v` into the join, and a +# collector between the join and the bind (on every path, including the one +# carrying `%v`). +_SELFTEST_PHI_HAZARD = """\ +define double @perry_fn_selftest__carrying_edge(double %a, i1 %k) { +entry.0: + %slot = alloca i64 + call void @js_shadow_frame_enter(i32 1) + %v = call double @js_jsvalue_to_string_method_box(double %a) + br i1 %k, label %fast.1, label %slow.2 + +fast.1: + %poll = call double @js_gc_loop_safepoint(double %a) + br label %merge.3 + +slow.2: + %c = call double @js_template_string_coerce_box(double %v) + br label %merge.3 + +merge.3: + %p = phi double [ %v, %fast.1 ], [ %c, %slow.2 ] + %pb = bitcast double %p to i64 + store i64 %pb, ptr %slot + call void @js_shadow_slot_bind(i32 0, ptr %slot) + ret double %p +} + +define double @perry_fn_selftest__after_join(double %a, i1 %k) { +entry.0: + %slot = alloca i64 + call void @js_shadow_frame_enter(i32 1) + %v = call double @js_jsvalue_to_string_method_box(double %a) + br i1 %k, label %merge.2, label %slow.1 + +slow.1: + %c = call double @js_template_string_coerce_box(double %v) + br label %merge.2 + +merge.2: + %p = phi double [ %v, %entry.0 ], [ %c, %slow.1 ] + %ret = call double @js_call_function(double %a) + %pb = bitcast double %p to i64 + store i64 %pb, ptr %slot + call void @js_shadow_slot_bind(i32 0, ptr %slot) + ret double %ret +} +""" + + # ------------------------------------------------- stale-register invariant # # The check above anchors on a shadow-slot BIND, so it can only see values that @@ -5298,6 +5438,37 @@ def self_test(): file=sys.stderr) ok = False + # --- #11604: phi edges that REPLACE the value, both directions ------ + # + # The safe join must clear, and the refinement that clears it must + # not have blinded the check to a collector on an edge that carries + # the value, or to one between the join and the bind. + phi_safe = os.path.join(td, "phi_safe.ll") + phi_hazard = os.path.join(td, "phi_hazard.ll") + for p, text in ((phi_safe, _SELFTEST_PHI_SAFE_EDGE), + (phi_hazard, _SELFTEST_PHI_HAZARD)): + with open(p, "w") as fh: + fh.write(text) + found, binds = _scan([phi_safe], False, "alloc") + if found or binds != 1: + print(f"self-test FAIL: template-coercion join fixture -> " + f"{len(found)} violations over {binds} binds, expected 0 " + "over 1. The only collecting call is on the arm whose phi " + "edge REPLACES the value, so the slot never receives the " + "allocation across it (#11604).", file=sys.stderr) + ok = False + found, binds = _scan([phi_hazard], False, "alloc") + got = sorted(v.func for v in found) + want = ["perry_fn_selftest__after_join", + "perry_fn_selftest__carrying_edge"] + if got != want or binds != 2 or not all(v.moving for v in found): + print(f"self-test FAIL: phi hazard fixture -> {got} over {binds} " + f"binds, expected {want} over 2, both MOVING. The phi-edge " + "refinement must only drop edges that REPLACE the value; a " + "collector on a carrying edge, or after the join, is still " + "a late root store.", file=sys.stderr) + ok = False + # --stale-registers is a diagnostic, so its exit status is asserted # from both ends: the default must NOT go red on a corpus that has # hits, and --max-stale must actually be able to. A budget nobody has diff --git a/test-files/test_gap_gc_template_coerce_join.ts b/test-files/test_gap_gc_template_coerce_join.ts new file mode 100644 index 0000000000..2d2c38ce45 --- /dev/null +++ b/test-files/test_gap_gc_template_coerce_join.ts @@ -0,0 +1,60 @@ +// #11604: S2 (#11554) split `${v}` into an inline string/SSO tag test plus the +// collecting `js_template_string_coerce_box` on a cold arm, joined by +// +// %p = phi double [ %v, %entry ], [ %coerced, %tmpl_coerce.slow ] +// +// When `v` is itself a heap value the gate anchors on -- `${x.toString()}` +// lowers `v` to `js_jsvalue_to_string_method_box` -- the root-dominance check +// used to follow the phi back to `v` and count the slow arm's call as a +// collection between `v` and its root store: 96 violations on the +// dependency-scale corpus, none in the curated one. On the path through the +// slow arm the slot receives `%coerced`, not `v`, so that path was never a +// window for `v`. This file puts the shape in the curated corpus +// (`test_gap_gc_*` is one of gc_root_dominance_corpus.sh's patterns), so the +// checker's phi-edge refinement is exercised on real emitted IR and not only +// on its self-test fixtures. +// +// It also runs the shape under the moving collector. Every template below +// sits in a loop that allocates, so a back-edge poll has something to move: +// +// PERRY_GC_SCHEDULE_SEED=7 PERRY_GC_SCHEDULE_RATE=1 \ +// PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_DIAG=1 ./out +// +// must print the same lines as node, with `copying_minors` above zero. + +class Point { + x: number; + y: number; + constructor(x: number, y: number) { + this.x = x; + this.y = y; + } +} + +// Operands of every class: a `.toString()` result (heap value into the join's +// fast edge), a number (slow arm allocates its text), an object with a user +// `toString` (slow arm runs JS), and an already-string local. +function label(n: number, o: any, s: string): string { + const a = `${n.toString()}:${o.toString()}`; + const b = `${n}|${o}|${a}|${s}`; + return a + "/" + b; +} + +const kept: string[] = []; +let total = 0; +for (let i = 0; i < 6000; i++) { + const p = new Point(i, i * 2); + const o = { + k: i, + toString() { + return "o" + this.k; + }, + }; + const r = label(i + 0.5, o, "s" + i) + `${p.x.toString()}-${[i, i + 1].toString()}`; + total += r.length; + if (i % 1500 === 0) kept.push(r); +} + +// Values bound across the loop must still read correctly after every move. +for (const k of kept) console.log(k); +console.log("total", total); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index e1a2dc6536..61f133693d 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -878,3 +878,9 @@ test_gap_gc_store_ic_generic_receivers # clone polls and grows through a cached copy of the global, which must be a # GC root. SIGSEGV'd on every seed under the seeded schedule before the fix. test_gap_gc_11590_packed_loop_global_cache_rooting + +# --- #11604: template coercion's S2 phi join ---------------------------------- +# `${x.toString()}` feeds a heap value into `phi [ %v, %entry ], [ %coerced, +# %tmpl_coerce.slow ]`; puts the shape in the root-dominance curated corpus and +# runs it under every moving arm. +test_gap_gc_template_coerce_join From 7b9920bdb7a7e471c03d0e097ae3b7f1a863708f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 28 Sep 2026 03:47:16 +0200 Subject: [PATCH 2/3] changelog: #11606 gc-root-dominance phi-replacing edge --- changelog.d/11606-gc-root-dominance-phi-replacing-edge.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11606-gc-root-dominance-phi-replacing-edge.md diff --git a/changelog.d/11606-gc-root-dominance-phi-replacing-edge.md b/changelog.d/11606-gc-root-dominance-phi-replacing-edge.md new file mode 100644 index 0000000000..a48f18e613 --- /dev/null +++ b/changelog.d/11606-gc-root-dominance-phi-replacing-edge.md @@ -0,0 +1 @@ +- **gc-root-dominance: a phi edge that replaces the value is no longer counted in its window (#11604).** The 96 `js_jsvalue_to_string_method_box` violations on the dependency-scale corpus were a checker false positive. S2 (#11554) joins `${v}`'s inline string test and its collecting cold arm as `phi [ %v, %entry ], [ %coerced, %tmpl_coerce.slow ]`. On the only path through the collecting call, the slot receives `%coerced`, not `%v`. The bind-anchored check (`check_func`) followed the phi back to `%v` and then counted that path. It now drops, per origin, the phi edges whose operand is not derived from the origin (`phi_replacing_edges`), which is the shadow-mode twin of #7664's `--statepoints` refinement. A collector on an edge that carries the value, or between the join and the bind, is still reported, and `--self-test` asserts both plus the safe join. The new `test_gap_gc_template_coerce_join` puts the shape in the curated corpus: 4 violations under the old checker, 0 now. On the dependency corpus the unfiltered count goes from 146 to 50: exactly the 96 template-coercion fingerprints are removed, and no new ones appear. From b52b3478239bfcbbd22078424383b4792e0b56a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 28 Sep 2026 06:19:34 +0200 Subject: [PATCH 3/3] fix(gc-root-dominance): same phi-edge rule in --stale-registers; pin budgets 39->2, 118->10 The PR's labelled run went red on the curated stale budget (49 > 39). The first commit did not change that mode: its stale output is byte-identical with and without it. The count came from two things: * #11554's template join added 24 uses on main (8 at 62ab59233, 32 at 7401e6c0d), and the dependency corpus went 21 -> 457 against a budget of 118. That step never ran on main because the dominance step failed first on the same join. * the new test_gap_gc_template_coerce_join added 17 curated uses of the same shape (32 -> 49). All of them are uses reached only through a phi edge that REPLACES the source: S2's template join, and the `&&` / `??` short-circuit join (the dropped non-template ones, checked by hand in the IR). check_func_stale now drops those edges per use, and only when the use names no register derived from the source without a phi. The self-test asserts both directions. Measured: curated 49 -> 2, dependency 457 -> 10. Both are subsets of the old reports with no new ones. The budgets are pinned at those values. --- .github/workflows/gc-root-dominance.yml | 20 ++++- ...06-gc-root-dominance-phi-replacing-edge.md | 2 + docs/src/internals/gc-rooting-invariant.md | 4 + scripts/gc_root_dominance_check.py | 77 ++++++++++++++++++- .../gc_root_dominance_dep_native_corpus.sh | 2 +- 5 files changed, 100 insertions(+), 5 deletions(-) diff --git a/.github/workflows/gc-root-dominance.yml b/.github/workflows/gc-root-dominance.yml index af2897b991..074d40cc10 100644 --- a/.github/workflows/gc-root-dominance.yml +++ b/.github/workflows/gc-root-dominance.yml @@ -402,25 +402,41 @@ jobs: # This step is minutes, not seconds -- the scan is superlinear in # instruction count and the dependency corpus is 62 MB. That is the price # of checking the population that actually breaks. + # + # 39 -> 2 at #11604. The checker stopped counting a use reached only + # through a phi edge that REPLACES the source (the `&&`/`??` short-circuit + # join, and S2's template-coercion join from #11554). Those uses were + # never below the replacing arm's collector. Measured on the corpus + # before the change: 8 on 62ab59233, and 32 on 7401e6c0d, because #11554 + # added 24 template-join uses. After the change: 2, the two + # `InOrder.run` uses in test_gap_class_field_named_stack_index. Pinned + # at the measured value, because a ratchet is only a ratchet if it is + # pinned where it stands. - name: Stale-register budget (curated) run: | set -euo pipefail python3 scripts/gc_root_dominance_check.py ir-corpus \ --stale-registers --moving-only \ --min-files 90 --min-binds 1500 --min-funcs 1200 \ - --max-stale 39 + --max-stale 2 # 86 -> 104 at #7616: widening POLL_CAPABLE_RUNTIME by 77 symbols makes # windows MOVING that `--moving-only` previously dropped. Still inside # the pinned 118, so the budget is deliberately NOT raised — a ratchet # you loosen every time it gets closer is not a ratchet. + # + # 118 -> 10 at #11604, for the same phi-edge reason as the curated step. + # Main was at 457, over this budget, and nothing reported it: the + # dominance step above was red on the same #11554 join, so this step + # never ran. 436 of those uses were S2 template joins and 11 were + # `??`/`&&` joins. After the change: 10. - name: Stale-register budget (dependency-scale) run: | set -euo pipefail python3 scripts/gc_root_dominance_check.py ir-corpus-dep \ --stale-registers --moving-only \ --min-files 60 --min-binds 4000 --min-funcs 5000 \ - --max-stale 118 + --max-stale 10 - name: Upload the IR corpus on failure if: failure() diff --git a/changelog.d/11606-gc-root-dominance-phi-replacing-edge.md b/changelog.d/11606-gc-root-dominance-phi-replacing-edge.md index a48f18e613..03f244ee40 100644 --- a/changelog.d/11606-gc-root-dominance-phi-replacing-edge.md +++ b/changelog.d/11606-gc-root-dominance-phi-replacing-edge.md @@ -1 +1,3 @@ - **gc-root-dominance: a phi edge that replaces the value is no longer counted in its window (#11604).** The 96 `js_jsvalue_to_string_method_box` violations on the dependency-scale corpus were a checker false positive. S2 (#11554) joins `${v}`'s inline string test and its collecting cold arm as `phi [ %v, %entry ], [ %coerced, %tmpl_coerce.slow ]`. On the only path through the collecting call, the slot receives `%coerced`, not `%v`. The bind-anchored check (`check_func`) followed the phi back to `%v` and then counted that path. It now drops, per origin, the phi edges whose operand is not derived from the origin (`phi_replacing_edges`), which is the shadow-mode twin of #7664's `--statepoints` refinement. A collector on an edge that carries the value, or between the join and the bind, is still reported, and `--self-test` asserts both plus the safe join. The new `test_gap_gc_template_coerce_join` puts the shape in the curated corpus: 4 violations under the old checker, 0 now. On the dependency corpus the unfiltered count goes from 146 to 50: exactly the 96 template-coercion fingerprints are removed, and no new ones appear. + + `--stale-registers` had the same over-approximation, so it now applies the same phi-edge rule (`_stale_use_replacing_edges`), and the self-test asserts it in both directions. #11554's template join had pushed the stale counts past budget: curated 8 → 32, and dependency-scale 21 → 457 against a budget of 118. Nothing reported it, because the dominance step failed before the stale step could run. The new gap test added 17 more curated uses, which is what made this PR red. After the change the counts are 2 (curated) and 10 (dependency-scale), a subset of the old reports with no new ones, and both budgets are pinned there (39 → 2, 118 → 10). I checked the dropped non-template uses (`x && x.p`, `a ?? o.p`) by hand in the IR: the source reaches the join on a collector-free edge. diff --git a/docs/src/internals/gc-rooting-invariant.md b/docs/src/internals/gc-rooting-invariant.md index 529099b999..13d7983008 100644 --- a/docs/src/internals/gc-rooting-invariant.md +++ b/docs/src/internals/gc-rooting-invariant.md @@ -264,6 +264,10 @@ where the only collecting call is on the arm that replaces `%v`. Before the refinement that one join read as 96 violations. A collector on an edge that *carries* the value, or between the join and the bind, is still reported, and `--self-test` asserts both (`_SELFTEST_PHI_SAFE_EDGE` / `_SELFTEST_PHI_HAZARD`). +`--stale-registers` applies the same rule when a use reaches its source only +through such a phi (`_stale_use_replacing_edges`). That took the stale budgets +from 39 to 2 (curated) and from 118 to 10 (dependency-scale). The +dependency-scale corpus had been at 457 on `main`, and no step reported it. For a single file you are iterating on: diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index fbdc3eef94..066e3a58d9 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -2897,6 +2897,7 @@ def check_func_stale(module, f, poll_reaching=frozenset(), moving_only=False): and uses(ins.text, chain)): chain.add(ins.result) grew = True + direct = None # phi-free closure, built on first need (#11604) # First real (non-transparent) use of any register in the chain # that sits below a collection point. for bb in f.blocks: @@ -2915,6 +2916,18 @@ def check_func_stale(module, f, poll_reaching=frozenset(), moving_only=False): hits = window_hits_generic(f, src, use) if not hits: continue + # #11604: a use reached only through a phi edge that + # REPLACES the source is not below that edge's collector. + # Refining can only remove hits, so it runs only when + # there are some. + if direct is None: + direct = _phi_free_closure(def_of, src.result, chain) + killed = _stale_use_replacing_edges( + f, def_of, src.result, chain, direct, use) + if killed: + hits = window_hits_generic(f, src, use, killed=killed) + if not hits: + continue v = StaleUse(module, f.name, src, kind, use, hits, src.result, poll_reaching) if moving_only and not v.moving: @@ -2927,11 +2940,58 @@ def check_func_stale(module, f, poll_reaching=frozenset(), moving_only=False): return out +def _phi_free_closure(def_of, src_reg, chain): + """The registers of `chain` (the source's forward transparent closure) + that derive from `src_reg` WITHOUT passing through a phi: they carry the + source on every path.""" + direct = {src_reg} + grew = True + while grew: + grew = False + for r in chain: + if r in direct: + continue + d = def_of.get(r) + if (d is not None and not _is_phi(d) and is_transparent(d) + and operand_regs(d.text) & direct): + direct.add(r) + grew = True + return direct + + +def _stale_use_replacing_edges(f, def_of, src_reg, chain, direct, use): + """`phi_replacing_edges` for a stale-register use (#11604). + + The use reads the source only through a phi when none of the chain + registers it names is in `direct` (the phi-free closure). Then a path + that enters such a phi through an edge carrying something else delivers + that other value to the use, and a collector reachable only through that + edge is not in the source's window. S2's template join is the case: + `phi [ %v, %entry ], [ %coerced, %tmpl_coerce.slow ]` then a store of + the phi, where the only collector is the slow arm's call. If the use + names any directly-derived register, nothing is dropped. + """ + used = {r for r in operand_regs(use.text) if r in chain} + if not used or used & direct: + return frozenset() + back = set() + q = deque(used) + while q: + r = q.popleft() + if r in back: + continue + back.add(r) + d = def_of.get(r) + if d is not None and is_transparent(d): + q.extend(operand_regs(d.text)) + return phi_replacing_edges(f, def_of, src_reg, back) + + def _collecting_insn(ins): return is_collecting(ins.callee) -def window_hits_generic(f, A, B, pred=_collecting_insn): +def window_hits_generic(f, A, B, pred=_collecting_insn, killed=frozenset()): """Collection points on some CFG path from just after A to just before B. `pred` decides what a collection point IS. The shadow modes pass the @@ -2951,7 +3011,7 @@ def window_hits_generic(f, A, B, pred=_collecting_insn): for c in f.insns[B.block]: if pred(c) and c.idx < B.idx: hits.append(c) - for m_blk in between_blocks(f, A.block, B.block): + for m_blk in between_blocks(f, A.block, B.block, killed): for c in f.insns[m_blk]: if pred(c): hits.append(c) @@ -5468,6 +5528,19 @@ def self_test(): "collector on a carrying edge, or after the join, is still " "a late root store.", file=sys.stderr) ok = False + # Same refinement, same two directions, in --stale-registers: the + # join's store must not read as a stale use of `%v`, and both hazard + # shapes must still do so. + n_safe, _rc = _stale_probe(phi_safe, None) + n_hazard, _rc = _stale_probe(phi_hazard, None) + if n_safe != 0 or n_hazard != 2: + print(f"self-test FAIL: --stale-registers over the phi fixtures -> " + f"{n_safe} (safe join) / {n_hazard} (hazards), expected 0 / " + "2. A use reached only through a phi edge that REPLACES the " + "source is not below the replacing arm's collector (#11604); " + "a carrying edge's collector, or one after the join, is.", + file=sys.stderr) + ok = False # --stale-registers is a diagnostic, so its exit status is asserted # from both ends: the default must NOT go red on a corpus that has diff --git a/scripts/gc_root_dominance_dep_native_corpus.sh b/scripts/gc_root_dominance_dep_native_corpus.sh index 60692c47f9..97fa9e7241 100755 --- a/scripts/gc_root_dominance_dep_native_corpus.sh +++ b/scripts/gc_root_dominance_dep_native_corpus.sh @@ -8,7 +8,7 @@ # # shadow (PERRY_RS4GC=0) native (statepoints, SHIPS) # curated ~124 files gated gated, --max-unrooted 0 -# dependency (zod) gated, --max-stale 118 THIS SCRIPT (was missing) +# dependency (zod) gated, --max-stale 10 THIS SCRIPT (was missing) # # #7280's own finding was that the curated corpus is the wrong POPULATION -- # 25 curated files pass while 20 lines of stock zod fault. #7452's finding was