From bbfc3c1d94adc9fac20ce513857de88fd16bc7a3 Mon Sep 17 00:00:00 2001 From: tomoaki Date: Sat, 27 Dec 2025 08:32:14 +0900 Subject: [PATCH] refactor(joinir): Phase 287 P2 - Strengthen BackEdge/latch conditions (WIP) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Problem**: Phase 188.3 Pattern6 (nested loop) encounters infinite loop - inner_step → inner_step (self-recursion) incorrectly classified as BackEdge - → redirects to outer header (loop_step) instead of inner_step entry - is_recursive_call causes inner recursion to overwrite outer latch values - → PHI receives wrong values → i doesn't increment → infinite loop **Fix 1: BackEdge classification strictness** (tail_call_classifier.rs) - Add `is_target_loop_entry` parameter to classify_tail_call() - BackEdge ONLY when target==loop_step (entry_func), not inner_step - Prevents inner_step → inner_step from redirecting to outer header **Fix 2: latch_incoming guard** (instruction_rewriter.rs) - Change condition from `is_recursive_call || is_target_loop_entry` to `is_target_loop_entry` only - Prevents inner_step self-recursion from overwriting outer loop's latch - set_latch_incoming() now called with correct values (verified by debug) **Status**: 🚧 WIP - Infinite loop still occurs - set_latch_incoming('i', BasicBlockId(8), ValueId(21)) ✅ Called correctly - But final PHI: `phi [%4, bb8]` instead of `phi [%21, bb8]` ❌ - Root cause likely in PHI generation (merge/mod.rs), not latch_incoming - Next: Investigate why latch_incoming values aren't used in PHI **Files**: - src/mir/builder/control_flow/joinir/merge/tail_call_classifier.rs - src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- docs/development/current/main/10-Now.md | 3 +- .../phases/phase-188.3/P1-INSTRUCTIONS.md | 27 ++++++++++++++- .../current/main/phases/phase-188.3/README.md | 28 +++++++++++++++ .../joinir/merge/instruction_rewriter.rs | 34 +++++++++++++------ .../joinir/merge/tail_call_classifier.rs | 29 ++++++++++++++-- 5 files changed, 105 insertions(+), 16 deletions(-) diff --git a/docs/development/current/main/10-Now.md b/docs/development/current/main/10-Now.md index 99c738c1..cc777350 100644 --- a/docs/development/current/main/10-Now.md +++ b/docs/development/current/main/10-Now.md @@ -6,7 +6,8 @@ - StepTreeの `max_loop_depth` を SSOT に採用（Option A） - strict mode で depth > 2 を明示エラー化（Fail-Fast） - quick 154/154 PASS、integration selfhost FAIL=0 維持 -- 次: `docs/development/current/main/phases/phase-188.3/README.md`（depth=2 を JoinIR lowering で通す / Phase 188.3は“最小のwrite-back”をcarrierとして明示、一般化は次フェーズ） +- 次: `docs/development/current/main/phases/phase-188.3/README.md`（depth=2 を JoinIR lowering で通す / “最小write-back”は carrier として明示） +- 実装導線（手順書）: `docs/development/current/main/phases/phase-188.3/P1-INSTRUCTIONS.md`（merge/rewriter の “undef ValueId” 典型罠もここに固定） **2025-12-27: Phase S0.1 完了** ✅ - integration selfhost を「落ちない状態」に収束（FAIL=0） diff --git a/docs/development/current/main/phases/phase-188.3/P1-INSTRUCTIONS.md b/docs/development/current/main/phases/phase-188.3/P1-INSTRUCTIONS.md index 899d9082..4dbdb968 100644 --- a/docs/development/current/main/phases/phase-188.3/P1-INSTRUCTIONS.md +++ b/docs/development/current/main/phases/phase-188.3/P1-INSTRUCTIONS.md @@ -129,9 +129,34 @@ nested loop では `inner_step` が混ざるので、**`inner_step` / `k_inner_e --- +## Troubleshooting: `use of undefined value ValueId(...)`（Pattern6） + +典型ログ： + +- `[cf_loop/joinir] Function 'inner_step' params: [ValueId(104), ...]` +- `use of undefined value ValueId(104)` + +意味： +- JoinIR の “param ValueId” は SSA 命令で定義されないため、`Copy(dst=param, src=arg)` が入らないと undefined になる。 + +優先して疑う場所（責務の順）： + +1. `src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs` + - `plan_rewrites()` の “tail-call param binding” の **skip 条件** + - SSOT: **skip は “target が loop header” のときだけ**（header PHI dst を上書きしないため） + - `loop_step（entry func）→ inner_step` まで “entry func だから” という理由で skip すると、inner_step param が未定義になる +2. `JoinInlineBoundary.continuation_func_ids` + - `inner_step` / `k_inner_exit` が continuation 扱いになっていないと、merge 側の entry func 選定がズレて skip 判定も破綻しやすい + +注意（避けたい対処）： +- `merge/mod.rs` で “param ValueId を index で PHI dst にリマップする” は危険 + - Pattern6 は `inner_step(j, i, sum)` のように先頭に loop-local があり、carrier index と一致しない + - index remap は `j` を `i` の PHI dst に誤接続しやすい + +--- + ## 追加の注意（Fail-Fast） - Pattern6 が選ばれたあとに `Ok(None)` で他パターンに流すのは禁止（silent fallback） - “選ぶ前に落とす” が最も安全: - `is_pattern6_lowerable()` を「lowering が確実に通る形だけ true」に強化する - diff --git a/docs/development/current/main/phases/phase-188.3/README.md b/docs/development/current/main/phases/phase-188.3/README.md index 92e07f11..1be2007d 100644 --- a/docs/development/current/main/phases/phase-188.3/README.md +++ b/docs/development/current/main/phases/phase-188.3/README.md @@ -126,6 +126,26 @@ Phase 188.3 の最小形は「outer の loop_step の中で inner の loop_step --- +## Merge/Rewrite contract (SSOT) — “undef ValueId” を防ぐ + +JoinIR merge は「JoinIR の param ValueId は SSA 命令で定義されない」前提なので、**適切な Copy（param binding）** が入らないと即 `use of undefined value ValueId(...)` になる。 + +特に Pattern6 では `loop_step`（outer）から `inner_step` へ tail-call するため、以下を SSOT として固定する： + +- **Skip するのは “target が loop header” のときだけ** + - header では PHI dst が carrier の SSOT なので、param Copy を入れて上書きしてはいけない + - 一方で `loop_step → inner_step` は “target が header ではない” ため、**inner_step params を定義する Copy が必要** +- **param の index ベース remap は危険** + - `inner_step(j, i, sum)` のように、先頭に loop-local（`j`）が混ざると carrier の index と一致しない + - index remap は `j` を `i` の PHI dst に誤接続しやすい + +この契約に反する場合の典型症状： +- `Function 'inner_step' params: [ValueId(104), ...]` の直後に `use of undefined value ValueId(104)` + +修正の第一候補は `merge/instruction_rewriter.rs` 側（tail-call param binding の skip 条件）であり、merge/mod.rs の “パラメータ再マップを拡張する” で逃げない（責務を混ぜない）。 + +--- + ## Deliverables 1. **Fixture + integration smoke（exit code SSOT）** @@ -161,6 +181,14 @@ Phase 188.3 の最小形は「outer の loop_step の中で inner の loop_step - **Phase 188.3**: depth=2 の最小形を “確実に通す” + PoC fixture を smoke 固定 - **Phase 188.4+**: write-back（outer carrier reconnection）と “再帰 lowering の一般化（depthを増やしても壊れない）” を docs-first で設計してから実装 +### Planned cleanup (after Phase 188.3) + +Pattern6 を通す過程で露出しやすい “暗黙ルール” を SSOT 化して、今後の nested/generalization を楽にする： + +- `JoinInlineBoundary` に **loop header func name を明示する SSOT**（merge の “entry func 推定” を段階的に減らす） +- `ParamBinding` の規則を “source-based ではなく target-based” に統一（header だけ特別扱い） +- （将来）param role（carrier vs local）を明文化し、index remap の誘惑を消す + --- ## Out of Scope diff --git a/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs b/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs index 5ef2f0dc..dba372f3 100644 --- a/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs +++ b/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs @@ -517,7 +517,9 @@ fn plan_rewrites( // 1. Loop entry point (header PHIs define carriers) // 2. Recursive/entry call to loop header with PHIs (latch edge) // 3. Continuation call (handled separately below) - if is_loop_entry_point { + // Phase 287 P1: Skip ONLY when target is loop header + // (not when source is entry func but target is non-entry like inner_step) + if is_loop_entry_point && is_target_loop_entry { log!( verbose, "[plan_rewrites] Skip param bindings in header block (PHIs define carriers)" @@ -584,9 +586,10 @@ fn plan_rewrites( } } - // Record latch incoming for loop header PHI (recursive calls + loop entry calls) - // Phase 188.3: Extended to support continuation→header calls (Pattern6) - if is_recursive_call || is_target_loop_entry { + // Record latch incoming for loop header PHI (only for calls to loop entry func) + // Phase 287 P2: Restrict to is_target_loop_entry only (not is_recursive_call) + // This prevents inner_step recursion from overwriting outer loop's latch values + if is_target_loop_entry { if let Some(b) = boundary { if let Some(loop_var_name) = &b.loop_var_name { if !args.is_empty() { @@ -812,25 +815,34 @@ fn plan_rewrites( } else if let Some((target_block, args)) = tail_call_target { // Tail call: Set Jump terminator // Classify tail call and determine actual target - let is_target_continuation = { - let mut target_func_name: Option = None; + let target_func_name = { + let mut name: Option = None; for (fname, &entry_block) in &ctx.function_entry_map { if entry_block == target_block { - target_func_name = Some(fname.clone()); + name = Some(fname.clone()); break; } } - target_func_name - .as_ref() - .map(|name| continuation_candidates.contains(name)) - .unwrap_or(false) + name }; + let is_target_continuation = target_func_name + .as_ref() + .map(|name| continuation_candidates.contains(name)) + .unwrap_or(false); + + // Phase 287 P2: Compute is_target_loop_entry for classify_tail_call + let is_target_loop_entry = target_func_name + .as_ref() + .map(|name| entry_func_name == Some(name.as_str())) + .unwrap_or(false); + let tail_call_kind = classify_tail_call( is_loop_entry_point, !loop_header_phi_info.carrier_phis.is_empty(), boundary.is_some(), is_target_continuation, + is_target_loop_entry, ); let actual_target = match tail_call_kind { diff --git a/src/mir/builder/control_flow/joinir/merge/tail_call_classifier.rs b/src/mir/builder/control_flow/joinir/merge/tail_call_classifier.rs index b35213a8..9358898a 100644 --- a/src/mir/builder/control_flow/joinir/merge/tail_call_classifier.rs +++ b/src/mir/builder/control_flow/joinir/merge/tail_call_classifier.rs @@ -38,6 +38,7 @@ pub enum TailCallKind { /// * `has_loop_header_phis` - True if loop header PHI nodes exist /// * `has_boundary` - True if JoinInlineBoundary exists (indicates loop context) /// * `is_target_continuation` - True if the tail call target is a continuation function (k_exit) +/// * `is_target_loop_entry` - True if the tail call target is the loop entry function (loop_step) /// /// # Returns /// The classification of this tail call @@ -46,6 +47,7 @@ pub fn classify_tail_call( has_loop_header_phis: bool, has_boundary: bool, is_target_continuation: bool, + is_target_loop_entry: bool, ) -> TailCallKind { // Phase 256 P1.10: Continuation calls (k_exit) are always ExitJump // They should NOT be redirected to the header block. @@ -60,9 +62,11 @@ pub fn classify_tail_call( return TailCallKind::LoopEntry; } - // If we have boundary and header PHIs, this is a back edge - // Must redirect to header for PHI merging - if has_boundary && has_loop_header_phis { + // Phase 287 P2: BackEdge is ONLY for target==loop_step (entry_func) + // This prevents inner_step→inner_step from being classified as BackEdge, + // which would incorrectly redirect it to the outer header (loop_step). + // inner_step→inner_step should jump to inner_step's entry block, not outer header. + if is_target_loop_entry && has_boundary && has_loop_header_phis && !is_entry_func_entry_block { return TailCallKind::BackEdge; } @@ -81,6 +85,7 @@ mod tests { true, // has_loop_header_phis true, // has_boundary false, // is_target_continuation + true, // is_target_loop_entry ); assert_eq!(result, TailCallKind::LoopEntry); } @@ -92,6 +97,7 @@ mod tests { true, // has_loop_header_phis true, // has_boundary false, // is_target_continuation + true, // is_target_loop_entry ← target is loop entry func ); assert_eq!(result, TailCallKind::BackEdge); } @@ -103,6 +109,7 @@ mod tests { false, // has_loop_header_phis (no header PHIs) true, // has_boundary false, // is_target_continuation + false, // is_target_loop_entry ); assert_eq!(result, TailCallKind::ExitJump); } @@ -114,6 +121,7 @@ mod tests { true, // has_loop_header_phis false, // has_boundary (no boundary → exit) false, // is_target_continuation + false, // is_target_loop_entry ); assert_eq!(result, TailCallKind::ExitJump); } @@ -127,6 +135,21 @@ mod tests { true, // has_loop_header_phis true, // has_boundary true, // is_target_continuation ← this makes it ExitJump + true, // is_target_loop_entry + ); + assert_eq!(result, TailCallKind::ExitJump); + } + + #[test] + fn test_classify_inner_step_recursion() { + // Phase 287 P2: inner_step→inner_step should NOT be BackEdge + // even with boundary and header PHIs, because target is not loop entry func + let result = classify_tail_call( + false, // is_entry_func_entry_block (inner_step body block) + true, // has_loop_header_phis + true, // has_boundary + false, // is_target_continuation + false, // is_target_loop_entry ← target is NOT loop entry (inner_step, not loop_step) ); assert_eq!(result, TailCallKind::ExitJump); }