fix(codegen): materialize transpose scratch tile#1406
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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:
📝 WalkthroughWalkthroughThis PR extends ChangesTile Transpose Scratch Allocation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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.
Actionable comments posted: 1
🤖 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/ir/op/tile_ops/transform.cpp`:
- Around line 344-349: The CHECK messages for the axis constants are using stale
ordinal names; in the block where axis_base is used to fetch axis1_const and
axis2_const (via As<ConstInt>(args[axis_base]) and As<ConstInt>(args[axis_base +
1])), update the error strings to correctly refer to the 3rd and 4th arguments
respectively (e.g., "tile.transpose requires third argument (axis1) to be a
ConstInt" for axis1_const and "tile.transpose requires fourth argument (axis2)
to be a ConstInt" for axis2_const) so diagnostics reflect the 4-arg transpose
ordering.
🪄 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: 68a9cd3e-4e91-41ea-ac55-4d59cef62bcb
📒 Files selected for processing (7)
python/pypto/ir/op/tile_ops.pypython/pypto/language/op/tile_ops.pysrc/backend/common/pto_ops_common.cppsrc/ir/op/tile_ops/transform.cppsrc/ir/transforms/lower_composite_ops_pass.cpptests/ut/codegen/test_pto_codegen_ops.pytests/ut/ir/transforms/test_lower_composite_ops.py
72706b9 to
9f12d8a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ir/op/tile_ops/transform.cpp (1)
337-372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
tmpagainst the inferred transpose result.The 4-arg branch only checks that
tmpis aTileType. A scratch tile with mismatched shape/dtype/valid-shape will still type-check, while the result type is derived entirely frominput. That can hand codegen alloc metadata fromtmpthat disagrees with the transposed result this op claims to produce.🤖 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 `@src/ir/op/tile_ops/transform.cpp` around lines 337 - 372, The 4-arg branch currently only ensures args[1] is a TileType (tmp_type) but must also validate that this scratch tile matches the transpose result: after computing axis1/axis2, build the expected new_shape (swap input_shape[axis1] and [axis2]) and expected new_valid_shape (use GetValidShape(tile_type) and swap when sizes match, otherwise use new_shape), then check tmp_type->dtype_ == tile_type->dtype_, tmp_type->shape == new_shape and that tmp_type's valid region equals or is compatible with new_valid_shape; if any check fails, emit a CHECK with a clear message referencing tile.transpose 4-arg tmp mismatch so codegen won't get allocation metadata that disagrees with the derived result.
🤖 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.
Outside diff comments:
In `@src/ir/op/tile_ops/transform.cpp`:
- Around line 337-372: The 4-arg branch currently only ensures args[1] is a
TileType (tmp_type) but must also validate that this scratch tile matches the
transpose result: after computing axis1/axis2, build the expected new_shape
(swap input_shape[axis1] and [axis2]) and expected new_valid_shape (use
GetValidShape(tile_type) and swap when sizes match, otherwise use new_shape),
then check tmp_type->dtype_ == tile_type->dtype_, tmp_type->shape == new_shape
and that tmp_type's valid region equals or is compatible with new_valid_shape;
if any check fails, emit a CHECK with a clear message referencing tile.transpose
4-arg tmp mismatch so codegen won't get allocation metadata that disagrees with
the derived result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a488c1a4-b94c-45ce-a598-e61b05a175bf
📒 Files selected for processing (7)
python/pypto/language/op/tile_ops.pysrc/ir/op/tile_ops/transform.cppsrc/ir/transforms/lower_composite_ops_pass.cppsrc/ir/transforms/python_printer.cpptests/ut/codegen/test_pto_codegen_ops.pytests/ut/ir/operators/test_tile_ops.pytests/ut/ir/transforms/test_lower_composite_ops.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/ut/ir/transforms/test_lower_composite_ops.py
- src/ir/transforms/lower_composite_ops_pass.cpp
- tests/ut/codegen/test_pto_codegen_ops.py
9f12d8a to
1f8a6a0
Compare
Summary
tile.transpose(src, axis0, axis1)into a 4-arg form with an explicit scratch tile so memory allocation assigns a UB address before PTO codegen.addr,valid_row,valid_col).Testing
cmake --build build --parallelpython -m pytest tests/ut/ir/transforms/test_lower_composite_ops.py tests/ut/codegen/test_pto_codegen_ops.py::TestTileTransposeCodegen -vpython tests/lint/clang_tidy.py --diff-base HEAD(passes; local clang-tidy is 18.1.8 while project expects 21.1.0)git diff --checktask-submit:task_20260519_115111_278459217976Fixes #1402