401 lines
12 KiB
Markdown
401 lines
12 KiB
Markdown
|
|
# Phase 243-EX: JoinIR Refactoring Investigation - Complete
|
||
|
|
|
||
|
|
**Investigation Date**: 2025-12-11
|
||
|
|
**Status**: ✅ Complete - Ready for Phase 244 Implementation
|
||
|
|
**Test Status**: ✅ 909 tests PASS (maintained throughout investigation)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Document Index
|
||
|
|
|
||
|
|
This phase produced 3 comprehensive documents analyzing JoinIR lowering refactoring opportunities:
|
||
|
|
|
||
|
|
### 📋 1. Quick Summary (Start Here!)
|
||
|
|
**File**: [phase243-ex-summary.md](phase243-ex-summary.md)
|
||
|
|
|
||
|
|
**Contents**:
|
||
|
|
- TL;DR (1 page)
|
||
|
|
- Quick stats (74 files, 23K lines)
|
||
|
|
- Top 3 opportunities with priorities
|
||
|
|
- Recommended roadmap (Phases 244-248)
|
||
|
|
- Success metrics
|
||
|
|
|
||
|
|
**Best For**: Decision-makers, quick reference
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 📊 2. Full Analysis (Detailed Report)
|
||
|
|
**File**: [phase243-ex-refactoring-opportunities.md](phase243-ex-refactoring-opportunities.md)
|
||
|
|
|
||
|
|
**Contents** (12K words):
|
||
|
|
1. Module structure visualization (74 files analyzed)
|
||
|
|
2. Common code & duplication analysis
|
||
|
|
3. Boxification candidates (5 major opportunities)
|
||
|
|
4. Module reorganization proposal (52 → 7 directories)
|
||
|
|
5. Dependency graph analysis
|
||
|
|
6. Implementation sketches (top 3 priorities)
|
||
|
|
7. Risk assessment
|
||
|
|
8. Priority scorecard
|
||
|
|
9. Recommended roadmap (Phases 244-248)
|
||
|
|
10. Open questions & future work
|
||
|
|
|
||
|
|
**Best For**: Implementation planning, detailed design
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 🔗 3. Dependency Analysis (Visual Graphs)
|
||
|
|
**File**: [phase243-ex-dependency-graph.md](phase243-ex-dependency-graph.md)
|
||
|
|
|
||
|
|
**Contents** (8K words):
|
||
|
|
1. High-level architecture diagram
|
||
|
|
2. Core module dependencies (Condition, Carrier, Expression)
|
||
|
|
3. Pattern implementation dependencies (Pattern 1-4)
|
||
|
|
4. Shared infrastructure dependencies
|
||
|
|
5. Cross-cutting concerns (Pattern detection, Method call lowering)
|
||
|
|
6. Dependency metrics (depth, breadth, circular check)
|
||
|
|
7. Proposed boxification dependencies
|
||
|
|
8. Impact analysis by phase
|
||
|
|
|
||
|
|
**Best For**: Understanding module relationships, impact analysis
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Investigation Findings
|
||
|
|
|
||
|
|
### Scale
|
||
|
|
- **74 files** in JoinIR lowering
|
||
|
|
- **23,183 total lines**
|
||
|
|
- **52 files in root** (too flat!)
|
||
|
|
- **15 large files** (>500 lines, 41% of code)
|
||
|
|
|
||
|
|
### Key Issues Identified
|
||
|
|
1. **Condition Logic Fragmentation**: 19 files touch condition lowering (1,639 lines)
|
||
|
|
2. **Carrier Logic Spread**: 7 files manage carriers (2,359 lines)
|
||
|
|
3. **Flat Module Structure**: 52 root files → hard to navigate
|
||
|
|
4. **Pattern Detection Split**: 4 files handle pattern detection
|
||
|
|
|
||
|
|
### Opportunities Found (5 Major)
|
||
|
|
|
||
|
|
| Candidate | Priority | Impact | Effort | Phase |
|
||
|
|
|-----------|----------|--------|--------|-------|
|
||
|
|
| **ConditionLoweringBox** | ⭐⭐⭐ | High (19 files) | Medium | 244 |
|
||
|
|
| **CarrierManagerBox** | ⭐⭐ | Medium (7 files) | Low | 245 |
|
||
|
|
| **Module Reorganization** | ⭐⭐ | Medium (all files) | Large | 246 |
|
||
|
|
| **PatternDetectorBox** | ⭐⭐ | Medium (4 files) | Medium | 247 |
|
||
|
|
| **Legacy Cleanup** | ⭐ | Low | Low | 248 |
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Recommended Implementation Sequence
|
||
|
|
|
||
|
|
### Phase 244: ConditionLoweringBox (⭐⭐⭐ Highest Priority)
|
||
|
|
**Goal**: Unify 19 files touching condition logic into single API
|
||
|
|
|
||
|
|
**Tasks**:
|
||
|
|
1. Create `core/condition_lowering/` directory
|
||
|
|
2. Define `ConditionLoweringBox` trait + dispatcher
|
||
|
|
3. Implement SimpleConditionLowerer + ComplexConditionLowerer
|
||
|
|
4. Migrate 19 callers to new API
|
||
|
|
5. Add backward compatibility shims
|
||
|
|
6. Run all 909 tests (expect 100% pass)
|
||
|
|
|
||
|
|
**Estimated Effort**: 1-2 days
|
||
|
|
**Risk**: Medium (19 files affected)
|
||
|
|
**Value**: High (single API, eliminates duplication)
|
||
|
|
|
||
|
|
**Files to Consolidate** (1,639 lines total):
|
||
|
|
- `condition_lowerer.rs` (537 lines)
|
||
|
|
- `condition_to_joinir.rs` (154 lines)
|
||
|
|
- `condition_env.rs` (237 lines)
|
||
|
|
- `condition_pattern.rs` (527 lines)
|
||
|
|
- `condition_var_extractor.rs` (184 lines)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### Phase 245: CarrierManagerBox (⭐⭐)
|
||
|
|
**Goal**: Extend Phase 228 infrastructure with unified lifecycle management
|
||
|
|
|
||
|
|
**Tasks**:
|
||
|
|
1. Create `CarrierManagerBox` struct (wraps CarrierInfo)
|
||
|
|
2. Move `init_carriers()` from 3 pattern implementations
|
||
|
|
3. Move `generate_exit_bindings()` from inline_boundary_builder
|
||
|
|
4. Add `carriers_for_phi()` convenience method
|
||
|
|
5. Update Pattern 2-4 to use manager
|
||
|
|
|
||
|
|
**Estimated Effort**: 0.5-1 day
|
||
|
|
**Risk**: Low (extends existing API)
|
||
|
|
**Value**: Medium (consolidates 3 modules)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### Phase 246: Module Reorganization (⭐⭐)
|
||
|
|
**Goal**: Reorganize 52 root files into 7 hierarchical directories
|
||
|
|
|
||
|
|
**Structure Change**:
|
||
|
|
```
|
||
|
|
Before: 52 files in root (flat)
|
||
|
|
After: 7 directories (hierarchical)
|
||
|
|
├── core/ (condition, carrier, exit)
|
||
|
|
├── infrastructure/ (expr, scope, boundary)
|
||
|
|
├── patterns/ (detection, loop, if)
|
||
|
|
├── specialized/ (function-specific)
|
||
|
|
└── generic_case_a/ (generic)
|
||
|
|
```
|
||
|
|
|
||
|
|
**Estimated Effort**: 1-2 days
|
||
|
|
**Risk**: Low (pure reorganization)
|
||
|
|
**Value**: Medium (86% reduction in root files)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### Phase 247: PatternDetectorBox (⭐⭐)
|
||
|
|
**Goal**: Consolidate pattern detection into single API
|
||
|
|
|
||
|
|
**Tasks**:
|
||
|
|
1. Create `patterns/detection/` directory
|
||
|
|
2. Define `PatternDetectorBox` trait
|
||
|
|
3. Move if/loop/update detection logic
|
||
|
|
4. Create unified dispatcher
|
||
|
|
5. Update routers (loop_pattern_router, if_lowering_router)
|
||
|
|
|
||
|
|
**Estimated Effort**: 1 day
|
||
|
|
**Risk**: Medium (3 modules affected)
|
||
|
|
**Value**: Medium (unified API)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### Phase 248: Legacy Cleanup (⭐)
|
||
|
|
**Goal**: Remove backward compatibility shims and unused code
|
||
|
|
|
||
|
|
**Tasks**:
|
||
|
|
1. Remove bool_expr_lowerer.rs (446 lines, unused)
|
||
|
|
2. Remove backward compat shims from Phase 244-247
|
||
|
|
3. Address or track 9 TODO items
|
||
|
|
4. Update documentation
|
||
|
|
|
||
|
|
**Estimated Effort**: 0.5 day
|
||
|
|
**Risk**: Low
|
||
|
|
**Value**: High (clean codebase)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Success Criteria
|
||
|
|
|
||
|
|
### Overall Goals
|
||
|
|
- ✅ All 909 tests passing throughout (no regressions)
|
||
|
|
- ✅ Improved code organization (52 → 7 root files)
|
||
|
|
- ✅ Unified APIs (3 new boxes: Condition, Carrier, Pattern)
|
||
|
|
- ✅ Reduced duplication (consolidate 1,639 lines)
|
||
|
|
- ✅ Better maintainability (single responsibility per module)
|
||
|
|
|
||
|
|
### Phase-by-Phase Metrics
|
||
|
|
|
||
|
|
| Phase | Files Created | Files Modified | Tests Expected | Risk Level |
|
||
|
|
|-------|--------------|----------------|----------------|------------|
|
||
|
|
| 244 | 5 | 19 | 909 PASS | Medium |
|
||
|
|
| 245 | 1 | 7 | 909 PASS | Low |
|
||
|
|
| 246 | 7 dirs | 74 | 909 PASS | Low |
|
||
|
|
| 247 | 4 | 10 | 909 PASS | Medium |
|
||
|
|
| 248 | 0 | 20 | 909 PASS | Low |
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Infrastructure Ready (Built During Phase 226-242)
|
||
|
|
|
||
|
|
The following infrastructure is already in place and ready to build on:
|
||
|
|
|
||
|
|
### ✅ Phase 227: CarrierRole
|
||
|
|
- `enum CarrierRole { LoopState, ConditionOnly }`
|
||
|
|
- Distinguishes carriers that need exit PHI vs condition-only
|
||
|
|
|
||
|
|
### ✅ Phase 228: CarrierInit
|
||
|
|
- `enum CarrierInit { FromHost, BoolConst(bool) }`
|
||
|
|
- Initialization policy for header PHI
|
||
|
|
|
||
|
|
### ✅ Phase 231: ExprLowerer + ScopeManager
|
||
|
|
- `trait ScopeManager { lookup(&str) -> Option<ValueId> }`
|
||
|
|
- `struct ExprLowerer<S: ScopeManager>`
|
||
|
|
- Unified expression lowering with scope management
|
||
|
|
|
||
|
|
### ✅ Phase 33-10: ExitLineReconnector
|
||
|
|
- ExitLineReconnector, ExitMetaCollector, ExitLineOrchestrator
|
||
|
|
- Already boxified! (no work needed)
|
||
|
|
|
||
|
|
### ✅ Phase 240-EX: ExprLowerer Integration
|
||
|
|
- ExprLowerer successfully integrated into Pattern 3 if-sum mode
|
||
|
|
- Pilot implementation validated
|
||
|
|
|
||
|
|
### ✅ Phase 242-EX-A: Complex Condition Support
|
||
|
|
- BinaryOp in LHS (e.g., `i % 2 == 1`)
|
||
|
|
- Removed hardcoded legacy code
|
||
|
|
- 909 tests PASS
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Risk Mitigation Strategy
|
||
|
|
|
||
|
|
### 1. Test-Driven Approach
|
||
|
|
- Run all 909 tests after each step
|
||
|
|
- No logic changes in initial refactoring (pure reorganization)
|
||
|
|
- Fail-Fast if tests break
|
||
|
|
|
||
|
|
### 2. Backward Compatibility
|
||
|
|
- Add re-export shims in old file locations
|
||
|
|
- Gradual migration (update callers one by one)
|
||
|
|
- Keep old API working during migration
|
||
|
|
|
||
|
|
### 3. Incremental Rollout
|
||
|
|
- One phase at a time (244 → 245 → 246 → 247 → 248)
|
||
|
|
- Each phase is independently valuable
|
||
|
|
- Can pause/adjust between phases
|
||
|
|
|
||
|
|
### 4. Clear Rollback Plan
|
||
|
|
- All changes are additive initially (new files alongside old)
|
||
|
|
- Git commits per phase (easy to revert)
|
||
|
|
- Backward compat shims stay until Phase 248
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Dependency Health Check
|
||
|
|
|
||
|
|
### ✅ No Circular Dependencies
|
||
|
|
All dependencies flow in one direction:
|
||
|
|
```
|
||
|
|
Pattern Implementations
|
||
|
|
↓
|
||
|
|
Condition/Carrier/Expression Lowering
|
||
|
|
↓
|
||
|
|
Infrastructure (ValueSpace, Boundary, Env)
|
||
|
|
↓
|
||
|
|
Core Structures (ValueId, JoinInst)
|
||
|
|
```
|
||
|
|
|
||
|
|
### ✅ Clean Dependency Flow
|
||
|
|
Longest chain (6 levels):
|
||
|
|
```
|
||
|
|
Pattern 3 → ExprLowerer → ScopeManager →
|
||
|
|
ConditionEnv → CarrierInfo → InlineBoundary
|
||
|
|
```
|
||
|
|
|
||
|
|
### ✅ Well-Defined Boundaries
|
||
|
|
- JoinIR Frontend: AST → JoinModule
|
||
|
|
- Join→MIR Bridge: JoinModule → MIR
|
||
|
|
- Boundary: InlineBoundary (host ↔ JoinIR)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Key Design Principles (Box-First)
|
||
|
|
|
||
|
|
1. **Box-First Architecture**
|
||
|
|
- Condition lowering → ConditionLoweringBox
|
||
|
|
- Carrier management → CarrierManagerBox
|
||
|
|
- Pattern detection → PatternDetectorBox
|
||
|
|
|
||
|
|
2. **Single Responsibility**
|
||
|
|
- Each module has one clear purpose
|
||
|
|
- No mixed concerns (e.g., condition lowering doesn't manage carriers)
|
||
|
|
|
||
|
|
3. **Fail-Fast**
|
||
|
|
- Explicit errors instead of fallback logic
|
||
|
|
- Unsupported patterns return error (not silent failure)
|
||
|
|
|
||
|
|
4. **Backward Compatibility**
|
||
|
|
- Gradual migration with shims
|
||
|
|
- Old API works during transition
|
||
|
|
- Remove shims in Phase 248
|
||
|
|
|
||
|
|
5. **Test-Driven**
|
||
|
|
- 909 tests must pass at each step
|
||
|
|
- No untested refactoring
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Quick Wins (Low-Hanging Fruit)
|
||
|
|
|
||
|
|
### 1. Remove bool_expr_lowerer.rs (Phase 248)
|
||
|
|
- 446 lines, completely unused
|
||
|
|
- TODO comment: "Consider removal or unification"
|
||
|
|
- All tests commented out
|
||
|
|
- Superseded by ExprLowerer (Phase 231)
|
||
|
|
|
||
|
|
### 2. Consolidate TODO Items
|
||
|
|
- 9 TODOs scattered across files
|
||
|
|
- Collect in single tracking issue
|
||
|
|
- Prioritize by impact
|
||
|
|
|
||
|
|
### 3. Update Outdated Comments
|
||
|
|
- Phase 242-EX-A removed loop_with_if_phi_minimal.rs
|
||
|
|
- Update references in mod.rs line 65
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Questions & Answers
|
||
|
|
|
||
|
|
### Q: Why not do all 5 phases at once?
|
||
|
|
**A**: Incremental rollout reduces risk. Each phase is independently valuable and can be paused/adjusted.
|
||
|
|
|
||
|
|
### Q: Will this break existing code?
|
||
|
|
**A**: No. Backward compatibility shims maintain existing API during migration. Tests run at each step.
|
||
|
|
|
||
|
|
### Q: What if tests break?
|
||
|
|
**A**: Fail-Fast. Rollback to previous commit and investigate. No logic changes in initial refactoring (pure reorganization).
|
||
|
|
|
||
|
|
### Q: Can we skip any phases?
|
||
|
|
**A**: Yes. Each phase is independent. Minimum viable: Phase 244 (ConditionLoweringBox). Rest are optional improvements.
|
||
|
|
|
||
|
|
### Q: What's the ROI?
|
||
|
|
**A**: High. 86% reduction in root files, unified APIs, reduced duplication, better maintainability. 4-6 days investment for long-term code health.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Stakeholder Approval Checklist
|
||
|
|
|
||
|
|
- [ ] Review Phase 243-EX documents (this summary + full report + dependency graph)
|
||
|
|
- [ ] Approve Phase 244 implementation plan (ConditionLoweringBox)
|
||
|
|
- [ ] Schedule 1-2 days for Phase 244 implementation
|
||
|
|
- [ ] Assign Phase 244 to implementer (AI or human)
|
||
|
|
- [ ] Track progress in [CURRENT_TASK.md](../../../../CURRENT_TASK.md)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Next Actions
|
||
|
|
|
||
|
|
1. **Review** this summary + linked documents
|
||
|
|
2. **Approve** Phase 244 (ConditionLoweringBox)
|
||
|
|
3. **Start** implementation following detailed plan in [phase243-ex-refactoring-opportunities.md](phase243-ex-refactoring-opportunities.md)
|
||
|
|
4. **Track** in [CURRENT_TASK.md](../../../../CURRENT_TASK.md)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Related Documents
|
||
|
|
|
||
|
|
- **Main Report**: [phase243-ex-refactoring-opportunities.md](phase243-ex-refactoring-opportunities.md)
|
||
|
|
- **Dependency Graph**: [phase243-ex-dependency-graph.md](phase243-ex-dependency-graph.md)
|
||
|
|
- **Quick Summary**: [phase243-ex-summary.md](phase243-ex-summary.md)
|
||
|
|
- **JoinIR Architecture**: [joinir-architecture-overview.md](joinir-architecture-overview.md)
|
||
|
|
- **Current Task**: [CURRENT_TASK.md](../../../../CURRENT_TASK.md)
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Test Status Confirmation
|
||
|
|
|
||
|
|
```
|
||
|
|
✅ test result: ok. 909 passed; 0 failed; 64 ignored; 0 measured; 0 filtered out
|
||
|
|
```
|
||
|
|
|
||
|
|
**Verified**: 2025-12-11 after Phase 243-EX investigation complete
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
**Status**: ✅ Investigation Complete - Ready for Phase 244 Implementation
|
||
|
|
|
||
|
|
**Confidence**: High (clean dependency graph, solid foundation, 909 tests passing)
|
||
|
|
|
||
|
|
**Risk**: Low-Medium (mitigated by gradual migration + backward compatibility)
|
||
|
|
|
||
|
|
**Recommendation**: Proceed with Phase 244 (ConditionLoweringBox) 🚀
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
**End of Phase 243-EX Investigation Summary**
|