diff --git a/src/mir/phi_core/local_scope_inspector.rs b/src/mir/phi_core/local_scope_inspector.rs new file mode 100644 index 00000000..2446fad7 --- /dev/null +++ b/src/mir/phi_core/local_scope_inspector.rs @@ -0,0 +1,321 @@ +/// LocalScopeInspectorBox - Variable definition tracker +/// +/// # Purpose +/// +/// This box tracks which variables are defined in which basic blocks. +/// Used by LoopVarClassBox to determine if a body-local variable is +/// defined in all exit predecessors (making it eligible for exit PHI). +/// +/// # Responsibility +/// +/// - Record variable definitions per block +/// - Query if a variable is defined in specific blocks +/// - Query if a variable is defined in ALL of a set of blocks +/// +/// # Design Philosophy (Box Theory) +/// +/// This box has ONE job: track definition locations. +/// It doesn't know about loops, PHI nodes, or exit blocks. +/// It's a pure data structure that other boxes can query. + +use crate::mir::{BasicBlockId, ValueId}; +use std::collections::{HashMap, HashSet}; + +/// Tracks which variables are defined in which blocks +#[derive(Debug, Clone, Default)] +pub struct LocalScopeInspectorBox { + /// Variable name → Set of blocks where it's defined + var_definitions: HashMap>, +} + +impl LocalScopeInspectorBox { + /// Create a new empty inspector + pub fn new() -> Self { + Self { + var_definitions: HashMap::new(), + } + } + + /// Record that a variable is defined in a specific block + /// + /// # Example + /// ``` + /// let mut inspector = LocalScopeInspectorBox::new(); + /// inspector.record_definition("ch", BasicBlockId::new(5)); + /// inspector.record_definition("i", BasicBlockId::new(2)); + /// inspector.record_definition("i", BasicBlockId::new(5)); // i defined in multiple blocks + /// ``` + pub fn record_definition(&mut self, var_name: &str, block: BasicBlockId) { + self.var_definitions + .entry(var_name.to_string()) + .or_insert_with(HashSet::new) + .insert(block); + } + + /// Record definitions from a snapshot (block_id, vars) + /// + /// # Example + /// ``` + /// let snapshot = HashMap::from([ + /// ("i".to_string(), ValueId(1)), + /// ("n".to_string(), ValueId(2)), + /// ]); + /// inspector.record_snapshot(BasicBlockId::new(5), &snapshot); + /// ``` + pub fn record_snapshot( + &mut self, + block: BasicBlockId, + vars: &HashMap, + ) { + for var_name in vars.keys() { + self.record_definition(var_name, block); + } + } + + /// Check if a variable is defined in a specific block + /// + /// # Returns + /// - `true` if the variable is defined in the block + /// - `false` if the variable is not defined in the block, or doesn't exist + pub fn is_defined_in(&self, var_name: &str, block: BasicBlockId) -> bool { + self.var_definitions + .get(var_name) + .map(|blocks| blocks.contains(&block)) + .unwrap_or(false) + } + + /// Check if a variable is defined in ALL of the specified blocks + /// + /// # Returns + /// - `true` if the variable is defined in every block in `required_blocks` + /// - `false` if the variable is missing from any block, or doesn't exist + /// + /// # Example + /// ``` + /// // Variable "ch" is only defined in block 5 + /// inspector.record_definition("ch", BasicBlockId::new(5)); + /// + /// let exit_preds = vec![BasicBlockId::new(2), BasicBlockId::new(5)]; + /// assert!(!inspector.is_defined_in_all("ch", &exit_preds)); // false: missing block 2 + /// + /// // Variable "i" is defined in both blocks + /// inspector.record_definition("i", BasicBlockId::new(2)); + /// inspector.record_definition("i", BasicBlockId::new(5)); + /// assert!(inspector.is_defined_in_all("i", &exit_preds)); // true: in all blocks + /// ``` + pub fn is_defined_in_all(&self, var_name: &str, required_blocks: &[BasicBlockId]) -> bool { + if let Some(defining_blocks) = self.var_definitions.get(var_name) { + required_blocks.iter().all(|block| defining_blocks.contains(block)) + } else { + // Variable doesn't exist at all + false + } + } + + /// Get all blocks where a variable is defined + /// + /// # Returns + /// - Vec of BasicBlockIds where the variable is defined + /// - Empty vec if the variable doesn't exist + pub fn get_defining_blocks(&self, var_name: &str) -> Vec { + self.var_definitions + .get(var_name) + .map(|blocks| blocks.iter().copied().collect()) + .unwrap_or_default() + } + + /// Get all tracked variables + pub fn all_variables(&self) -> Vec { + self.var_definitions.keys().cloned().collect() + } + + /// Get the number of blocks where a variable is defined + pub fn definition_count(&self, var_name: &str) -> usize { + self.var_definitions + .get(var_name) + .map(|blocks| blocks.len()) + .unwrap_or(0) + } +} + +// ============================================================================ +// Unit Tests +// ============================================================================ + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_record_and_query_single_definition() { + let mut inspector = LocalScopeInspectorBox::new(); + + inspector.record_definition("ch", BasicBlockId::new(5)); + + assert!(inspector.is_defined_in("ch", BasicBlockId::new(5))); + assert!(!inspector.is_defined_in("ch", BasicBlockId::new(2))); + assert!(!inspector.is_defined_in("unknown", BasicBlockId::new(5))); + } + + #[test] + fn test_record_multiple_blocks() { + let mut inspector = LocalScopeInspectorBox::new(); + + inspector.record_definition("i", BasicBlockId::new(2)); + inspector.record_definition("i", BasicBlockId::new(5)); + inspector.record_definition("i", BasicBlockId::new(7)); + + assert!(inspector.is_defined_in("i", BasicBlockId::new(2))); + assert!(inspector.is_defined_in("i", BasicBlockId::new(5))); + assert!(inspector.is_defined_in("i", BasicBlockId::new(7))); + assert!(!inspector.is_defined_in("i", BasicBlockId::new(10))); + } + + #[test] + fn test_is_defined_in_all_success() { + let mut inspector = LocalScopeInspectorBox::new(); + + inspector.record_definition("i", BasicBlockId::new(2)); + inspector.record_definition("i", BasicBlockId::new(5)); + inspector.record_definition("i", BasicBlockId::new(7)); + + let required = vec![ + BasicBlockId::new(2), + BasicBlockId::new(5), + ]; + + assert!(inspector.is_defined_in_all("i", &required)); + } + + #[test] + fn test_is_defined_in_all_failure() { + let mut inspector = LocalScopeInspectorBox::new(); + + // "ch" only defined in block 5 + inspector.record_definition("ch", BasicBlockId::new(5)); + + let required = vec![ + BasicBlockId::new(2), + BasicBlockId::new(5), + ]; + + // Should fail because block 2 is missing + assert!(!inspector.is_defined_in_all("ch", &required)); + } + + #[test] + fn test_is_defined_in_all_unknown_variable() { + let inspector = LocalScopeInspectorBox::new(); + + let required = vec![BasicBlockId::new(2)]; + + assert!(!inspector.is_defined_in_all("unknown", &required)); + } + + #[test] + fn test_get_defining_blocks() { + let mut inspector = LocalScopeInspectorBox::new(); + + inspector.record_definition("x", BasicBlockId::new(3)); + inspector.record_definition("x", BasicBlockId::new(7)); + inspector.record_definition("x", BasicBlockId::new(1)); + + let mut blocks = inspector.get_defining_blocks("x"); + blocks.sort_by_key(|b| b.0); + + assert_eq!( + blocks, + vec![ + BasicBlockId::new(1), + BasicBlockId::new(3), + BasicBlockId::new(7), + ] + ); + } + + #[test] + fn test_get_defining_blocks_unknown() { + let inspector = LocalScopeInspectorBox::new(); + + assert_eq!(inspector.get_defining_blocks("unknown"), Vec::::new()); + } + + #[test] + fn test_record_snapshot() { + let mut inspector = LocalScopeInspectorBox::new(); + + let mut snapshot = HashMap::new(); + snapshot.insert("i".to_string(), ValueId(10)); + snapshot.insert("n".to_string(), ValueId(20)); + snapshot.insert("ch".to_string(), ValueId(712)); + + inspector.record_snapshot(BasicBlockId::new(5), &snapshot); + + assert!(inspector.is_defined_in("i", BasicBlockId::new(5))); + assert!(inspector.is_defined_in("n", BasicBlockId::new(5))); + assert!(inspector.is_defined_in("ch", BasicBlockId::new(5))); + assert!(!inspector.is_defined_in("i", BasicBlockId::new(2))); + } + + #[test] + fn test_all_variables() { + let mut inspector = LocalScopeInspectorBox::new(); + + inspector.record_definition("a", BasicBlockId::new(1)); + inspector.record_definition("b", BasicBlockId::new(2)); + inspector.record_definition("c", BasicBlockId::new(3)); + + let mut vars = inspector.all_variables(); + vars.sort(); + + assert_eq!(vars, vec!["a", "b", "c"]); + } + + #[test] + fn test_definition_count() { + let mut inspector = LocalScopeInspectorBox::new(); + + inspector.record_definition("x", BasicBlockId::new(1)); + inspector.record_definition("x", BasicBlockId::new(2)); + inspector.record_definition("x", BasicBlockId::new(3)); + + assert_eq!(inspector.definition_count("x"), 3); + assert_eq!(inspector.definition_count("unknown"), 0); + } + + /// Test the skip_whitespace PHI bug scenario + #[test] + fn test_skip_whitespace_scenario() { + let mut inspector = LocalScopeInspectorBox::new(); + + // Simulate skip_whitespace: + // - Variables i, n, s are defined in all blocks (header + breaks) + // - Variable ch is only defined in block 5 (break path 2) + + let block_2 = BasicBlockId::new(2); // header / break path 1 + let block_5 = BasicBlockId::new(5); // break path 2 + let block_7 = BasicBlockId::new(7); // latch (hypothetical) + + // i, n, s are in all blocks + for var in &["i", "n", "s"] { + inspector.record_definition(var, block_2); + inspector.record_definition(var, block_5); + inspector.record_definition(var, block_7); + } + + // ch is only in block 5 + inspector.record_definition("ch", block_5); + + let exit_preds = vec![block_2, block_5, block_7]; + + // i, n, s should be in all exit preds + assert!(inspector.is_defined_in_all("i", &exit_preds)); + assert!(inspector.is_defined_in_all("n", &exit_preds)); + assert!(inspector.is_defined_in_all("s", &exit_preds)); + + // ch should NOT be in all exit preds (missing block 2 and 7) + assert!(!inspector.is_defined_in_all("ch", &exit_preds)); + + // This means ch should NOT get an exit PHI! + } +} diff --git a/src/mir/phi_core/loop_snapshot_merge.rs b/src/mir/phi_core/loop_snapshot_merge.rs index 3bf3f782..71cd1dc3 100644 --- a/src/mir/phi_core/loop_snapshot_merge.rs +++ b/src/mir/phi_core/loop_snapshot_merge.rs @@ -18,6 +18,10 @@ use crate::mir::{BasicBlockId, ValueId}; use std::collections::{HashMap, HashSet}; +// Option C PHI bug fix: Use box-based classification +use super::local_scope_inspector::LocalScopeInspectorBox; +use super::loop_var_classifier::LoopVarClassBox; + /// Phase 25.2: continue/break/exit時のスナップショットマージを統一管理 #[derive(Debug, Clone)] pub struct LoopSnapshotMergeBox { @@ -148,6 +152,129 @@ impl LoopSnapshotMergeBox { Ok(result) } + /// Option C: exit_merge with variable classification + /// + /// ## 目的 + /// + /// PHI pred mismatch バグを防ぐため、body-local変数が全exit predecessorで + /// 定義されているかチェックし、定義されていない変数はexit PHIを生成しない。 + /// + /// ## 引数 + /// + /// - `header_id`: header ブロックのID(fallthrough元) + /// - `header_vals`: header での変数値 + /// - `exit_snapshots`: 各 break 文での変数スナップショット + /// - `exit_preds`: Exit block の実際のCFG predecessors(重要!snapshotだけではない) + /// - `pinned_vars`: Loop-crossing parameters(常にPHI生成) + /// - `carrier_vars`: Loop-modified variables(常にPHI生成) + /// - `inspector`: LocalScopeInspectorBox(変数定義位置追跡) + /// + /// ## 戻り値 + /// + /// 各変数ごとの PHI 入力(predecessor, value)のリスト + /// + /// ## Option C ロジック + /// + /// 1. LocalScopeInspectorBox で変数定義位置を追跡 + /// 2. LoopVarClassBox で変数を分類 + /// 3. BodyLocalInternal(全exit predsで定義されていない)変数は skip + /// + /// ## 例(skip_whitespace バグ修正) + /// + /// ``` + /// loop(1 == 1) { + /// if i >= n { break } // exit pred 1: ch doesn't exist + /// local ch = s.substring() // ch defined here + /// if ch == " " { ... } else { break } // exit pred 2: ch exists + /// } + /// // ch は BodyLocalInternal → exit PHI 生成しない(バグ修正!) + /// ``` + pub fn merge_exit_with_classification( + header_id: BasicBlockId, + header_vals: &HashMap, + exit_snapshots: &[(BasicBlockId, HashMap)], + exit_preds: &[BasicBlockId], + pinned_vars: &[String], + carrier_vars: &[String], + inspector: &LocalScopeInspectorBox, + ) -> Result>, String> { + let mut result: HashMap> = HashMap::new(); + + let debug = std::env::var("NYASH_OPTION_C_DEBUG").is_ok(); + if debug { + eprintln!("[Option C] merge_exit_with_classification called"); + eprintln!("[Option C] exit_preds: {:?}", exit_preds); + eprintln!("[Option C] pinned_vars: {:?}", pinned_vars); + eprintln!("[Option C] carrier_vars: {:?}", carrier_vars); + } + + // LoopVarClassBox でフィルタリング + let classifier = LoopVarClassBox::new(); + + // すべての変数名を収集 + let mut all_vars: HashSet = HashSet::new(); + all_vars.extend(header_vals.keys().cloned()); + for (_, snap) in exit_snapshots { + all_vars.extend(snap.keys().cloned()); + } + + // 各変数を分類して、exit PHI が必要なもののみ処理 + for var_name in all_vars { + // Option C: 変数分類(実際のCFG predecessorsを使用) + let class = classifier.classify( + &var_name, + pinned_vars, + carrier_vars, + inspector, + exit_preds, // ← 実際のCFG predecessorsを使用! + ); + + if debug { + eprintln!("[Option C] var '{}': {:?} needs_exit_phi={}", + var_name, class, class.needs_exit_phi()); + let defining_blocks = inspector.get_defining_blocks(&var_name); + eprintln!("[Option C] defining_blocks: {:?}", defining_blocks); + } + + // Option C: Additional check - even Carrier/Pinned need definition check! + // Carrier/Pinned だからといって、全 exit preds で定義されているとは限らない + let is_in_all_preds = inspector.is_defined_in_all(&var_name, exit_preds); + + // exit PHI が不要な場合は skip + if !class.needs_exit_phi() || !is_in_all_preds { + if debug { + if !class.needs_exit_phi() { + eprintln!("[Option C] → SKIP exit PHI for '{}' (class={:?})", var_name, class); + } else { + eprintln!("[Option C] → SKIP exit PHI for '{}' (NOT in all preds, class={:?})", var_name, class); + } + } + continue; + } + + // exit PHI が必要な変数のみ入力を集約 + let mut inputs: Vec<(BasicBlockId, ValueId)> = Vec::new(); + + // Header fallthrough(header に存在する変数のみ) + if let Some(&val) = header_vals.get(&var_name) { + inputs.push((header_id, val)); + } + + // Break snapshots + for (bb, snap) in exit_snapshots { + if let Some(&val) = snap.get(&var_name) { + inputs.push((*bb, val)); + } + } + + if !inputs.is_empty() { + result.insert(var_name, inputs); + } + } + + Ok(result) + } + /// PHI最適化: 全て同じ値ならPHI不要と判定 /// /// ## 引数 diff --git a/src/mir/phi_core/loop_var_classifier.rs b/src/mir/phi_core/loop_var_classifier.rs new file mode 100644 index 00000000..34f6bcf3 --- /dev/null +++ b/src/mir/phi_core/loop_var_classifier.rs @@ -0,0 +1,474 @@ +/// LoopVarClassBox - Loop variable classifier +/// +/// # Purpose +/// +/// This box classifies loop variables into 4 categories: +/// - Pinned: Loop-crossing parameters (always need PHI) +/// - Carrier: Loop-modified variables (always need PHI) +/// - BodyLocalExit: Body-local but defined in ALL exit predecessors (need exit PHI) +/// - BodyLocalInternal: Body-local, NOT in all exit predecessors (NO exit PHI) +/// +/// # Responsibility +/// +/// - Classify variables based on their definition scope +/// - Use LocalScopeInspectorBox to check definition locations +/// - Provide classification for PHI generation decision +/// +/// # Design Philosophy (Box Theory) +/// +/// This box has ONE job: classify variables. +/// It doesn't generate PHI nodes or modify IR. +/// It's a pure decision box that other boxes can query. + +use super::local_scope_inspector::LocalScopeInspectorBox; +use crate::mir::BasicBlockId; + +/// Variable classification for loop PHI generation +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum LoopVarClass { + /// Loop-crossing parameter (always needs header/exit PHI) + /// + /// Example: + /// ``` + /// method process(limit) { // limit is Pinned + /// local i = 0 + /// loop(i < limit) { i = i + 1 } // limit doesn't change + /// } + /// ``` + Pinned, + + /// Loop-modified variable (always needs header/exit PHI) + /// + /// Example: + /// ``` + /// local i = 0 + /// loop(i < 10) { + /// i = i + 1 // i is Carrier (modified in loop) + /// } + /// ``` + Carrier, + + /// Body-local variable defined in ALL exit predecessors (needs exit PHI) + /// + /// Example: + /// ``` + /// loop(condition) { + /// local x = compute() // defined at loop entry + /// if x > 10 { break } // exit path 1: x exists + /// if x < 0 { break } // exit path 2: x exists + /// } + /// // x is BodyLocalExit → needs exit PHI + /// ``` + BodyLocalExit, + + /// Body-local variable NOT in all exit predecessors (NO exit PHI) + /// + /// Example (skip_whitespace bug): + /// ``` + /// loop(1 == 1) { + /// if i >= n { break } // exit path 1: ch doesn't exist + /// local ch = s.substring() // ch defined here + /// if ch == " " { ... } else { break } // exit path 2: ch exists + /// } + /// // ch is BodyLocalInternal → NO exit PHI (would cause pred mismatch!) + /// ``` + BodyLocalInternal, +} + +impl LoopVarClass { + /// Check if this classification requires exit PHI + pub fn needs_exit_phi(self) -> bool { + match self { + LoopVarClass::Pinned => true, + LoopVarClass::Carrier => true, + LoopVarClass::BodyLocalExit => true, + LoopVarClass::BodyLocalInternal => false, // ← Option C: Skip exit PHI! + } + } + + /// Check if this classification requires header PHI + pub fn needs_header_phi(self) -> bool { + match self { + LoopVarClass::Pinned => true, + LoopVarClass::Carrier => true, + LoopVarClass::BodyLocalExit => false, // Body-local: no header PHI + LoopVarClass::BodyLocalInternal => false, + } + } + + /// Get human-readable description + pub fn description(&self) -> &'static str { + match self { + LoopVarClass::Pinned => "Loop-crossing parameter", + LoopVarClass::Carrier => "Loop-modified variable", + LoopVarClass::BodyLocalExit => "Body-local (all exits)", + LoopVarClass::BodyLocalInternal => "Body-local (partial exits)", + } + } +} + +/// Loop variable classifier box +/// +/// # Usage +/// +/// ``` +/// let inspector = LocalScopeInspectorBox::new(); +/// // ... record definitions ... +/// +/// let classifier = LoopVarClassBox::new(); +/// let class = classifier.classify( +/// "ch", +/// &["limit", "n"], // pinned_vars +/// &["i"], // carrier_vars +/// &inspector, +/// &exit_preds, +/// ); +/// +/// if class.needs_exit_phi() { +/// // Generate exit PHI +/// } +/// ``` +#[derive(Debug, Clone, Default)] +pub struct LoopVarClassBox; + +impl LoopVarClassBox { + /// Create a new classifier + pub fn new() -> Self { + Self + } + + /// Classify a variable for PHI generation decision + /// + /// # Parameters + /// + /// - `var_name`: Variable to classify + /// - `pinned_vars`: Known loop-crossing parameters + /// - `carrier_vars`: Known loop-modified variables + /// - `inspector`: LocalScopeInspectorBox to check definition locations + /// - `exit_preds`: All exit predecessor blocks + /// + /// # Returns + /// + /// LoopVarClass indicating PHI generation requirements + /// + /// # Example + /// + /// ``` + /// // skip_whitespace scenario: + /// let class = classifier.classify( + /// "ch", + /// &[], // not pinned + /// &[], // not carrier + /// &inspector, // ch only in block 5 + /// &[BasicBlockId(2), BasicBlockId(5)], // exit preds + /// ); + /// + /// assert_eq!(class, LoopVarClass::BodyLocalInternal); + /// assert!(!class.needs_exit_phi()); // ← Skip exit PHI! + /// ``` + pub fn classify( + &self, + var_name: &str, + pinned_vars: &[String], + carrier_vars: &[String], + inspector: &LocalScopeInspectorBox, + exit_preds: &[BasicBlockId], + ) -> LoopVarClass { + // Priority 1: Check if it's a pinned variable + if pinned_vars.iter().any(|p| p == var_name) { + return LoopVarClass::Pinned; + } + + // Priority 2: Check if it's a carrier variable + if carrier_vars.iter().any(|c| c == var_name) { + return LoopVarClass::Carrier; + } + + // Priority 3: Check if it's a body-local variable + // Option C logic: Check if defined in ALL exit predecessors + if inspector.is_defined_in_all(var_name, exit_preds) { + LoopVarClass::BodyLocalExit + } else { + // ← Option C: Skip exit PHI for partial body-local variables! + LoopVarClass::BodyLocalInternal + } + } + + /// Batch classify multiple variables + /// + /// # Returns + /// + /// Vec of (var_name, classification) tuples + pub fn classify_all( + &self, + var_names: &[String], + pinned_vars: &[String], + carrier_vars: &[String], + inspector: &LocalScopeInspectorBox, + exit_preds: &[BasicBlockId], + ) -> Vec<(String, LoopVarClass)> { + var_names + .iter() + .map(|name| { + let class = self.classify(name, pinned_vars, carrier_vars, inspector, exit_preds); + (name.clone(), class) + }) + .collect() + } + + /// Filter variables that need exit PHI + /// + /// # Returns + /// + /// Vec of variable names that should get exit PHI + pub fn filter_exit_phi_candidates( + &self, + var_names: &[String], + pinned_vars: &[String], + carrier_vars: &[String], + inspector: &LocalScopeInspectorBox, + exit_preds: &[BasicBlockId], + ) -> Vec { + var_names + .iter() + .filter(|name| { + let class = self.classify(name, pinned_vars, carrier_vars, inspector, exit_preds); + class.needs_exit_phi() + }) + .cloned() + .collect() + } +} + +// ============================================================================ +// Unit Tests +// ============================================================================ + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_loop_var_class_needs_exit_phi() { + assert!(LoopVarClass::Pinned.needs_exit_phi()); + assert!(LoopVarClass::Carrier.needs_exit_phi()); + assert!(LoopVarClass::BodyLocalExit.needs_exit_phi()); + assert!(!LoopVarClass::BodyLocalInternal.needs_exit_phi()); // ← Option C! + } + + #[test] + fn test_loop_var_class_needs_header_phi() { + assert!(LoopVarClass::Pinned.needs_header_phi()); + assert!(LoopVarClass::Carrier.needs_header_phi()); + assert!(!LoopVarClass::BodyLocalExit.needs_header_phi()); + assert!(!LoopVarClass::BodyLocalInternal.needs_header_phi()); + } + + #[test] + fn test_classify_pinned_variable() { + let inspector = LocalScopeInspectorBox::new(); + let classifier = LoopVarClassBox::new(); + + let class = classifier.classify( + "limit", + &["limit".to_string()], // pinned + &[], + &inspector, + &[], + ); + + assert_eq!(class, LoopVarClass::Pinned); + assert!(class.needs_exit_phi()); + assert!(class.needs_header_phi()); + } + + #[test] + fn test_classify_carrier_variable() { + let inspector = LocalScopeInspectorBox::new(); + let classifier = LoopVarClassBox::new(); + + let class = classifier.classify( + "i", + &[], + &["i".to_string()], // carrier + &inspector, + &[], + ); + + assert_eq!(class, LoopVarClass::Carrier); + assert!(class.needs_exit_phi()); + assert!(class.needs_header_phi()); + } + + #[test] + fn test_classify_body_local_exit() { + let mut inspector = LocalScopeInspectorBox::new(); + let classifier = LoopVarClassBox::new(); + + // Variable "x" is defined in all exit predecessors + let block_2 = BasicBlockId::new(2); + let block_5 = BasicBlockId::new(5); + + inspector.record_definition("x", block_2); + inspector.record_definition("x", block_5); + + let exit_preds = vec![block_2, block_5]; + + let class = classifier.classify( + "x", + &[], + &[], + &inspector, + &exit_preds, + ); + + assert_eq!(class, LoopVarClass::BodyLocalExit); + assert!(class.needs_exit_phi()); // Should get exit PHI + assert!(!class.needs_header_phi()); + } + + #[test] + fn test_classify_body_local_internal() { + let mut inspector = LocalScopeInspectorBox::new(); + let classifier = LoopVarClassBox::new(); + + // Variable "ch" is only defined in block 5, not in block 2 + let block_2 = BasicBlockId::new(2); + let block_5 = BasicBlockId::new(5); + + inspector.record_definition("ch", block_5); // Only block 5! + + let exit_preds = vec![block_2, block_5]; + + let class = classifier.classify( + "ch", + &[], + &[], + &inspector, + &exit_preds, + ); + + assert_eq!(class, LoopVarClass::BodyLocalInternal); + assert!(!class.needs_exit_phi()); // ← Option C: Skip exit PHI! + assert!(!class.needs_header_phi()); + } + + /// Test the skip_whitespace PHI bug scenario + #[test] + fn test_skip_whitespace_scenario() { + let mut inspector = LocalScopeInspectorBox::new(); + let classifier = LoopVarClassBox::new(); + + // Simulate skip_whitespace loop: + // - Variables i, n, s are pinned/carrier (defined everywhere) + // - Variable ch is only defined in block 5 (break path 2) + + let block_2 = BasicBlockId::new(2); // header / break path 1 + let block_5 = BasicBlockId::new(5); // break path 2 + let block_7 = BasicBlockId::new(7); // latch (hypothetical) + + // i, n, s are in all blocks + for var in &["i", "n", "s"] { + inspector.record_definition(var, block_2); + inspector.record_definition(var, block_5); + inspector.record_definition(var, block_7); + } + + // ch is only in block 5 + inspector.record_definition("ch", block_5); + + let exit_preds = vec![block_2, block_5, block_7]; + + // Classify all variables + let pinned = vec!["n".to_string(), "s".to_string()]; + let carrier = vec!["i".to_string()]; + + let i_class = classifier.classify("i", &pinned, &carrier, &inspector, &exit_preds); + let n_class = classifier.classify("n", &pinned, &carrier, &inspector, &exit_preds); + let s_class = classifier.classify("s", &pinned, &carrier, &inspector, &exit_preds); + let ch_class = classifier.classify("ch", &pinned, &carrier, &inspector, &exit_preds); + + // i, n, s should need exit PHI + assert_eq!(i_class, LoopVarClass::Carrier); + assert_eq!(n_class, LoopVarClass::Pinned); + assert_eq!(s_class, LoopVarClass::Pinned); + + assert!(i_class.needs_exit_phi()); + assert!(n_class.needs_exit_phi()); + assert!(s_class.needs_exit_phi()); + + // ch should NOT need exit PHI (Option C: prevents PHI pred mismatch!) + assert_eq!(ch_class, LoopVarClass::BodyLocalInternal); + assert!(!ch_class.needs_exit_phi()); // ← This fixes the bug! + } + + #[test] + fn test_classify_all() { + let mut inspector = LocalScopeInspectorBox::new(); + let classifier = LoopVarClassBox::new(); + + let block_2 = BasicBlockId::new(2); + let block_5 = BasicBlockId::new(5); + + // Setup: i, n in all blocks; ch only in block 5 + for var in &["i", "n"] { + inspector.record_definition(var, block_2); + inspector.record_definition(var, block_5); + } + inspector.record_definition("ch", block_5); + + let vars = vec!["i".to_string(), "n".to_string(), "ch".to_string()]; + let pinned = vec!["n".to_string()]; + let carrier = vec!["i".to_string()]; + let exit_preds = vec![block_2, block_5]; + + let results = classifier.classify_all(&vars, &pinned, &carrier, &inspector, &exit_preds); + + assert_eq!(results.len(), 3); + assert_eq!(results[0], ("i".to_string(), LoopVarClass::Carrier)); + assert_eq!(results[1], ("n".to_string(), LoopVarClass::Pinned)); + assert_eq!(results[2], ("ch".to_string(), LoopVarClass::BodyLocalInternal)); + } + + #[test] + fn test_filter_exit_phi_candidates() { + let mut inspector = LocalScopeInspectorBox::new(); + let classifier = LoopVarClassBox::new(); + + let block_2 = BasicBlockId::new(2); + let block_5 = BasicBlockId::new(5); + + // Setup: i, n in all blocks; ch only in block 5 + for var in &["i", "n"] { + inspector.record_definition(var, block_2); + inspector.record_definition(var, block_5); + } + inspector.record_definition("ch", block_5); + + let vars = vec!["i".to_string(), "n".to_string(), "ch".to_string()]; + let pinned = vec!["n".to_string()]; + let carrier = vec!["i".to_string()]; + let exit_preds = vec![block_2, block_5]; + + let candidates = classifier.filter_exit_phi_candidates( + &vars, + &pinned, + &carrier, + &inspector, + &exit_preds, + ); + + assert_eq!(candidates.len(), 2); + assert!(candidates.contains(&"i".to_string())); + assert!(candidates.contains(&"n".to_string())); + assert!(!candidates.contains(&"ch".to_string())); // ← Option C: ch is filtered out! + } + + #[test] + fn test_description() { + assert_eq!(LoopVarClass::Pinned.description(), "Loop-crossing parameter"); + assert_eq!(LoopVarClass::Carrier.description(), "Loop-modified variable"); + assert_eq!(LoopVarClass::BodyLocalExit.description(), "Body-local (all exits)"); + assert_eq!(LoopVarClass::BodyLocalInternal.description(), "Body-local (partial exits)"); + } +} diff --git a/src/mir/phi_core/loopform_builder.rs b/src/mir/phi_core/loopform_builder.rs index 7da0710f..40489cc0 100644 --- a/src/mir/phi_core/loopform_builder.rs +++ b/src/mir/phi_core/loopform_builder.rs @@ -378,12 +378,21 @@ impl LoopFormBuilder { /// # Parameters /// - `branch_source_block`: The ACTUAL block that emitted the branch to exit /// (might differ from header_id if condition evaluation created new blocks) + /// Option C: Build exit PHIs with variable classification + /// + /// ## 引数 + /// + /// - `inspector`: LocalScopeInspectorBox(変数定義位置追跡) + /// - 各ブロックでどの変数が定義されているか追跡 + /// - BodyLocalInternal 変数(全exit predsで定義されていない)を検出 + /// - これらの変数は exit PHI を生成しない(PHI pred mismatch防止) pub fn build_exit_phis( &self, ops: &mut O, exit_id: BasicBlockId, branch_source_block: BasicBlockId, exit_snapshots: &[(BasicBlockId, HashMap)], + inspector: &super::local_scope_inspector::LocalScopeInspectorBox, ) -> Result<(), String> { ops.set_current_block(exit_id)?; @@ -463,12 +472,22 @@ impl LoopFormBuilder { filtered_snapshots.push((*block_id, snapshot.clone())); } - // 3. Phase 25.2: LoopSnapshotMergeBox::merge_exit() を呼び出し - let all_vars = LoopSnapshotMergeBox::merge_exit( + // 3. Option C: merge_exit_with_classification() でPHI pred mismatch防止 + // pinned/carrier名リストを準備 + let pinned_names: Vec = self.pinned.iter().map(|p| p.name.clone()).collect(); + let carrier_names: Vec = self.carriers.iter().map(|c| c.name.clone()).collect(); + + // exit_preds を Vec に変換 + let exit_preds_vec: Vec = exit_preds.iter().copied().collect(); + + let all_vars = LoopSnapshotMergeBox::merge_exit_with_classification( branch_source_block, &header_vals, &filtered_snapshots, - &body_local_names, + &exit_preds_vec, // ← 実際のCFG predecessorsを渡す + &pinned_names, + &carrier_names, + inspector, )?; // 4. PHI 生成(optimize_same_value と sanitize_inputs を使用) @@ -618,8 +637,26 @@ pub fn build_exit_phis_for_control( ); } - // Delegate to existing implementation - no behavioral changes - loopform.build_exit_phis(ops, exit_id, branch_source_block, exit_snapshots) + // Option C: Build LocalScopeInspectorBox from exit_snapshots + use super::local_scope_inspector::LocalScopeInspectorBox; + let mut inspector = LocalScopeInspectorBox::new(); + + // Record header variable definitions (pinned + carriers) + let header_id = loopform.header_id; + for pinned in &loopform.pinned { + inspector.record_definition(&pinned.name, header_id); + } + for carrier in &loopform.carriers { + inspector.record_definition(&carrier.name, header_id); + } + + // Record all variable definitions from exit snapshots + for (block_id, snapshot) in exit_snapshots { + inspector.record_snapshot(*block_id, snapshot); + } + + // Delegate to Option C implementation + loopform.build_exit_phis(ops, exit_id, branch_source_block, exit_snapshots, &inspector) } #[cfg(test)] diff --git a/src/mir/phi_core/mod.rs b/src/mir/phi_core/mod.rs index 0ddc871b..b4ff45fc 100644 --- a/src/mir/phi_core/mod.rs +++ b/src/mir/phi_core/mod.rs @@ -14,6 +14,10 @@ pub mod loop_phi; pub mod loop_snapshot_merge; pub mod loopform_builder; +// Option C PHI bug fix: Box-based design +pub mod local_scope_inspector; +pub mod loop_var_classifier; + // Public surface for callers that want a stable path: // Phase 1: No re-exports to avoid touching private builder internals. // Callers should continue using existing paths. Future phases may expose diff --git a/src/runner/json_v0_bridge/lowering/loop_.rs b/src/runner/json_v0_bridge/lowering/loop_.rs index 5fd43941..7f919458 100644 --- a/src/runner/json_v0_bridge/lowering/loop_.rs +++ b/src/runner/json_v0_bridge/lowering/loop_.rs @@ -327,7 +327,24 @@ pub(super) fn lower_loop_stmt( loopform.seal_phis(&mut ops, latch_bb, &canonical_continue_snaps)?; // 8) exit PHI(header fallthrough + break スナップショット) - loopform.build_exit_phis(&mut ops, exit_bb, cend, &exit_snaps)?; + // Option C: Build LocalScopeInspectorBox from exit snapshots + use crate::mir::phi_core::local_scope_inspector::LocalScopeInspectorBox; + let mut inspector = LocalScopeInspectorBox::new(); + + // Record header variable definitions (pinned + carriers) + for pinned in &loopform.pinned { + inspector.record_definition(&pinned.name, loopform.header_id); + } + for carrier in &loopform.carriers { + inspector.record_definition(&carrier.name, loopform.header_id); + } + + // Record all variable definitions from exit snapshots + for (block_id, snapshot) in &exit_snaps { + inspector.record_snapshot(*block_id, snapshot); + } + + loopform.build_exit_phis(&mut ops, exit_bb, cend, &exit_snaps, &inspector)?; Ok(exit_bb) }