From a09ce0cbff830117f3cece5fe84e0e4088f17437 Mon Sep 17 00:00:00 2001 From: nyash-codex Date: Sun, 7 Dec 2025 05:07:28 +0900 Subject: [PATCH] feat(joinir): Phase 33-14 JoinFragmentMeta for expr/carrier separation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces JoinFragmentMeta to distinguish between loop expression results and carrier variable updates, fixing SSA correctness issues. ## Changes ### New: JoinFragmentMeta struct (carrier_info.rs) - `expr_result: Option` - Loop as expression (return loop(...)) - `exit_meta: ExitMeta` - Carrier updates for variable_map - Helper methods: with_expr_result(), carrier_only(), empty() ### Pattern 2 Lowerer Updates - loop_with_break_minimal.rs: Returns (JoinModule, JoinFragmentMeta) - pattern2_with_break.rs: Sets boundary.expr_result from fragment_meta ### instruction_rewriter.rs - Phase 33-14: Only add to exit_phi_inputs when boundary.expr_result is Some - Phase 33-13: MergeResult struct with carrier_inputs map ### JoinInlineBoundary (inline_boundary.rs) - New field: expr_result: Option - All constructors updated with expr_result: None default ## Design Philosophy Previously, exit_phi_inputs mixed expr results with carrier updates, causing: - PHI inputs referencing undefined remapped values - SSA-undef errors in VM execution With JoinFragmentMeta: - expr_result β†’ exit_phi_inputs (generates PHI for expr value) - exit_meta β†’ carrier_inputs (updates variable_map via carrier PHIs) ## Test Results - Pattern 1 (carrier-only): Works correctly (no exit_phi_inputs) - Pattern 2 (expr result): Design complete, SSA-undef fix deferred to Phase 33-15 πŸ€– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../joinir/merge/exit_line/mod.rs | 23 ++-- .../joinir/merge/exit_line/reconnector.rs | 80 +++++++------ .../joinir/merge/exit_phi_builder.rs | 54 +++++++-- .../joinir/merge/instruction_rewriter.rs | 62 +++++++++- .../builder/control_flow/joinir/merge/mod.rs | 17 ++- .../joinir/patterns/exit_binding.rs | 2 + .../joinir/patterns/pattern2_with_break.rs | 9 +- .../builder/control_flow/joinir/routing.rs | 1 + src/mir/join_ir/lowering/carrier_info.rs | 109 ++++++++++++++++++ src/mir/join_ir/lowering/inline_boundary.rs | 34 ++++++ .../lowering/loop_with_break_minimal.rs | 18 +-- 11 files changed, 339 insertions(+), 70 deletions(-) diff --git a/src/mir/builder/control_flow/joinir/merge/exit_line/mod.rs b/src/mir/builder/control_flow/joinir/merge/exit_line/mod.rs index 19a57faf..2e5fee76 100644 --- a/src/mir/builder/control_flow/joinir/merge/exit_line/mod.rs +++ b/src/mir/builder/control_flow/joinir/merge/exit_line/mod.rs @@ -34,12 +34,17 @@ //! 3. Maintain backward compatibility (no breaking changes) //! 4. Enable independent testing and evolution //! +//! ## Phase 33-13: Carrier PHI Integration +//! The ExitLineOrchestrator now receives carrier_phis from exit_phi_builder +//! and uses them in ExitLineReconnector instead of the remapped exit values. +//! This ensures variable_map points to PHI-defined values (SSA-correct). +//! //! ## Future Extensions //! When implementing Pattern 4 (continue), new pattern lowerers can: //! ```rust //! let exit_bindings = ExitMetaCollector::collect(self, &exit_meta, debug); //! let boundary = JoinInlineBoundary::new_with_exits(...); -//! exit_line::ExitLineOrchestrator::execute(builder, &boundary, &remapper, debug); +//! exit_line::ExitLineOrchestrator::execute(builder, &boundary, &carrier_phis, debug); //! ``` //! No changes to exit_line module needed! @@ -49,6 +54,9 @@ pub mod meta_collector; pub use reconnector::ExitLineReconnector; pub use meta_collector::ExitMetaCollector; +use std::collections::BTreeMap; +use crate::mir::ValueId; + /// Phase 33-10-Refactor-P2: ExitLineOrchestrator facade /// /// Coordinates the entire exit line reconnection process for Phase 6. @@ -61,7 +69,7 @@ impl ExitLineOrchestrator { /// # Inputs /// - builder: MirBuilder with variable_map to update /// - boundary: JoinInlineBoundary with exit_bindings - /// - remapper: JoinIrIdRemapper for ValueId lookup + /// - carrier_phis: Map from carrier name to PHI dst ValueId (Phase 33-13) /// - debug: Debug logging enabled /// /// # Returns @@ -69,21 +77,22 @@ impl ExitLineOrchestrator { /// /// # Process /// 1. Validate exit_bindings (empty case) - /// 2. Delegate to ExitLineReconnector + /// 2. Delegate to ExitLineReconnector with carrier_phis pub fn execute( builder: &mut crate::mir::builder::MirBuilder, boundary: &crate::mir::join_ir::lowering::inline_boundary::JoinInlineBoundary, - remapper: &crate::mir::builder::joinir_id_remapper::JoinIrIdRemapper, + carrier_phis: &BTreeMap, debug: bool, ) -> Result<(), String> { if debug { eprintln!( - "[cf_loop/joinir/exit_line] ExitLineOrchestrator: Starting Phase 6 reconnection" + "[cf_loop/joinir/exit_line] ExitLineOrchestrator: Starting Phase 6 reconnection with {} carrier PHIs", + carrier_phis.len() ); } - // Delegate to ExitLineReconnector - ExitLineReconnector::reconnect(builder, boundary, remapper, debug)?; + // Phase 33-13: Delegate to ExitLineReconnector with carrier_phis + ExitLineReconnector::reconnect(builder, boundary, carrier_phis, debug)?; if debug { eprintln!("[cf_loop/joinir/exit_line] ExitLineOrchestrator: Phase 6 complete"); diff --git a/src/mir/builder/control_flow/joinir/merge/exit_line/reconnector.rs b/src/mir/builder/control_flow/joinir/merge/exit_line/reconnector.rs index 0bede334..44438bfe 100644 --- a/src/mir/builder/control_flow/joinir/merge/exit_line/reconnector.rs +++ b/src/mir/builder/control_flow/joinir/merge/exit_line/reconnector.rs @@ -3,32 +3,43 @@ //! Modularizes the exit line reconnection logic (Phase 6 from merge/mod.rs) //! into a focused, testable Box. //! -//! **Responsibility**: Update host variable_map with remapped exit values from JoinIR +//! **Responsibility**: Update host variable_map with PHI dst values from exit block +//! +//! # Phase 33-13 Architecture Change +//! +//! Before: Used remapper to get remapped exit values (SSA-incorrect!) +//! After: Uses carrier_phis map from exit_phi_builder (SSA-correct!) +//! +//! The remapped exit value is an INPUT to the PHI, not the OUTPUT. +//! Only the PHI dst is defined in the exit block and can be referenced later. -use crate::mir::builder::joinir_id_remapper::JoinIrIdRemapper; use crate::mir::builder::MirBuilder; use crate::mir::join_ir::lowering::inline_boundary::JoinInlineBoundary; +use crate::mir::ValueId; +use std::collections::BTreeMap; /// ExitLineReconnector: A Box that manages exit value reconnection /// /// # Design Notes /// /// ## Why Separate Box? -/// - Responsibility: Update host variable_map with remapped exit values -/// - Input: JoinInlineBoundary.exit_bindings + JoinIrIdRemapper +/// - Responsibility: Update host variable_map with PHI dst values +/// - Input: JoinInlineBoundary.exit_bindings + carrier_phis map /// - Output: Updated MirBuilder.variable_map /// - No side effects beyond variable_map updates /// -/// ## Phase 197-B Multi-Carrier Support -/// This Box implements the fix for multi-carrier exit binding: -/// - Before: Single exit_phi_result applied to all carriers (buggy!) -/// - After: Each carrier gets its specific remapped exit value -/// - Example: `sum` gets ValueId(456), `count` gets ValueId(457) +/// ## Phase 33-13 Carrier PHI Integration +/// This Box now uses carrier_phis from exit_phi_builder: +/// - Before: `variable_map[carrier] = remapper.get(join_exit)` (SSA-incorrect!) +/// - After: `variable_map[carrier] = carrier_phis[carrier]` (SSA-correct!) +/// +/// The key insight is that remapped exit values are PHI INPUTS, not OUTPUTS. +/// Only the PHI dst ValueId is defined in the exit block. /// /// ## Testing Strategy /// Can be tested independently: /// 1. Create mock boundary with exit_bindings -/// 2. Create mock remapper +/// 2. Create mock carrier_phis map /// 3. Call reconnect() and verify variable_map updates /// 4. No need to construct full merge/mod.rs machinery /// @@ -36,36 +47,36 @@ use crate::mir::join_ir::lowering::inline_boundary::JoinInlineBoundary; /// /// **Input**: /// - JoinInlineBoundary with exit_bindings (Carrier β†’ join_exit_value mappings) -/// - JoinIrIdRemapper (JoinIR local β†’ host function ValueId remappings) +/// - carrier_phis: Map from carrier name to PHI dst ValueId /// /// **Effect**: -/// - Updates builder.variable_map entries for each carrier with remapped exit values +/// - Updates builder.variable_map entries for each carrier with PHI dst values /// /// **Output**: /// - Result<(), String> (side effect on builder) pub struct ExitLineReconnector; impl ExitLineReconnector { - /// Reconnect exit values to host variable_map + /// Reconnect exit values to host variable_map using carrier PHI dst values /// - /// # Phase 197-B: Multi-carrier support + /// # Phase 33-13: Carrier PHI Integration /// - /// Previously, a single exit_phi_result was applied to all carriers, causing - /// all carriers to get the same value (e.g., both sum and count = 5). + /// Previously, we used the remapper to get remapped exit values. This was + /// SSA-incorrect because those values are PHI inputs, not outputs. /// - /// Now, we use each binding's join_exit_value and the remapper to find - /// the actual exit value for each carrier. + /// Now, we use the carrier_phis map from exit_phi_builder, which contains + /// the actual PHI dst ValueIds that are defined in the exit block. /// /// # Algorithm /// /// For each exit_binding: - /// 1. Look up the remapped ValueId for binding.join_exit_value - /// 2. Update variable_map[binding.carrier_name] with remapped value + /// 1. Look up the PHI dst for this carrier in carrier_phis + /// 2. Update variable_map[binding.carrier_name] with PHI dst /// 3. Log each update (if debug enabled) pub fn reconnect( builder: &mut MirBuilder, boundary: &JoinInlineBoundary, - remapper: &JoinIrIdRemapper, + carrier_phis: &BTreeMap, debug: bool, ) -> Result<(), String> { // Early return for empty exit_bindings @@ -78,33 +89,34 @@ impl ExitLineReconnector { if debug { eprintln!( - "[cf_loop/joinir/exit_line] ExitLineReconnector: Reconnecting {} exit bindings", - boundary.exit_bindings.len() + "[cf_loop/joinir/exit_line] ExitLineReconnector: Reconnecting {} exit bindings with {} carrier PHIs", + boundary.exit_bindings.len(), + carrier_phis.len() ); } // Process each exit binding for binding in &boundary.exit_bindings { - // Look up the remapped exit value - let remapped_exit = remapper.get_value(binding.join_exit_value); + // Phase 33-13: Look up the PHI dst for this carrier + let phi_dst = carrier_phis.get(&binding.carrier_name); if debug { eprintln!( - "[cf_loop/joinir/exit_line] ExitLineReconnector: Carrier '{}' join_exit={:?} β†’ remapped={:?}", - binding.carrier_name, binding.join_exit_value, remapped_exit + "[cf_loop/joinir/exit_line] ExitLineReconnector: Carrier '{}' β†’ phi_dst={:?}", + binding.carrier_name, phi_dst ); } - // Update variable_map if remapping found - if let Some(remapped_value) = remapped_exit { + // Update variable_map with PHI dst + if let Some(&phi_value) = phi_dst { if let Some(var_vid) = builder.variable_map.get_mut(&binding.carrier_name) { if debug { eprintln!( - "[cf_loop/joinir/exit_line] ExitLineReconnector: Updated variable_map['{}'] {:?} β†’ {:?}", - binding.carrier_name, var_vid, remapped_value + "[cf_loop/joinir/exit_line] ExitLineReconnector: Updated variable_map['{}'] {:?} β†’ {:?} (PHI dst)", + binding.carrier_name, var_vid, phi_value ); } - *var_vid = remapped_value; + *var_vid = phi_value; } else if debug { eprintln!( "[cf_loop/joinir/exit_line] ExitLineReconnector WARNING: Carrier '{}' not found in variable_map", @@ -113,8 +125,8 @@ impl ExitLineReconnector { } } else if debug { eprintln!( - "[cf_loop/joinir/exit_line] ExitLineReconnector WARNING: No remapped value for join_exit={:?}", - binding.join_exit_value + "[cf_loop/joinir/exit_line] ExitLineReconnector WARNING: No PHI dst for carrier '{}' (may be condition-only variable)", + binding.carrier_name ); } } diff --git a/src/mir/builder/control_flow/joinir/merge/exit_phi_builder.rs b/src/mir/builder/control_flow/joinir/merge/exit_phi_builder.rs index 775a0257..a0314283 100644 --- a/src/mir/builder/control_flow/joinir/merge/exit_phi_builder.rs +++ b/src/mir/builder/control_flow/joinir/merge/exit_phi_builder.rs @@ -1,22 +1,31 @@ //! JoinIR Exit PHI Builder //! -//! Constructs the exit block PHI node that merges return values +//! Constructs the exit block PHI nodes that merge return values //! from all inlined JoinIR functions. //! //! Phase 4 Extraction: Separated from merge_joinir_mir_blocks (lines 581-615) +//! Phase 33-13: Extended to support carrier PHIs for multi-carrier loops use crate::mir::{BasicBlock, BasicBlockId, MirInstruction, ValueId}; +use std::collections::BTreeMap; -/// Phase 5: Create exit block with PHI for return values from JoinIR functions +/// Phase 5: Create exit block with PHI for return values and carrier values /// /// Phase 189-Fix: Generate exit PHI if there are multiple return values. -/// If no return values, creates empty exit block and returns None. +/// Phase 33-13: Also generates PHI for each carrier variable. +/// +/// Returns: +/// - Option: The expr result PHI dst (if any return values) +/// - BTreeMap: Carrier name β†’ PHI dst mapping pub(super) fn build_exit_phi( builder: &mut crate::mir::builder::MirBuilder, exit_block_id: BasicBlockId, exit_phi_inputs: &[(BasicBlockId, ValueId)], + carrier_inputs: &BTreeMap>, debug: bool, -) -> Result, String> { +) -> Result<(Option, BTreeMap), String> { + let mut carrier_phis: BTreeMap = BTreeMap::new(); + let exit_phi_result_id = if let Some(ref mut func) = builder.current_function { let mut exit_block = BasicBlock::new(exit_block_id); @@ -35,7 +44,7 @@ pub(super) fn build_exit_phi( .push(crate::ast::Span::unknown()); if debug { eprintln!( - "[cf_loop/joinir] Exit block PHI: {:?} = phi {:?}", + "[cf_loop/joinir] Exit block PHI (expr result): {:?} = phi {:?}", phi_dst, exit_phi_inputs ); } @@ -44,11 +53,40 @@ pub(super) fn build_exit_phi( None }; + // Phase 33-13: Create PHI for each carrier variable + // This ensures that carrier exit values are properly merged when + // there are multiple paths to the exit block + for (carrier_name, inputs) in carrier_inputs { + if inputs.is_empty() { + continue; + } + + // Allocate a new ValueId for this carrier's PHI + let phi_dst = builder.value_gen.next(); + exit_block.instructions.push(MirInstruction::Phi { + dst: phi_dst, + inputs: inputs.clone(), + type_hint: None, + }); + exit_block + .instruction_spans + .push(crate::ast::Span::unknown()); + + carrier_phis.insert(carrier_name.clone(), phi_dst); + + if debug { + eprintln!( + "[cf_loop/joinir] Exit block PHI (carrier '{}'): {:?} = phi {:?}", + carrier_name, phi_dst, inputs + ); + } + } + func.add_block(exit_block); if debug { eprintln!( - "[cf_loop/joinir] Created exit block: {:?}", - exit_block_id + "[cf_loop/joinir] Created exit block: {:?} with {} carrier PHIs", + exit_block_id, carrier_phis.len() ); } phi_result @@ -56,5 +94,5 @@ pub(super) fn build_exit_phi( None }; - Ok(exit_phi_result_id) + Ok((exit_phi_result_id, carrier_phis)) } 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 60ccfb55..37bec1bf 100644 --- a/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs +++ b/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs @@ -10,11 +10,22 @@ use crate::mir::builder::joinir_id_remapper::JoinIrIdRemapper; use crate::mir::join_ir::lowering::inline_boundary::JoinInlineBoundary; use std::collections::HashMap; +/// Phase 33-13: Return type for merge_and_rewrite +/// +/// Contains all information needed for exit PHI construction. +pub struct MergeResult { + /// The ID of the exit block where all Returns jump to + pub exit_block_id: BasicBlockId, + /// Vec of (from_block, return_value) for expr result PHI generation + pub exit_phi_inputs: Vec<(BasicBlockId, ValueId)>, + /// Map of carrier_name β†’ Vec of (from_block, exit_value) for carrier PHI generation + pub carrier_inputs: std::collections::BTreeMap>, +} + /// Phase 4: Merge ALL functions and rewrite instructions /// /// Returns: -/// - exit_block_id: The ID of the exit block where all Returns jump to -/// - exit_phi_inputs: Vec of (from_block, return_value) for exit PHI generation +/// - MergeResult containing exit_block_id, exit_phi_inputs, and carrier_inputs pub(super) fn merge_and_rewrite( builder: &mut crate::mir::builder::MirBuilder, mir_module: &MirModule, @@ -23,7 +34,7 @@ pub(super) fn merge_and_rewrite( function_params: &HashMap>, boundary: Option<&JoinInlineBoundary>, debug: bool, -) -> Result<(BasicBlockId, Vec<(BasicBlockId, ValueId)>), String> { +) -> Result { // Create exit block for Return conversion (single for all functions) let exit_block_id = builder.block_gen.next(); if debug { @@ -38,6 +49,11 @@ pub(super) fn merge_and_rewrite( // Phase 189-Fix: Collect return values from JoinIR functions for exit PHI let mut exit_phi_inputs: Vec<(BasicBlockId, ValueId)> = Vec::new(); + // Phase 33-13: Collect carrier exit values for carrier PHIs + // Map from carrier_name to Vec of (from_block, exit_value) + let mut carrier_inputs: std::collections::BTreeMap> = + std::collections::BTreeMap::new(); + // Build function_entry_map for Callβ†’Jump conversion let mut function_entry_map: HashMap = HashMap::new(); for (func_name, func) in &mir_module.functions { @@ -314,6 +330,7 @@ pub(super) fn merge_and_rewrite( // Convert Return to Jump to exit block // All functions return to same exit block (Phase 189) // Phase 189-Fix: Add Copy instruction to pass return value to exit PHI + // Phase 33-14: Only add to exit_phi_inputs if boundary.expr_result is Some if let Some(ret_val) = value { let remapped_val = remapper.get_value(*ret_val).unwrap_or(*ret_val); if debug { @@ -322,9 +339,38 @@ pub(super) fn merge_and_rewrite( remapped_val ); } - // Collect (from_block, return_value) for exit PHI generation - exit_phi_inputs.push((new_block_id, remapped_val)); + // Phase 33-14: Only collect for exit PHI if this loop has an expr result + // This separates "loop as expression" from "carrier-only loop" + let has_expr_result = boundary.map(|b| b.expr_result.is_some()).unwrap_or(true); + if has_expr_result { + // Collect (from_block, return_value) for exit PHI generation + exit_phi_inputs.push((new_block_id, remapped_val)); + } else if debug { + eprintln!( + "[cf_loop/joinir] Phase 33-14: Skipping exit_phi_inputs (carrier-only pattern)" + ); + } } + + // Phase 33-13: Collect carrier exit values for carrier PHIs + // When jumping to exit_block, collect each carrier's exit value + if let Some(b) = boundary { + for binding in &b.exit_bindings { + if let Some(remapped_exit) = remapper.get_value(binding.join_exit_value) { + carrier_inputs + .entry(binding.carrier_name.clone()) + .or_insert_with(Vec::new) + .push((new_block_id, remapped_exit)); + if debug { + eprintln!( + "[cf_loop/joinir] Phase 33-13: Carrier '{}' exit: ({:?}, {:?})", + binding.carrier_name, new_block_id, remapped_exit + ); + } + } + } + } + MirInstruction::Jump { target: exit_block_id, } @@ -429,5 +475,9 @@ pub(super) fn merge_and_rewrite( } } - Ok((exit_block_id, exit_phi_inputs)) + Ok(MergeResult { + exit_block_id, + exit_phi_inputs, + carrier_inputs, + }) } diff --git a/src/mir/builder/control_flow/joinir/merge/mod.rs b/src/mir/builder/control_flow/joinir/merge/mod.rs index 7df3352c..e937b577 100644 --- a/src/mir/builder/control_flow/joinir/merge/mod.rs +++ b/src/mir/builder/control_flow/joinir/merge/mod.rs @@ -106,7 +106,7 @@ pub(in crate::mir::builder) fn merge_joinir_mir_blocks( remap_values(builder, &used_values, &mut remapper, debug)?; // Phase 4: Merge blocks and rewrite instructions - let (exit_block_id, exit_phi_inputs) = instruction_rewriter::merge_and_rewrite( + let merge_result = instruction_rewriter::merge_and_rewrite( builder, mir_module, &mut remapper, @@ -116,21 +116,26 @@ pub(in crate::mir::builder) fn merge_joinir_mir_blocks( debug, )?; - // Phase 5: Build exit PHI - let exit_phi_result_id = exit_phi_builder::build_exit_phi( + // Phase 5: Build exit PHI (expr result and carrier PHIs) + // Phase 33-13: Now also builds carrier PHIs and returns their mapping + let (exit_phi_result_id, carrier_phis) = exit_phi_builder::build_exit_phi( builder, - exit_block_id, - &exit_phi_inputs, + merge_result.exit_block_id, + &merge_result.exit_phi_inputs, + &merge_result.carrier_inputs, debug, )?; // Phase 6: Reconnect boundary (if specified) // Phase 197-B: Pass remapper to enable per-carrier exit value lookup // Phase 33-10-Refactor-P3: Delegate to ExitLineOrchestrator + // Phase 33-13: Pass carrier_phis for proper variable_map update if let Some(boundary) = boundary { - exit_line::ExitLineOrchestrator::execute(builder, boundary, &remapper, debug)?; + exit_line::ExitLineOrchestrator::execute(builder, boundary, &carrier_phis, debug)?; } + let exit_block_id = merge_result.exit_block_id; + // Jump from current block to entry function's entry block let (entry_func_name, entry_func) = mir_module .functions diff --git a/src/mir/builder/control_flow/joinir/patterns/exit_binding.rs b/src/mir/builder/control_flow/joinir/patterns/exit_binding.rs index 1d4a463d..221db5b5 100644 --- a/src/mir/builder/control_flow/joinir/patterns/exit_binding.rs +++ b/src/mir/builder/control_flow/joinir/patterns/exit_binding.rs @@ -387,6 +387,8 @@ mod tests { join_outputs: vec![], exit_bindings: vec![], // Phase 171: Add missing field condition_inputs: vec![], // Phase 171: Add missing field + condition_bindings: vec![], // Phase 171-fix: Add missing field + expr_result: None, // Phase 33-14: Add missing field }; builder.apply_to_boundary(&mut boundary) diff --git a/src/mir/builder/control_flow/joinir/patterns/pattern2_with_break.rs b/src/mir/builder/control_flow/joinir/patterns/pattern2_with_break.rs index d1f27479..508e9f24 100644 --- a/src/mir/builder/control_flow/joinir/patterns/pattern2_with_break.rs +++ b/src/mir/builder/control_flow/joinir/patterns/pattern2_with_break.rs @@ -142,8 +142,8 @@ impl MirBuilder { }; // Phase 169 / Phase 171-fix / Phase 172-3: Call Pattern 2 lowerer with ConditionEnv - // Now returns (JoinModule, ExitMeta) for proper exit_bindings construction - let (join_module, exit_meta) = match lower_loop_with_break_minimal(scope, condition, &env, &loop_var_name) { + // Phase 33-14: Now returns (JoinModule, JoinFragmentMeta) for expr_result + carrier separation + let (join_module, fragment_meta) = match lower_loop_with_break_minimal(scope, condition, &env, &loop_var_name) { Ok((module, meta)) => (module, meta), Err(e) => { // Phase 195: Use unified trace @@ -152,6 +152,9 @@ impl MirBuilder { } }; + // Phase 33-14: Extract exit_meta from fragment_meta for backward compatibility + let exit_meta = &fragment_meta.exit_meta; + // Phase 195: Use unified trace trace::trace().joinir_stats( "pattern2", @@ -186,12 +189,14 @@ impl MirBuilder { let exit_bindings = ExitMetaCollector::collect(self, &exit_meta, debug); // Phase 172-3: Build boundary with both condition_bindings and exit_bindings + // Phase 33-14: Set expr_result from fragment_meta let mut boundary = crate::mir::join_ir::lowering::inline_boundary::JoinInlineBoundary::new_inputs_only( vec![ValueId(0)], // JoinIR's main() parameter (loop variable init) vec![loop_var_id], // Host's loop variable ); boundary.condition_bindings = condition_bindings; boundary.exit_bindings = exit_bindings.clone(); + boundary.expr_result = fragment_meta.expr_result; // Phase 33-14: Pass expr_result to merger // Phase 189: Capture exit PHI result (now used for reconnect) let _ = self.merge_joinir_mir_blocks(&mir_module, Some(&boundary), debug)?; diff --git a/src/mir/builder/control_flow/joinir/routing.rs b/src/mir/builder/control_flow/joinir/routing.rs index b2af1fd3..11b2f976 100644 --- a/src/mir/builder/control_flow/joinir/routing.rs +++ b/src/mir/builder/control_flow/joinir/routing.rs @@ -75,6 +75,7 @@ impl MirBuilder { // Phase 170-A-1: Test methods (simplified versions) "TrimTest.trim/1" => true, "Main.trim/1" => true, // Phase 171-fix: Main box variant + "Main.trim_string_simple/1" => true, // Phase 33-13: Simple trim variant _ => false, }; diff --git a/src/mir/join_ir/lowering/carrier_info.rs b/src/mir/join_ir/lowering/carrier_info.rs index 6740df4b..ad6ca8b1 100644 --- a/src/mir/join_ir/lowering/carrier_info.rs +++ b/src/mir/join_ir/lowering/carrier_info.rs @@ -206,6 +206,115 @@ pub struct ExitMeta { pub exit_values: Vec<(String, ValueId)>, } +/// Phase 33-14: JoinFragmentMeta - Distinguishes expr result from carrier updates +/// +/// ## Purpose +/// +/// Separates two distinct use cases for JoinIR loops: +/// +/// 1. **Expr Result Pattern** (joinir_min_loop.hako): +/// ```nyash +/// local result = loop(...) { ... } // Loop used as expression +/// return result +/// ``` +/// Here, the k_exit return value is the "expr result" that should go to exit_phi_inputs. +/// +/// 2. **Carrier Update Pattern** (trim pattern): +/// ```nyash +/// loop(...) { start = start + 1 } // Loop used for side effects +/// print(start) // Use carrier after loop +/// ``` +/// Here, there's no "expr result" - only carrier variable updates. +/// +/// ## SSA Correctness +/// +/// Previously, exit_phi_inputs mixed expr results with carrier updates, causing: +/// - PHI inputs that referenced undefined remapped values +/// - SSA-undef errors in VM execution +/// +/// With JoinFragmentMeta: +/// - `expr_result`: Only goes to exit_phi_inputs (generates PHI for expr value) +/// - `exit_meta`: Only goes to carrier_inputs (updates variable_map via carrier PHIs) +/// +/// ## Example: Pattern 2 (joinir_min_loop.hako) +/// +/// ```rust +/// JoinFragmentMeta { +/// expr_result: Some(i_exit), // k_exit returns i as expr value +/// exit_meta: ExitMeta::single("i".to_string(), i_exit), // Also a carrier +/// } +/// ``` +/// +/// ## Example: Pattern 3 (trim pattern) +/// +/// ```rust +/// JoinFragmentMeta { +/// expr_result: None, // Loop doesn't return a value +/// exit_meta: ExitMeta::multiple(vec![ +/// ("start".to_string(), start_exit), +/// ("end".to_string(), end_exit), +/// ]), +/// } +/// ``` +#[derive(Debug, Clone)] +pub struct JoinFragmentMeta { + /// Expression result ValueId from k_exit (JoinIR-local) + /// + /// - `Some(vid)`: Loop is used as expression, k_exit's return value β†’ exit_phi_inputs + /// - `None`: Loop is used for side effects only, no PHI for expr value + pub expr_result: Option, + + /// Carrier variable exit bindings (existing ExitMeta) + /// + /// Maps carrier names to their JoinIR-local exit values. + /// These go to carrier_inputs for carrier PHI generation. + pub exit_meta: ExitMeta, +} + +impl JoinFragmentMeta { + /// Create JoinFragmentMeta for expression result pattern + /// + /// Use when the loop returns a value (like `return loop(...)`). + pub fn with_expr_result(expr_result: ValueId, exit_meta: ExitMeta) -> Self { + Self { + expr_result: Some(expr_result), + exit_meta, + } + } + + /// Create JoinFragmentMeta for carrier-only pattern + /// + /// Use when the loop only updates carriers (like trim pattern). + pub fn carrier_only(exit_meta: ExitMeta) -> Self { + Self { + expr_result: None, + exit_meta, + } + } + + /// Create empty JoinFragmentMeta (no expr result, no carriers) + pub fn empty() -> Self { + Self { + expr_result: None, + exit_meta: ExitMeta::empty(), + } + } + + /// Check if this fragment has an expression result + pub fn has_expr_result(&self) -> bool { + self.expr_result.is_some() + } + + /// Phase 33-14: Backward compatibility - convert to ExitMeta + /// + /// During migration, some code may still expect ExitMeta. + /// This extracts just the carrier bindings. + #[deprecated(since = "33-14", note = "Use exit_meta directly for carrier access")] + pub fn to_exit_meta(&self) -> ExitMeta { + self.exit_meta.clone() + } +} + impl ExitMeta { /// Create new ExitMeta with no exit values pub fn empty() -> Self { diff --git a/src/mir/join_ir/lowering/inline_boundary.rs b/src/mir/join_ir/lowering/inline_boundary.rs index e8121aca..3a6f95eb 100644 --- a/src/mir/join_ir/lowering/inline_boundary.rs +++ b/src/mir/join_ir/lowering/inline_boundary.rs @@ -205,6 +205,33 @@ pub struct JoinInlineBoundary { /// /// This replaces `condition_inputs` to ensure proper ValueId separation. pub condition_bindings: Vec, + + /// Phase 33-14: Expression result ValueId (JoinIR-local) + /// + /// If the loop is used as an expression (like `return loop(...)`), this field + /// contains the JoinIR-local ValueId of k_exit's return value. + /// + /// - `Some(ValueId)`: Loop returns a value β†’ k_exit return goes to exit_phi_inputs + /// - `None`: Loop only updates carriers β†’ no exit_phi_inputs generation + /// + /// # Example: joinir_min_loop.hako (expr result pattern) + /// + /// ```nyash + /// loop(i < 3) { if (i >= 2) { break } i = i + 1 } + /// return i + /// ``` + /// + /// Here, `expr_result = Some(i_exit)` because the loop's result is used. + /// + /// # Example: trim pattern (carrier-only) + /// + /// ```nyash + /// loop(start < end) { start = start + 1 } + /// print(start) // Uses carrier after loop + /// ``` + /// + /// Here, `expr_result = None` because the loop doesn't return a value. + pub expr_result: Option, } impl JoinInlineBoundary { @@ -229,6 +256,7 @@ impl JoinInlineBoundary { #[allow(deprecated)] condition_inputs: vec![], // Phase 171: Default to empty (deprecated) condition_bindings: vec![], // Phase 171-fix: Default to empty + expr_result: None, // Phase 33-14: Default to carrier-only pattern } } @@ -268,6 +296,7 @@ impl JoinInlineBoundary { #[allow(deprecated)] condition_inputs: vec![], // Phase 171: Default to empty (deprecated) condition_bindings: vec![], // Phase 171-fix: Default to empty + expr_result: None, // Phase 33-14 } } @@ -303,6 +332,7 @@ impl JoinInlineBoundary { #[allow(deprecated)] condition_inputs: vec![], // Phase 171: Default to empty (deprecated) condition_bindings: vec![], // Phase 171-fix: Default to empty + expr_result: None, // Phase 33-14 } } @@ -360,6 +390,7 @@ impl JoinInlineBoundary { #[allow(deprecated)] condition_inputs: vec![], // Phase 171: Default to empty (deprecated) condition_bindings: vec![], // Phase 171-fix: Default to empty + expr_result: None, // Phase 33-14 } } @@ -403,6 +434,7 @@ impl JoinInlineBoundary { #[allow(deprecated)] condition_inputs, condition_bindings: vec![], // Phase 171-fix: Will be populated by new constructor + expr_result: None, // Phase 33-14 } } @@ -450,6 +482,7 @@ impl JoinInlineBoundary { #[allow(deprecated)] condition_inputs, condition_bindings: vec![], // Phase 171-fix: Will be populated by new constructor + expr_result: None, // Phase 33-14 } } @@ -504,6 +537,7 @@ impl JoinInlineBoundary { #[allow(deprecated)] condition_inputs: vec![], // Deprecated, use condition_bindings instead condition_bindings, + expr_result: None, // Phase 33-14 } } } diff --git a/src/mir/join_ir/lowering/loop_with_break_minimal.rs b/src/mir/join_ir/lowering/loop_with_break_minimal.rs index 2e008913..b2073255 100644 --- a/src/mir/join_ir/lowering/loop_with_break_minimal.rs +++ b/src/mir/join_ir/lowering/loop_with_break_minimal.rs @@ -56,7 +56,7 @@ //! Following the "80/20 rule" from CLAUDE.md - get it working first, generalize later. use crate::ast::ASTNode; -use crate::mir::join_ir::lowering::carrier_info::ExitMeta; +use crate::mir::join_ir::lowering::carrier_info::{ExitMeta, JoinFragmentMeta}; use crate::mir::join_ir::lowering::condition_to_joinir::{lower_condition_to_joinir, ConditionEnv}; use crate::mir::join_ir::lowering::loop_scope_shape::LoopScopeShape; use crate::mir::join_ir::{ @@ -109,9 +109,11 @@ use crate::mir::ValueId; /// This ensures JoinIR never accesses HOST ValueIds directly. /// /// # Phase 172-3: ExitMeta Return +/// # Phase 33-14: JoinFragmentMeta Return /// -/// Returns `(JoinModule, ExitMeta)` where ExitMeta contains the JoinIR-local ValueId -/// of the k_exit parameter. This allows the caller to build proper exit_bindings. +/// Returns `(JoinModule, JoinFragmentMeta)` where: +/// - `expr_result`: k_exit's return value (i_exit) - this is what `return i` uses +/// - `exit_meta`: carrier bindings for variable_map updates /// /// # Arguments /// @@ -121,7 +123,7 @@ pub fn lower_loop_with_break_minimal( condition: &ASTNode, env: &ConditionEnv, loop_var_name: &str, -) -> Result<(JoinModule, ExitMeta), String> { +) -> Result<(JoinModule, JoinFragmentMeta), String> { // Phase 188-Impl-2: Use local ValueId allocator (sequential from 0) // JoinIR has NO knowledge of host ValueIds - boundary handled separately let mut value_counter = 0u32; @@ -305,9 +307,11 @@ pub fn lower_loop_with_break_minimal( eprintln!("[joinir/pattern2] Condition from AST (not hardcoded)"); eprintln!("[joinir/pattern2] Exit PHI: k_exit receives i from both natural exit and break"); - // Phase 172-3: Build ExitMeta with k_exit parameter's ValueId + // Phase 172-3 β†’ Phase 33-14: Build JoinFragmentMeta with expr_result + // Pattern 2: Loop is used as expression β†’ `return i` means k_exit's return is expr_result let exit_meta = ExitMeta::single(loop_var_name.to_string(), i_exit); - eprintln!("[joinir/pattern2] Phase 172-3: ExitMeta {{ {} β†’ {:?} }}", loop_var_name, i_exit); + let fragment_meta = JoinFragmentMeta::with_expr_result(i_exit, exit_meta); + eprintln!("[joinir/pattern2] Phase 33-14: JoinFragmentMeta {{ expr_result: {:?}, carrier: {} }}", i_exit, loop_var_name); - Ok((join_module, exit_meta)) + Ok((join_module, fragment_meta)) }