feat(ir): Accept pad_value kwarg on tile.slice#1129
feat(ir): Accept pad_value kwarg on tile.slice#1129lyfne123 merged 2 commits intohw-native-sys:mainfrom
Conversation
Extends tile.slice with an optional pad_value kwarg mirroring tile.fillpad. The value is written into TileView::pad on the output type. DSL wrapper warns when pad_value is set without a valid_shape, since pad only takes effect when valid_shape < shape. This is the first of three PRs unifying slice+fillpad semantics. It is purely additive: default pad_value is null, so existing slice calls produce identical output TileTypes.
📝 WalkthroughWalkthroughThis pull request introduces a new optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Pull request overview
Adds an optional pad_value kwarg to tile.slice across the C++ IR op registry/type-deducer and both Python IR + DSL wrappers, enabling slice to carry padding semantics via TileView::pad (aligned with tile.fillpad behavior).
Changes:
- Extend C++
tile.sliceop registration to acceptpad_valueand plumb it intoDeduceTileSliceTypeby settingtile_view.pad. - Add
pad_valueto Python DSLpl.tile.slicewith aUserWarningwhen used withoutvalid_shape. - Add Python IR-layer forwarding for
ir.op.tile.slice(..., pad_value=...)and add unit tests + kwarg-schema coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/ir/op/tile_ops/transform.cpp |
Adds pad_value kwarg schema + sets TileView::pad during type deduction for tile.slice. |
python/pypto/language/op/tile_ops.py |
Exposes pad_value on pl.tile.slice and emits a warning when it won’t take effect. |
python/pypto/ir/op/tile_ops.py |
Forwards pad_value into create_op_call kwargs when provided. |
tests/ut/ir/operators/test_tile_ops.py |
Adds unit tests covering pad_value propagation, defaulting, bad-type rejection, and DSL warning behavior. |
tests/ut/ir/operators/test_op_registry.py |
Adds a schema test ensuring tile.slice declares pad_value as an allowed kwarg. |
![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 introduces a pad_value parameter to the tile.slice operation across the IR, language, and C++ layers, allowing for padding of out-of-bounds elements. It also includes corresponding unit tests and attribute registration. The review feedback suggests enhancing the implementation by supporting numeric literals for pad_value and using normalize_pad_value for consistency with other operations like tile.fillpad.
…e-sys#1129 Mirror the numeric-literal sugar that main's newly-landed tile.fillpad accepts, by routing non-null pad_value through normalize_pad_value: - Both the DSL wrapper (pl.tile.slice) and the IR-layer wrapper (ir.op.tile.slice) now accept PadValue | int | float | None. - Non-null values are validated at the Python boundary via normalize_pad_value; PadValue.null is passed through unchanged (slice treats it as "no padding", unlike fillpad which rejects it). - Docstrings now document None / PadValue.null as the no-op default and the 0 / math.inf / -math.inf literal sugar. - New tests: numeric-sugar round-trip, Python-boundary rejection of non-sugar numeric values. Addresses gemini-code-assist and copilot review comments on hw-native-sys#1129.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
python/pypto/ir/op/tile_ops.py (1)
1917-1923:⚠️ Potential issue | 🟠 MajorPreserve positional
spancompatibility.Line 1922 inserts
pad_valuebefore the existingspanparameter, so old calls likeslice(tile, shape, offset, valid_shape, span)now treatspanaspad_valueand fail normalization. Since this is a kwarg feature, keepspanin its old positional slot and makepad_valuekeyword-only.🐛 Proposed fix
def slice( tile: Expr, shape: Sequence[int | Expr] | _ir_core.MakeTuple, offset: Sequence[int | Expr] | _ir_core.MakeTuple, valid_shape: Sequence[int | Expr] | _ir_core.MakeTuple | None = None, - pad_value: PadValue | int | float | None = None, span: Span | None = None, + *, + pad_value: PadValue | int | float | None = None, ) -> Call:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/pypto/ir/op/tile_ops.py` around lines 1917 - 1923, The new parameter ordering broke positional compatibility for slice(...) by inserting pad_value before span; to restore compatibility, keep span as the last positional parameter in the slice function signature and make pad_value keyword-only by inserting a bare * before pad_value (i.e., leave parameters up through span unchanged and place a * so pad_value can only be passed by name). Update the slice function declaration and any related docstrings/annotations to reflect pad_value as keyword-only while leaving span in its original positional slot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@python/pypto/ir/op/tile_ops.py`:
- Around line 1917-1923: The new parameter ordering broke positional
compatibility for slice(...) by inserting pad_value before span; to restore
compatibility, keep span as the last positional parameter in the slice function
signature and make pad_value keyword-only by inserting a bare * before pad_value
(i.e., leave parameters up through span unchanged and place a * so pad_value can
only be passed by name). Update the slice function declaration and any related
docstrings/annotations to reflect pad_value as keyword-only while leaving span
in its original positional slot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 67ad3d24-01ce-4be0-b981-a1be036edcf7
📒 Files selected for processing (5)
python/pypto/ir/op/tile_ops.pypython/pypto/language/op/tile_ops.pysrc/ir/op/tile_ops/transform.cpptests/ut/ir/operators/test_op_registry.pytests/ut/ir/operators/test_tile_ops.py
Summary
tile.slicewith an optionalpad_valuekwarg mirroringtile.fillpad. The value is written intoTileView::padon the output type.pl.tile.slicegains apad_value: PadValue | None = Noneparameter and emits aUserWarningwhenpad_valueis set without avalid_shape, since pad only takes effect whenvalid_shape < shape.ir.op.tile.sliceforwards the new kwarg; existing callers unaffected (defaultPadValue.null→ identical outputTileType).tests/ut/ir/operators/test_tile_ops.pyplus a kwarg-schema check intest_op_registry.py.This is the first of three PRs unifying slice+fillpad semantics. It is purely additive —
tile.fillpadremains a first-class op, and the optimization pass that fusesslice → fillpadlives in a separate follow-up PR.Testing
tests/ut/ir/operators/test_tile_ops.py::TestTileSliceReshapeOps— 18/18 passtests/ut/ir/operators/test_op_registry.py— 112/112 passtest_legalize_pto_buffer_reuse.py,test_memory_reuse.py,test_pto_codegen_slice_fillpad_partial_dynamic_valid_shape) — all passtests/ut/suite: 3909 passed, 16 skipped (unchanged baseline)Notes for reviewers
DeduceTileSliceTypeuses the exact template mirrored across the planned 3-PR series (typeid check + enum-range CHECK), so futuretensor.sliceand the fuse pass will share the pattern verbatim.pad_value = PadValue::null→ existingslice(...)calls produce identical outputTileTypeas before; no existing test required modification.test_tile_slice_rejects_bad_pad_value) assertsTypeErrorfromOpRegistry::ValidateKwargs, which fires before the deducer-level CHECK. The typeid CHECK in the deducer is retained as defense-in-depth and to match the shared validation template.