From 2c4268b69106a6207721fc2e63a2a7cea2f3f4a0 Mon Sep 17 00:00:00 2001 From: tomoaki Date: Sat, 20 Dec 2025 10:25:13 +0900 Subject: [PATCH] wip(joinir): Phase 256 P1.10 - classify_tail_call continuation support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 修正内容 - classify_tail_call に is_target_continuation パラメータ追加 - k_exit 呼び出しを ExitJump として分類（BackEdge に誤分類されるのを防止） - entry_func_name の決定ロジックを修正（loop_step を正しく識別） - Continuation param bindings の Copy 生成ロジック追加 ## 残存問題 bb8/bb9 の instructions が 0 になる問題 - add_block 時は 4 instructions ある - printer 時は instructions=0, spans=4 - 原因調査中 🔍 WIP - ChatGPT レビュー用 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- .../joinir/merge/instruction_rewriter.rs | 199 +++++++++++++++--- .../builder/control_flow/joinir/merge/mod.rs | 52 ++++- .../joinir/merge/tail_call_classifier.rs | 32 ++- .../joinir/merge/value_collector.rs | 24 ++- 4 files changed, 258 insertions(+), 49 deletions(-) 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 8029502e..f9f63b2a 100644 --- a/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs +++ b/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs @@ -210,6 +210,10 @@ pub(super) fn merge_and_rewrite( function_entry_map.insert(func_name.clone(), entry_block_new); } + // Phase 256 P1.10: Pre-pass removed - param mappings are set up during block processing + // The issue was that pre-pass used stale remapper values before Phase 33-21 updates. + // Instead, we now generate Copies for non-carrier params in the main block processing loop. + // DETERMINISM FIX: Sort functions by name to ensure consistent iteration order if debug { log!( @@ -222,7 +226,22 @@ pub(super) fn merge_and_rewrite( let mut functions_merge: Vec<_> = mir_module.functions.iter().collect(); functions_merge.sort_by_key(|(name, _)| name.as_str()); - let entry_func_name = functions_merge.first().map(|(name, _)| name.as_str()); + // Phase 256 P1.10: Determine actual entry function (loop header) + // + // The entry function is the one that: + // - Is NOT a continuation function (k_exit, etc.) + // - Is NOT "main" (the trampoline that calls the loop entry) + // + // This is the function that receives boundary inputs and is the loop header. + let entry_func_name = functions_merge + .iter() + .find(|(name, _)| { + let name_str = name.as_str(); + let is_continuation = continuation_candidates.contains(*name); + let is_main = name_str == crate::mir::join_ir::lowering::canonical_names::MAIN; + !is_continuation && !is_main + }) + .map(|(name, _)| name.as_str()); for (func_name, func) in functions_merge { // Phase 33-15: Identify continuation functions (k_exit, etc.) @@ -301,6 +320,23 @@ pub(super) fn merge_and_rewrite( entry_func_name == Some(func_name.as_str()) && *old_block_id == func.entry_block; // DEBUG: Print block being processed + // Phase 256 P1.10: Always log block mapping for debugging + log!( + true, + "[cf_loop/joinir] Phase 256 P1.10: Block mapping: func='{}' old={:?} → new={:?} (inst_count={})", + func_name, old_block_id, new_block_id, old_block.instructions.len() + ); + + // Phase 256 P1.10 DEBUG: Log first instruction for k_exit to trace content placement + if func_name == "k_exit" { + if let Some(first_inst) = old_block.instructions.first() { + log!( + true, + "[cf_loop/joinir] Phase 256 P1.10 DEBUG: k_exit first instruction: {:?}", + first_inst + ); + } + } if debug { log!( true, @@ -552,8 +588,40 @@ pub(super) fn merge_and_rewrite( } } - if let Some(target_func_name) = target_func_name { - if let Some(target_params) = function_params.get(&target_func_name) { + // Phase 256 P1.10: Define is_target_continuation for classify_tail_call + // This must be defined before the inner if lets so it's in scope for classify_tail_call + let is_target_continuation = target_func_name + .as_ref() + .map(|name| continuation_candidates.contains(name)) + .unwrap_or(false); + + if let Some(ref target_func_name) = target_func_name { + if let Some(target_params) = function_params.get(target_func_name) { + // Phase 256 P1.10: Detect call type for param binding strategy + // - Recursive call (loop_step → loop_step): Skip all param bindings (PHI handles via latch edges) + // - Exit call (loop_step → k_exit): Skip all param bindings (exit PHI handles via exit edges) + // - Header entry (main → loop_step at header): Skip all (PHI handles via entry edges) + let is_recursive_call = target_func_name == func_name; + + // Phase 256 P1.10: Detect if target is the loop entry function + // When calling the loop entry function, header PHIs will define the carriers. + // We should skip param bindings in this case. + let is_target_loop_entry = entry_func_name == Some(target_func_name.as_str()); + + // Phase 256 P1.10: Detect if target is a continuation (k_exit) + // k_exit params are exit binding values, which are remapped to exit PHI dsts. + // Generating Copies for k_exit would create multiple definitions (Copy + PHI). + // Use continuation_candidates (not skippable_continuation_func_names) because + // k_exit may have instructions (non-skippable) but still needs param skip. + let is_target_continuation = continuation_candidates.contains(target_func_name); + + // Phase 256 P1.10 DEBUG: Always log call type detection + log!( + true, + "[cf_loop/joinir] Phase 256 P1.10: Tail call from '{}' to '{}' (is_recursive={}, is_loop_entry={}, is_target_loop_entry={}, is_target_continuation={})", + func_name, target_func_name, is_recursive_call, is_loop_entry_point, is_target_loop_entry, is_target_continuation + ); + // Phase 33-21: Skip parameter binding in header block // // The header block (loop entry point) has PHIs that define carriers. @@ -572,6 +640,58 @@ pub(super) fn merge_and_rewrite( "[cf_loop/joinir] Phase 33-21: Skip param bindings in header block (PHIs define carriers)" ); } + } else if is_recursive_call || is_target_loop_entry { + // Phase 256 P1.10: Skip param bindings for: + // - Recursive call (loop_step → loop_step): latch edge + // - Entry call (main → loop_step): entry edge + // + // Header PHIs receive values from these edges via separate mechanism. + // Generating Copies here would cause multiple definitions. + // + // Update remapper mappings for any further instructions. + for (i, arg_val_remapped) in args.iter().enumerate() { + if i < target_params.len() { + let param_val_original = target_params[i]; + remapper.set_value(param_val_original, *arg_val_remapped); + } + } + log!( + true, + "[cf_loop/joinir] Phase 256 P1.10: Skip Copy bindings for {} call to '{}' (remapper updated)", + if is_recursive_call { "recursive" } else { "entry" }, + target_func_name + ); + } else if is_target_continuation { + // Phase 256 P1.10: Continuation call (loop_step → k_exit) + // k_exit body uses its params to compute return values. + // We must Copy call args to ORIGINAL params (not remapped). + // Params are excluded from remapping (Phase 256 P1.10 exit_binding skip), + // so using original params avoids conflict with exit PHI. + log!( + true, + "[cf_loop/joinir] Phase 256 P1.10 DEBUG: Before continuation Copies, new_block {:?} has {} instructions", + new_block_id, new_block.instructions.len() + ); + for (i, arg_val_remapped) in args.iter().enumerate() { + if i < target_params.len() { + let param_val_original = target_params[i]; + // Use original param as dst - it won't be remapped + new_block.instructions.push(MirInstruction::Copy { + dst: param_val_original, + src: *arg_val_remapped, + }); + log!( + true, + "[cf_loop/joinir] Phase 256 P1.10: Continuation param binding: {:?} = copy {:?}", + param_val_original, arg_val_remapped + ); + } + } + log!( + true, + "[cf_loop/joinir] Phase 256 P1.10 DEBUG: After continuation Copies, new_block {:?} has {} instructions", + new_block_id, new_block.instructions.len() + ); } else { // Insert Copy instructions for parameter binding // Phase 131-6 FIX: Skip Copy when dst is a header PHI destination @@ -579,40 +699,49 @@ pub(super) fn merge_and_rewrite( for (i, arg_val_remapped) in args.iter().enumerate() { if i < target_params.len() { let param_val_original = target_params[i]; - if let Some(param_val_remapped) = - remapper.get_value(param_val_original) - { - // Phase 131-6: Check if this would overwrite a header PHI dst - let is_header_phi_dst = loop_header_phi_info - .carrier_phis - .values() - .any(|entry| entry.phi_dst == param_val_remapped); + // Phase 256 P1.10: Use original param if not remapped + // Params are excluded from used_values to prevent undefined remaps, + // so we use the original param ValueId as the Copy dst. + let param_remap_result = remapper.get_value(param_val_original); + let param_val_dst = param_remap_result + .unwrap_or(param_val_original); - if is_header_phi_dst { - if debug { - log!( - true, - "[cf_loop/joinir] Phase 131-6: Skip param binding to PHI dst {:?} (PHI receives value via incoming edge)", - param_val_remapped - ); - } - // Skip - PHI will receive this value as incoming edge - continue; - } + // Phase 256 P1.10 DEBUG: Log remap decision + log!( + true, + "[cf_loop/joinir] Phase 256 P1.10 DEBUG: Param[{}] original={:?} remap_result={:?} final_dst={:?}", + i, param_val_original, param_remap_result, param_val_dst + ); - new_block.instructions.push(MirInstruction::Copy { - dst: param_val_remapped, - src: *arg_val_remapped, - }); + // Phase 131-6: Check if this would overwrite a header PHI dst + let is_header_phi_dst = loop_header_phi_info + .carrier_phis + .values() + .any(|entry| entry.phi_dst == param_val_dst); + if is_header_phi_dst { if debug { log!( true, - "[cf_loop/joinir] Param binding: arg {:?} → param {:?}", - arg_val_remapped, param_val_remapped + "[cf_loop/joinir] Phase 131-6: Skip param binding to PHI dst {:?} (PHI receives value via incoming edge)", + param_val_dst ); } + // Skip - PHI will receive this value as incoming edge + continue; } + + // Phase 256 P1.10 DEBUG: Always log Copy generation + log!( + true, + "[cf_loop/joinir] Phase 256 P1.10 DEBUG: Generating Copy: {:?} = copy {:?} (func='{}', target='{}')", + param_val_dst, arg_val_remapped, func_name, target_func_name + ); + + new_block.instructions.push(MirInstruction::Copy { + dst: param_val_dst, + src: *arg_val_remapped, + }); } } } @@ -726,10 +855,13 @@ pub(super) fn merge_and_rewrite( // - LoopEntry: First entry (entry function's entry block) → no redirect // - BackEdge: Loop continuation (other blocks) → redirect to header PHI // - ExitJump: Exit to continuation → handled by Return conversion + // Phase 256 P1.10: Add is_target_continuation to prevent k_exit calls + // from being redirected to header block. 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, ); let actual_target = match tail_call_kind { @@ -1003,6 +1135,17 @@ pub(super) fn merge_and_rewrite( // Add block to current function if let Some(ref mut current_func) = builder.scope_ctx.current_function { + // Phase 256 P1.10 DEBUG: Log block content before adding (for blocks with multiple instructions) + if new_block.instructions.len() >= 4 { + log!( + true, + "[cf_loop/joinir] Phase 256 P1.10 DEBUG: Adding block {:?} with {} instructions to function", + new_block_id, new_block.instructions.len() + ); + for (idx, inst) in new_block.instructions.iter().enumerate() { + log!(true, "[cf_loop/joinir] Phase 256 P1.10 DEBUG: [{}] {:?}", idx, inst); + } + } current_func.add_block(new_block); } } diff --git a/src/mir/builder/control_flow/joinir/merge/mod.rs b/src/mir/builder/control_flow/joinir/merge/mod.rs index 8c34256e..fb238fc5 100644 --- a/src/mir/builder/control_flow/joinir/merge/mod.rs +++ b/src/mir/builder/control_flow/joinir/merge/mod.rs @@ -230,16 +230,33 @@ pub(in crate::mir::builder) fn merge_joinir_mir_blocks( used_values.insert(binding.join_value); } - // Phase 172-3: Add exit_bindings' join_exit_values to used_values for remapping + // Phase 172-3 + Phase 256 P1.10: Add exit_bindings' join_exit_values to used_values + // UNLESS they are function params. Params should NOT be remapped (they're defined + // by call site Copies and used directly in k_exit body). + let all_params: std::collections::HashSet = function_params + .values() + .flat_map(|params| params.iter().copied()) + .collect(); + for binding in &boundary.exit_bindings { - trace.stderr_if( - &format!( - "[cf_loop/joinir] Phase 172-3: Adding exit binding '{}' JoinIR {:?} to used_values", - binding.carrier_name, binding.join_exit_value - ), - debug, - ); - used_values.insert(binding.join_exit_value); + if all_params.contains(&binding.join_exit_value) { + trace.stderr_if( + &format!( + "[cf_loop/joinir] Phase 256 P1.10: Skipping exit binding '{}' (JoinIR {:?} is a param)", + binding.carrier_name, binding.join_exit_value + ), + debug, + ); + } else { + trace.stderr_if( + &format!( + "[cf_loop/joinir] Phase 172-3: Adding exit binding '{}' JoinIR {:?} to used_values", + binding.carrier_name, binding.join_exit_value + ), + debug, + ); + used_values.insert(binding.join_exit_value); + } } } @@ -253,10 +270,17 @@ pub(in crate::mir::builder) fn merge_joinir_mir_blocks( let mut loop_header_phi_info = if let Some(boundary) = boundary { if let Some(loop_var_name) = &boundary.loop_var_name { // Get entry function and block for building PHI info + // Phase 256 P1.10: Find the actual entry function (loop header) + // The entry function is NOT a continuation (k_exit) and NOT "main". let (entry_func_name, entry_func) = mir_module .functions .iter() - .next() + .find(|(name, _)| { + let is_continuation = boundary.continuation_func_ids.contains(*name); + let is_main = *name == crate::mir::join_ir::lowering::canonical_names::MAIN; + !is_continuation && !is_main + }) + .or_else(|| mir_module.functions.iter().next()) .ok_or("JoinIR module has no functions (Phase 201-A)")?; let entry_block_remapped = remapper .get_block(entry_func_name, entry_func.entry_block) @@ -310,6 +334,14 @@ pub(in crate::mir::builder) fn merge_joinir_mir_blocks( .collect() }; + // Phase 256 P1.10: Always log entry function determination + trace.stderr_if( + &format!( + "[cf_loop/joinir] Phase 256 P1.10: Entry function='{}', entry_block_remapped={:?}", + entry_func_name, entry_block_remapped + ), + true, // Always log for debugging + ); trace.stderr_if( &format!( "[cf_loop/joinir] Phase 201-A: Pre-building header PHIs for loop_var='{}' at {:?}", 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 1a382ae9..b35213a8 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 @@ -37,6 +37,7 @@ pub enum TailCallKind { /// * `is_entry_func_entry_block` - True if this is the first function's first block (loop entry point) /// * `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) /// /// # Returns /// The classification of this tail call @@ -44,7 +45,15 @@ pub fn classify_tail_call( is_entry_func_entry_block: bool, has_loop_header_phis: bool, has_boundary: bool, + is_target_continuation: bool, ) -> TailCallKind { + // Phase 256 P1.10: Continuation calls (k_exit) are always ExitJump + // They should NOT be redirected to the header block. + // k_exit body needs to execute before exiting. + if is_target_continuation { + return TailCallKind::ExitJump; + } + // Entry function's entry block is the loop entry point // It already IS at the header, so no redirection needed if is_entry_func_entry_block { @@ -68,9 +77,10 @@ mod tests { #[test] fn test_classify_loop_entry() { let result = classify_tail_call( - true, // is_entry_func_entry_block - true, // has_loop_header_phis - true, // has_boundary + true, // is_entry_func_entry_block + true, // has_loop_header_phis + true, // has_boundary + false, // is_target_continuation ); assert_eq!(result, TailCallKind::LoopEntry); } @@ -81,6 +91,7 @@ mod tests { false, // is_entry_func_entry_block (not entry block) true, // has_loop_header_phis true, // has_boundary + false, // is_target_continuation ); assert_eq!(result, TailCallKind::BackEdge); } @@ -91,6 +102,7 @@ mod tests { false, // is_entry_func_entry_block false, // has_loop_header_phis (no header PHIs) true, // has_boundary + false, // is_target_continuation ); assert_eq!(result, TailCallKind::ExitJump); } @@ -101,6 +113,20 @@ mod tests { false, // is_entry_func_entry_block true, // has_loop_header_phis false, // has_boundary (no boundary → exit) + false, // is_target_continuation + ); + assert_eq!(result, TailCallKind::ExitJump); + } + + #[test] + fn test_classify_continuation_target() { + // Phase 256 P1.10: Continuation calls (k_exit) are always ExitJump + // even when they would otherwise be classified as BackEdge + let result = classify_tail_call( + false, // is_entry_func_entry_block + true, // has_loop_header_phis + true, // has_boundary + true, // is_target_continuation ← this makes it ExitJump ); assert_eq!(result, TailCallKind::ExitJump); } diff --git a/src/mir/builder/control_flow/joinir/merge/value_collector.rs b/src/mir/builder/control_flow/joinir/merge/value_collector.rs index 583466da..d3006830 100644 --- a/src/mir/builder/control_flow/joinir/merge/value_collector.rs +++ b/src/mir/builder/control_flow/joinir/merge/value_collector.rs @@ -52,6 +52,15 @@ pub(super) fn collect_values( // Phase 188-Impl-3: Collect function parameters for tail call conversion function_params.insert(func_name.clone(), func.params.clone()); + // Phase 256 P1.10 DEBUG: Always log function params for debugging + trace.stderr_if( + &format!( + "[cf_loop/joinir] Phase 256 P1.10 DEBUG: Function '{}' params: {:?}", + func_name, func.params + ), + true, + ); + for block in func.blocks.values() { // Phase 189: Use remapper to collect values let block_values = remapper.collect_values_in_block(block); @@ -81,7 +90,7 @@ pub(super) fn collect_values( } } - // Phase 33-15: DO NOT collect parameter ValueIds into used_values + // Phase 33-15 / Phase 256 P1.10: DO NOT remap parameter ValueIds // // Reasoning: Parameters are implicitly defined at function entry in JoinIR. // When inlined into host MIR, if parameters are remapped, the remapped ValueIds @@ -92,13 +101,12 @@ pub(super) fn collect_values( // 1. BoundaryInjector Copy (for entry function: host_input → join_param) // 2. Tail call Copy (for recursive calls: call_arg → param) // - // We still store parameters in function_params for tail call handling, - // but don't add them to used_values so they won't be remapped. - // - // OLD CODE (removed): - // for param in &func.params { - // used_values.insert(*param); - // } + // Phase 256 P1.10: We must REMOVE params from used_values after block collection, + // because collect_values_in_block() adds all operand ValueIds including params. + // Without this removal, params get remapped and their remapped values are undefined. + for param in &func.params { + used_values.remove(param); + } } if debug {