diff --git a/docs/development/current/main/phase202-a-pattern1-joinvaluespace.md b/docs/development/current/main/phase202-a-pattern1-joinvaluespace.md new file mode 100644 index 00000000..13073986 --- /dev/null +++ b/docs/development/current/main/phase202-a-pattern1-joinvaluespace.md @@ -0,0 +1,246 @@ +# Phase 202-A: Pattern 1 JoinValueSpace Migration + +**Status**: ✅ Complete +**Date**: 2025-12-09 +**Commit**: `6e778948` + +## Overview + +Migrated Pattern 1 (Simple While Loop) from manual `value_counter` to unified `JoinValueSpace` allocation system, following the same pattern established in Phase 201 for Pattern 2. + +## Motivation + +Pattern 2 (Phase 201) revealed that using separate ValueId allocation mechanisms (manual counters) can cause collisions between: +- **Param region (100-999)**: Used by ConditionEnv, CarrierInfo for loop parameters +- **Local region (1000+)**: Used for temporary values (Const, BinOp, etc.) + +Pattern 1 needed the same unification to: +1. **Consistency**: All patterns should use the same allocation mechanism +2. **Future-proofing**: Pattern 1 may need Param region in future enhancements +3. **Safety**: Prevent potential ValueId collision bugs + +## Changes + +### 1. `simple_while_minimal.rs` + +**Before (Phase 188)**: +```rust +pub(crate) fn lower_simple_while_minimal(_scope: LoopScopeShape) -> Option { + let mut value_counter = 0u32; + let mut alloc_value = || { + let id = ValueId(value_counter); + value_counter += 1; + id + }; + // ... +} +``` + +**After (Phase 202-A)**: +```rust +pub(crate) fn lower_simple_while_minimal( + _scope: LoopScopeShape, + join_value_space: &mut JoinValueSpace, +) -> Option { + let mut alloc_value = || join_value_space.alloc_local(); + // ... +} +``` + +**Key Points**: +- Added `join_value_space: &mut JoinValueSpace` parameter +- Removed manual `value_counter` allocation +- Uses Local region (1000+) exclusively (no Param region needed for Pattern 1) +- Added Phase 202-A documentation comments + +### 2. `pattern1_minimal.rs` (Caller) + +**Before**: +```rust +let join_module = match lower_simple_while_minimal(ctx.loop_scope) { + Some(module) => module, + None => return Ok(None), +}; +``` + +**After**: +```rust +// Phase 202-A: Create JoinValueSpace for unified ValueId allocation +use crate::mir::join_ir::lowering::join_value_space::JoinValueSpace; +let mut join_value_space = JoinValueSpace::new(); + +let join_module = match lower_simple_while_minimal(ctx.loop_scope, &mut join_value_space) { + Some(module) => module, + None => return Ok(None), +}; +``` + +**Key Points**: +- Create `JoinValueSpace` before calling lowerer +- Pass mutable reference to lowerer +- Pattern 1 uses Local region only (no ConditionEnv, no Param allocation) + +### 3. `loop_view_builder.rs` + +**Before**: +```rust +if let Some(result) = super::simple_while_minimal::lower_simple_while_minimal(scope.clone()) { + // ... +} +``` + +**After**: +```rust +// Phase 202-A: Create JoinValueSpace for Pattern 1 +use super::join_value_space::JoinValueSpace; +let mut join_value_space = JoinValueSpace::new(); + +if let Some(result) = super::simple_while_minimal::lower_simple_while_minimal( + scope.clone(), + &mut join_value_space, +) { + // ... +} +``` + +**Key Points**: +- Create `JoinValueSpace` in `try_pattern1()` helper +- Pass to lowerer for unified allocation + +## Technical Details + +### ValueId Allocation Strategy (Pattern 1) + +Pattern 1 is simpler than Pattern 2: + +| Region | Range | Usage | Pattern 1 | +|--------|-------|-------|-----------| +| PHI Reserved | 0-99 | Loop header PHI dst | ❌ Not used | +| Param | 100-999 | ConditionEnv, CarrierInfo | ❌ Not used | +| Local | 1000+ | Const, BinOp, temps | ✅ Used | + +**Why Pattern 1 doesn't need Param region**: +- No break conditions → No ConditionEnv needed +- Simple while loop → No complex carrier analysis +- All allocations are for temporary values (Const, Compare, UnaryOp, etc.) + +### Example Allocation Sequence + +For `loop_min_while.hako`: +```nyash +loop(i < 3) { + print(i) + i = i + 1 +} +``` + +**Allocated ValueIds**: +```rust +i_init = alloc_local() // ValueId(1000) +loop_result = alloc_local() // ValueId(1001) +const_0_main = alloc_local() // ValueId(1002) +i_param = alloc_local() // ValueId(1003) +const_3 = alloc_local() // ValueId(1004) +cmp_lt = alloc_local() // ValueId(1005) +exit_cond = alloc_local() // ValueId(1006) +const_1 = alloc_local() // ValueId(1007) +i_next = alloc_local() // ValueId(1008) +const_0_exit = alloc_local() // ValueId(1009) +``` + +All in Local region (1000+), no collision possible. + +## Testing + +### Build Status +```bash +$ cargo build --release --lib +✅ Success (0 errors, 4 warnings) +``` + +### Unit Tests +```bash +$ cargo test --release --lib pattern +✅ 119 passed; 0 failed; 18 ignored +``` + +### Full Test Suite +```bash +$ cargo test --release --lib +✅ 821 passed; 0 failed; 64 ignored +``` + +### E2E Tests +```bash +$ ./target/release/hakorune apps/tests/loop_min_while.hako +✅ Output: "0 1 2" (correct) + +$ ./target/release/hakorune apps/tests/minimal_ssa_bug_loop.hako +✅ RC: 0 (success) +``` + +## Benefits + +1. **Consistency**: All patterns (1, 2, 3, 4) use JoinValueSpace +2. **Safety**: Guaranteed no ValueId collisions between regions +3. **Maintainability**: Single allocation mechanism to understand +4. **Future-proof**: Easy to add Param region if Pattern 1 needs ConditionEnv later +5. **Debuggability**: Clear region boundaries make debugging easier + +## Comparison with Pattern 2 + +| Aspect | Pattern 1 | Pattern 2 | +|--------|-----------|-----------| +| ConditionEnv | ❌ No | ✅ Yes | +| Param region | ❌ Not used | ✅ Used (100+) | +| Local region | ✅ Used (1000+) | ✅ Used (1000+) | +| CarrierInfo | ❌ No | ✅ Yes | +| Break conditions | ❌ No | ✅ Yes | + +**Key Difference**: Pattern 1 is simpler - it only needs Local region because it has no complex condition analysis. + +## Next Steps + +### Phase 202-B: Pattern 3 Migration (Planned) +- Migrate Pattern 3 (If-Else PHI) to JoinValueSpace +- Similar to Pattern 2 (needs both Param and Local regions) +- Will use ConditionEnv for PHI value resolution + +### Phase 202-C: Pattern 4 Migration (Planned) +- Migrate Pattern 4 (Continue) to JoinValueSpace +- Similar complexity to Pattern 3 +- Needs Param region for continue condition analysis + +## References + +- **Phase 201**: Pattern 2 JoinValueSpace migration (reference implementation) +- **JoinValueSpace Design**: `src/mir/join_ir/lowering/join_value_space.rs` +- **Pattern 1 Implementation**: `src/mir/join_ir/lowering/simple_while_minimal.rs` +- **Pattern 2 Reference**: `src/mir/join_ir/lowering/loop_with_break_minimal.rs` + +## Commit Message + +``` +feat(joinir): Phase 202-A Pattern 1 uses JoinValueSpace + +Migrated Pattern 1 (Simple While) to use JoinValueSpace for unified +ValueId allocation, following the same pattern as Pattern 2 (Phase 201). + +Changes: +- simple_while_minimal.rs: Added join_value_space parameter, replaced + value_counter with join_value_space.alloc_local() +- pattern1_minimal.rs: Create JoinValueSpace before calling lowerer +- loop_view_builder.rs: Create JoinValueSpace in try_pattern1() + +Pattern 1 uses Local region (1000+) only, since it doesn't need +ConditionEnv (no Param region allocation required). + +Tested: +- cargo build --release --lib: Success (0 errors, 4 warnings) +- cargo test --release --lib pattern: 119 passed +- E2E test apps/tests/loop_min_while.hako: Outputs "0 1 2" correctly + +🤖 Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude Sonnet 4.5 +``` diff --git a/src/mir/builder/control_flow/joinir/patterns/pattern3_with_if_phi.rs b/src/mir/builder/control_flow/joinir/patterns/pattern3_with_if_phi.rs index 63167cf6..0ff8c634 100644 --- a/src/mir/builder/control_flow/joinir/patterns/pattern3_with_if_phi.rs +++ b/src/mir/builder/control_flow/joinir/patterns/pattern3_with_if_phi.rs @@ -101,8 +101,12 @@ impl MirBuilder { // Phase 195: Use unified trace trace::trace().varmap("pattern3_start", &self.variable_map); + // Phase 202-B: Create JoinValueSpace for unified ValueId allocation + use crate::mir::join_ir::lowering::join_value_space::JoinValueSpace; + let mut join_value_space = JoinValueSpace::new(); + // Call Pattern 3 lowerer with preprocessed scope - let join_module = match lower_loop_with_if_phi_pattern(ctx.loop_scope) { + let join_module = match lower_loop_with_if_phi_pattern(ctx.loop_scope, &mut join_value_space) { Some(module) => module, None => { // Phase 195: Use unified trace diff --git a/src/mir/join_ir/lowering/loop_patterns/with_if_phi.rs b/src/mir/join_ir/lowering/loop_patterns/with_if_phi.rs index 91fa0a12..bd2c2392 100644 --- a/src/mir/join_ir/lowering/loop_patterns/with_if_phi.rs +++ b/src/mir/join_ir/lowering/loop_patterns/with_if_phi.rs @@ -85,6 +85,7 @@ pub fn lower_loop_with_conditional_phi_to_joinir( ) -> Option { // Phase 188-Impl-3: Delegate to minimal lowerer // TODO: Extract LoopScopeShape from loop_form for generic implementation + use crate::mir::join_ir::lowering::join_value_space::JoinValueSpace; use crate::mir::join_ir::lowering::loop_scope_shape::LoopScopeShape; use crate::mir::join_ir::lowering::loop_with_if_phi_minimal::lower_loop_with_if_phi_pattern; use crate::mir::BasicBlockId; @@ -105,8 +106,11 @@ pub fn lower_loop_with_conditional_phi_to_joinir( variable_definitions: BTreeMap::new(), }; + // Phase 202-B: Create JoinValueSpace for unified ValueId allocation + let mut join_value_space = JoinValueSpace::new(); + // Generate JoinIR module - let _join_module = lower_loop_with_if_phi_pattern(placeholder_scope)?; + let _join_module = lower_loop_with_if_phi_pattern(placeholder_scope, &mut join_value_space)?; // Phase 188-Impl-3: Pattern 3 is now integrated via the router // This function delegates to loop_with_if_phi_minimal which generates JoinModule diff --git a/src/mir/join_ir/lowering/loop_with_if_phi_minimal.rs b/src/mir/join_ir/lowering/loop_with_if_phi_minimal.rs index 0719fec3..bfa1ee06 100644 --- a/src/mir/join_ir/lowering/loop_with_if_phi_minimal.rs +++ b/src/mir/join_ir/lowering/loop_with_if_phi_minimal.rs @@ -67,6 +67,7 @@ //! //! Following the "80/20 rule" from CLAUDE.md - get it working first, generalize later. +use crate::mir::join_ir::lowering::join_value_space::JoinValueSpace; use crate::mir::join_ir::lowering::loop_scope_shape::LoopScopeShape; use crate::mir::join_ir::{ BinOpKind, CompareOp, ConstValue, JoinFuncId, JoinFunction, JoinInst, JoinModule, @@ -97,6 +98,7 @@ use crate::mir::ValueId; /// # Arguments /// /// * `_scope` - LoopScopeShape (reserved for future generic implementation) +/// * `join_value_space` - Phase 202-B: Unified JoinIR ValueId allocator (Local region: 1000+) /// /// # Returns /// @@ -109,15 +111,13 @@ use crate::mir::ValueId; /// - **Input slots**: ValueId(0), ValueId(1) in main function (i_init, sum_init) /// - **Output slot**: k_exit returns the final sum value /// - **Caller responsibility**: Create JoinInlineBoundary to map ValueIds -pub(crate) fn lower_loop_with_if_phi_pattern(_scope: LoopScopeShape) -> Option { - // Phase 188-Impl-3: Use local ValueId allocator (sequential from 0) - // JoinIR has NO knowledge of host ValueIds - boundary handled separately - let mut value_counter = 0u32; - let mut alloc_value = || { - let id = ValueId(value_counter); - value_counter += 1; - id - }; +pub(crate) fn lower_loop_with_if_phi_pattern( + _scope: LoopScopeShape, + join_value_space: &mut JoinValueSpace, +) -> Option { + // Phase 202-B: Use JoinValueSpace for unified ValueId allocation + // - Local region (1000+) ensures no collision with Param region (100-999) + let mut alloc_value = || join_value_space.alloc_local(); let mut join_module = JoinModule::new();