fix(ir): Rewrite lane1 replay loop state#1236
fix(ir): Rewrite lane1 replay loop state#1236lwDavid wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
📝 WalkthroughWalkthroughExtended lane-1 replay reconstruction to explicitly rewrite loop metadata: rebuilds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Code Review
This pull request enhances the split_vector_kernel_pass by correctly rebuilding loop iteration arguments and return variables for lane 1 in dual-dispatch scenarios. It introduces helper functions to handle type updates for TileType and ensures that loop bounds, conditions, and chunk configurations are properly substituted. A new unit test verifies that lane 1 loops use empty accumulators as expected. Feedback was provided to add an internal check to verify the size consistency between return variables and iteration arguments in RebuildLane1LoopReturnVars to enforce IR invariants.
| std::vector<VarPtr> RebuildLane1LoopReturnVars(const std::vector<VarPtr>& return_vars, | ||
| const std::vector<IterArgPtr>& iter_args, | ||
| ExprReplacementMap& replacements) { | ||
| std::vector<VarPtr> new_return_vars; |
There was a problem hiding this comment.
In RebuildLane1LoopReturnVars, it is recommended to add an INTERNAL_CHECK to verify that return_vars and iter_args have the same size. This enforces the IR invariant that loop-carried iteration arguments and their corresponding return variables must be 1-to-1, which helps catch potential issues in IR construction or previous transformation passes.
| std::vector<VarPtr> RebuildLane1LoopReturnVars(const std::vector<VarPtr>& return_vars, | |
| const std::vector<IterArgPtr>& iter_args, | |
| ExprReplacementMap& replacements) { | |
| std::vector<VarPtr> new_return_vars; | |
| std::vector<VarPtr> RebuildLane1LoopReturnVars(const std::vector<VarPtr>& return_vars, | |
| const std::vector<IterArgPtr>& iter_args, | |
| ExprReplacementMap& replacements) { | |
| INTERNAL_CHECK(return_vars.size() == iter_args.size()) | |
| << "Internal error: return_vars and iter_args sizes must match"; | |
| std::vector<VarPtr> new_return_vars; |
References
- Use INTERNAL_CHECK to validate internal invariants that are guaranteed by the design or preceding compiler passes, such as consistency between iter_args and return variables.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/ut/ir/transforms/test_split_vector_kernel.py (1)
868-910: ⚡ Quick winAdd a companion
while-loop replay regression.This new test closes the
for-loop case, but the C++ change also rewritesWhileStmtlane1 iter args / return vars / condition. A smallwhilecase with the same empty-accumulator pattern would keep that symmetric path from drifting unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ut/ir/transforms/test_split_vector_kernel.py` around lines 868 - 910, Add a companion unit test mirroring test_no_split_dual_dispatch_lane1_loop_init_uses_empty_accumulator but using a while-loop to exercise the WhileStmt rewrite path: create a new test (e.g., test_no_split_dual_dispatch_lane1_while_init_uses_empty_accumulator) that builds a pl.program where an accumulator tile (acc) is initialized empty, a while loop consumes from aic and yields next_acc similarly to the for-loop case, then run _run_split_vector_kernel(Before), extract lane1 from python_print(actual) and assert the same patterns (tile.full with TileView valid_shape=[0, 0], pl.range equivalents replaced by the while condition/iter-arg patterns, and the incremented/store checks) so the WhileStmt handling is covered alongside the ForStmt path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/ut/ir/transforms/test_split_vector_kernel.py`:
- Around line 868-910: Add a companion unit test mirroring
test_no_split_dual_dispatch_lane1_loop_init_uses_empty_accumulator but using a
while-loop to exercise the WhileStmt rewrite path: create a new test (e.g.,
test_no_split_dual_dispatch_lane1_while_init_uses_empty_accumulator) that builds
a pl.program where an accumulator tile (acc) is initialized empty, a while loop
consumes from aic and yields next_acc similarly to the for-loop case, then run
_run_split_vector_kernel(Before), extract lane1 from python_print(actual) and
assert the same patterns (tile.full with TileView valid_shape=[0, 0], pl.range
equivalents replaced by the while condition/iter-arg patterns, and the
incremented/store checks) so the WhileStmt handling is covered alongside the
ForStmt path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5cd192a4-166a-478b-9e17-3939509415fe
📒 Files selected for processing (2)
src/ir/transforms/split_vector_kernel_pass.cpptests/ut/ir/transforms/test_split_vector_kernel.py
Summary
Fix no-split dual-AIV lane1 replay so loop-carried tile state uses the zero-valid replay tiles instead of stale lane0 variables.
Add regression coverage for lane1 replay loops with accumulator init values.
Testing