fix(codegen): preserve static dims in !pto.tensor_view type#1541
fix(codegen): preserve static dims in !pto.tensor_view type#1541zhangqi-chen wants to merge 2 commits into
Conversation
Emit `ConstInt` shape values directly in the `!pto.tensor_view<...>` type string instead of always writing `?` for every dim. ptoas needs the static shape on the type signature when the SSA producer is an `scf.if` / `scf.for` phi — those phis carry no shape operands to trace back to, so a fully dynamic type `<?x?xf32>` flows into ptoas as `memref<?x?xf32, strided<[?, ?], offset: ?>>` and `pto.partition_view` is rejected. The four emission sites for `!pto.tensor_view<...>` (`GetTensorViewTypeString`, `EmitMakeTensorViews`, `EmitCommRemoteView`, `tensor.as_layout`) are kept in lockstep so `scf.yield` operand types stay equal to `scf.if` result types. Fixes hw-native-sys#1533
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughPTOCodegen now emits ChangesStatic Tensor Dimension Preservation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #1533 by preserving static dimension values (from ConstInt) in the !pto.tensor_view type signature, falling back to ? only for symbolic dimensions. This ensures downstream consumers can resolve shapes even when the SSA producer is an scf.if or scf.for phi. The documentation and unit tests have been updated to reflect this change. The reviewer feedback identifies incorrect dimensions in the documentation examples (using 32x32 instead of 16x1 for a [16, 1] column vector) and suggests refactoring the C++ backend code to reuse the centralized GetTensorViewTypeString helper method to avoid code duplication.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/zh-cn/dev/codegen/00-pto_codegen.md (1)
243-250:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win修正列向量示例中的类型与 shape 不一致问题。
Line 249 写的是
!pto.tensor_view<32x32xf32>，但示例 shape 是[16, 1]；这里应改为!pto.tensor_view<16x1xf32>（或与示例一致的符号形式）。建议修改
- : !pto.tensor_view<32x32xf32> + : !pto.tensor_view<16x1xf32>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/zh-cn/dev/codegen/00-pto_codegen.md` around lines 243 - 250, The example for the column vector is inconsistent: the pto.make_tensor_view call that constructs %col_view uses shape = [%c16_index, %c1_index] but the type annotation is still !pto.tensor_view<32x32xf32>; update the type to match the shape (e.g., change !pto.tensor_view<32x32xf32> to !pto.tensor_view<16x1xf32> or an equivalent symbol form) so %col_view, the pto.make_tensor_view invocation, and the tensor_view type are consistent.docs/en/dev/codegen/00-pto_codegen.md (1)
250-257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the column-vector example type to match the emitted shape.
Line 256 currently shows
!pto.tensor_view<32x32xf32>, but the snippet shape is[16, 1]; this should be!pto.tensor_view<16x1xf32>(or a matching symbolic form) to avoid misleading readers.Proposed doc fix
- : !pto.tensor_view<32x32xf32> + : !pto.tensor_view<16x1xf32>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/en/dev/codegen/00-pto_codegen.md` around lines 250 - 257, The documented column-vector example uses pto.make_tensor_view producing a shape of [16, 1] for %col_view but incorrectly annotates the type as !pto.tensor_view<32x32xf32>; update the type annotation for %col_view (in the pto.make_tensor_view example) to match the emitted shape, e.g. change the type to !pto.tensor_view<16x1xf32> (or an equivalent symbolic form) so the shape and the !pto.tensor_view<> type agree.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/common/pto_ops_common.cpp`:
- Around line 2341-2350: The duplicated manual construction of the tensor view
type string (the loop that writes "x" + either ConstInt value or "?" into
view_type) should be replaced by a single call to the existing
GetTensorViewTypeString(...) helper; locate the hand-rolled logic in
pto_ops_common.cpp (the for-loop using As<ir::ConstInt>(shape[i]) that appends
ci->value_ or "?") and remove it, calling GetTensorViewTypeString(shape, rank)
(or the appropriate signature) instead; do the same for the other occurrence
around the 2985-2995 region so both sites use the central
GetTensorViewTypeString and avoid drift.
---
Outside diff comments:
In `@docs/en/dev/codegen/00-pto_codegen.md`:
- Around line 250-257: The documented column-vector example uses
pto.make_tensor_view producing a shape of [16, 1] for %col_view but incorrectly
annotates the type as !pto.tensor_view<32x32xf32>; update the type annotation
for %col_view (in the pto.make_tensor_view example) to match the emitted shape,
e.g. change the type to !pto.tensor_view<16x1xf32> (or an equivalent symbolic
form) so the shape and the !pto.tensor_view<> type agree.
In `@docs/zh-cn/dev/codegen/00-pto_codegen.md`:
- Around line 243-250: The example for the column vector is inconsistent: the
pto.make_tensor_view call that constructs %col_view uses shape = [%c16_index,
%c1_index] but the type annotation is still !pto.tensor_view<32x32xf32>; update
the type to match the shape (e.g., change !pto.tensor_view<32x32xf32> to
!pto.tensor_view<16x1xf32> or an equivalent symbol form) so %col_view, the
pto.make_tensor_view invocation, and the tensor_view type are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 98dd039f-5812-491f-bf6b-effc6317f5d2
📒 Files selected for processing (6)
docs/en/dev/codegen/00-pto_codegen.mddocs/zh-cn/dev/codegen/00-pto_codegen.mdsrc/backend/common/pto_ops_common.cppsrc/codegen/pto/pto_codegen.cpptests/ut/codegen/test_dynamic_shape.pytests/ut/codegen/test_pto_codegen.py
- docs: restore the [16, 1] column-vector example type to `!pto.tensor_view<16x1xf32>` (the prior sed replacement over-broadly pinned it to 32x32, which contradicts the surrounding text) - backend: reuse `PTOCodegen::GetTensorViewTypeString` in `EmitCommRemoteView` and the `tensor.as_layout` lambda instead of duplicating the static-vs-symbolic dim loop — keeps every emission site in lockstep with the helper
1 participant
Summary
Emit
ConstIntshape values directly in the!pto.tensor_view<...>typestring instead of always writing
?for every dim.ptoas needs the static shape on the type signature when the SSA producer
is an
scf.if/scf.forphi — those phis carry no shape operands totrace back to, so a fully dynamic type
<?x?xf32>flows into ptoas asmemref<?x?xf32, strided<[?, ?], offset: ?>>andpto.partition_viewisrejected.
The four emission sites for
!pto.tensor_view<...>are kept in lockstepso
scf.yieldoperand types stay equal toscf.ifresult types:PTOCodegen::GetTensorViewTypeString(helper used by scf.if/scf.for results)PTOCodegen::EmitMakeTensorViews(function-parameter views)EmitCommRemoteView(distributed remote-load peer views)tensor.as_layout(slice-assign rebind)A pre-existing reference pattern in the
pld.system.waitpath alreadyhandled this correctly and was used as the template.
Behavior
[64, 128]→!pto.tensor_view<64x128xf32>[M, N]→!pto.tensor_view<?x?xf32>(unchanged)[64, M]→!pto.tensor_view<64x?xf32>Testing
python -m pytest tests/ut/codegen/— 344 passedpython -m pytest tests/ut/ --ignore=tests/ut/codegen— 4892 passedtest_pto_codegen_tensor_view_type_preserves_static_dimscovers static and mixed shape casesRelated Issues
Fixes #1533