fix: preserve singleton broadcast dims in SplitVectorKernel#976
fix: preserve singleton broadcast dims in SplitVectorKernel#976ndleslx wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to preserve singleton dimensions during vector kernel splitting for AIV operations by replacing the ComputeHalfDimSize function with ComputeSplitDimInfo. This allows the transformation to skip splitting for dimensions of size 1 when the preserve_singleton flag is enabled, preventing potential value errors. The logic was updated for tile.load, tile.full, and tile.create operations, and a new unit test was added to verify the behavior. Feedback points out that the tile.tpop_from_aic operation was not fully updated to use this new logic, which could lead to crashes or incorrect offset adjustments for singleton tiles.
| std::make_shared<Var>(assign->var_->name_hint_, new_call->GetType(), assign->var_->span_); | ||
| if (tt && split_dim < static_cast<int>(tt->shape_.size())) { | ||
| TileInfo info{ComputeHalfDimSize(tt->shape_[split_dim])}; | ||
| TileInfo info{ComputeSplitDimInfo(tt->shape_[split_dim]).dim_size}; |
There was a problem hiding this comment.
The tile.tpop_from_aic operation handling is incomplete and will still crash when encountering a singleton dimension on the split axis.
- This line calls
ComputeSplitDimInfowith the defaultpreserve_singleton=false, which will throw aValueErrorfor dimensions of size 1. - Additionally,
tile_varsshould only be updated if the split was actually applied (split_applied == true). Otherwise, subsequenttile.storeoperations will incorrectly adjust offsets for the non-split singleton tile. - The helper
RebuildTpopWithHalvedShape(called at line 296) also needs to be updated to support and pass thepreserve_singletonflag toHalveTileShapeto avoid a similar crash during type reconstruction.
References
- When an AIV operation produces a TileType, ensure that any shape-related arguments within the Call itself are also updated (e.g., halved) to maintain type consistency and prevent failures in subsequent passes or codegen.
Redesign the split decision algorithm in SplitVectorKernel to be op-semantics-aware instead of unconditionally halving all tile dims: - Add IsSingletonDim check: tiles with split-axis extent == 1 (e.g. broadcast [1, 128] under UP_DOWN) are now preserved as-is without halving shape, adjusting offsets, or tracking in tile_vars - Add IsReduceOnSplitAxis detection: reduce ops (tile.sum/max/min, tile.row_sum/max/min) that reduce on the split axis are rejected with a clear error, since partial reduction is semantically incorrect - Add regression tests for both UP_DOWN and LEFT_RIGHT singleton broadcast scenarios, plus a reduce-on-split-axis rejection test Fixes hw-native-sys#976 Closes hw-native-sys#975 Made-with: Cursor
Redesign the split decision algorithm in SplitVectorKernel to be op-semantics-aware instead of unconditionally halving all tile dims: - Add IsSingletonDim check: tiles with split-axis extent == 1 (e.g. broadcast [1, 128] under UP_DOWN) are now preserved as-is without halving shape, adjusting offsets, or tracking in tile_vars - Add IsReduceOnSplitAxis detection: reduce ops (tile.sum/max/min, tile.row_sum/max/min) that reduce on the split axis are rejected with a clear error, since partial reduction is semantically incorrect - Add regression tests for both UP_DOWN and LEFT_RIGHT singleton broadcast scenarios, plus a reduce-on-split-axis rejection test Fixes hw-native-sys#976 Closes hw-native-sys#975 Made-with: Cursor
…-native-sys#984) ## Summary - Redesign the split decision algorithm in `SplitVectorKernel` to be op-semantics-aware instead of unconditionally halving all tile dimensions on the split axis - Add `IsSingletonDim` check: tiles with split-axis extent == 1 (e.g. broadcast `[1, 128]` under `UP_DOWN`) are preserved as-is without halving, offset adjustment, or tile tracking - Add `IsReduceOnSplitAxis` detection: reduce ops (`tile.sum/max/min`, `tile.row_sum/max/min`) that reduce on the split axis are rejected with a clear error since partial reduction is semantically incorrect - Add regression tests for UP_DOWN and LEFT_RIGHT singleton broadcast scenarios, plus a reduce-on-split-axis rejection test Fixes hw-native-sys#976 Closes hw-native-sys#975 ## Test plan - [x] All 15 `test_split_vector_kernel.py` tests pass (12 existing + 3 new) - [x] All 980 `tests/ut/ir/transforms/` tests pass with no regressions - [x] Pre-commit hooks pass (clang-format, cpplint, ruff, pyright)
Summary
SplitVectorKernelinstead of halving them unconditionally[1, 128]broadcast tile loads used bycol_expand_mulunderSplitMode.UP_DOWNRepro
This fixes the failure where split AIV kernels hit:
for broadcast tile loads such as
Tile[[1, 128], ...]on the split axis.Testing
pip install -e ".[dev]"python -m pytest tests/ut/ir/transforms/test_split_vector_kernel.py -k singleton_broadcast -vCloses #975