feat(ir): Add pad field to TensorView and tensor.slice pad_value kwarg#1136
feat(ir): Add pad field to TensorView and tensor.slice pad_value kwarg#1136lyfne123 merged 2 commits intohw-native-sys:mainfrom
Conversation
Mirrors the tile-layer change from hw-native-sys#1129: TensorView gains a PadValue pad field (default PadValue::null), and tensor.slice accepts a pad_value kwarg that is written into the output TensorView::pad. DSL wrapper warns when pad_value is set without valid_shape. convert_tensor_to_tile_ops_pass forwards the kwarg so tensor.slice with pad lowers cleanly to tile.slice with pad (supported since hw-native-sys#1129). Serializer, deserializer, printer, structural_equal, and simplify/SSA type-remap all handle the new field; old serialized files without the field deserialize to null (backward compatible).
|
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 (2)
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client as Tensor DSL
end
rect rgba(200,255,200,0.5)
participant SliceOp as tensor.slice
participant PythonWrapper as python wrapper
end
rect rgba(255,200,200,0.5)
participant TypeDeduce as DeduceTensorSliceType
participant TensorView as TensorView/TensorType
end
rect rgba(240,240,160,0.5)
participant Serializer as Serialization
end
Client->>PythonWrapper: call slice(..., valid_shape?, pad_value?)
PythonWrapper->>SliceOp: normalize pad_value (numeric sugar) and forward kwarg
SliceOp->>TypeDeduce: forward pad_value in kwargs
TypeDeduce->>TypeDeduce: validate pad_value ∈ {null,zero,max,min}
TypeDeduce->>TensorView: construct TensorView(..., valid_shape?, pad=pad_value)
TensorView-->>TypeDeduce: return TensorType with pad
TypeDeduce-->>SliceOp: typed result
Client->>Serializer: serialize IR (includes TensorView.pad)
Serializer->>Serializer: encode pad as string enum
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 introduces PadValue support to TensorView, enabling the tensor.slice operation to handle padding for out-of-valid-region accesses. The implementation updates the C++ IR core, Python bindings, serialization logic, and various IR transformation passes to ensure padding modes are correctly preserved and utilized. Feedback focuses on resolving a NameError in the Python DSL due to a missing import, correcting a misleading documentation comment in the C++ header, and adding runtime assertions to enum switch statements to ensure robustness and consistency with project safety standards.
![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 (3)
src/ir/serialization/serializer.cpp (1)
343-378:⚠️ Potential issue | 🟠 MajorRound-trip
TensorView::valid_shapewith the new pad payload.
SerializeTensorViewnow emitspad, but the same TensorView payload still omitsvalid_shape. Atensor.slice(..., valid_shape=..., pad_value=...)can round-trip withpadpreserved but an emptyvalid_shape, changing fill/padding bounds semantics.Proposed fix
msgpack::object SerializeTensorView(const std::optional<TensorView>& tensor_view, msgpack::zone& zone) { if (!tensor_view.has_value()) { return msgpack::object(); // null } std::map<std::string, msgpack::object> tv_map; // Serialize stride std::vector<msgpack::object> stride_vec; for (const auto& dim : tensor_view->stride) { stride_vec.push_back(SerializeNode(dim, zone)); } tv_map["stride"] = msgpack::object(stride_vec, zone); + + // Serialize valid_shape + std::vector<msgpack::object> valid_shape_vec; + for (const auto& dim : tensor_view->valid_shape) { + valid_shape_vec.push_back(SerializeNode(dim, zone)); + } + tv_map["valid_shape"] = msgpack::object(valid_shape_vec, zone); // Serialize layout enum tv_map["layout"] = msgpack::object(TensorLayoutToString(tensor_view->layout), zone);Companion deserializer update:
if (key == "stride") { if (p->val.type == msgpack::type::ARRAY) { for (uint32_t i = 0; i < p->val.via.array.size; ++i) { tensor_view.stride.push_back( std::static_pointer_cast<const Expr>(DeserializeNode(p->val.via.array.ptr[i], zone))); } } + } else if (key == "valid_shape") { + if (p->val.type == msgpack::type::ARRAY) { + for (uint32_t i = 0; i < p->val.via.array.size; ++i) { + tensor_view.valid_shape.push_back( + std::static_pointer_cast<const Expr>(DeserializeNode(p->val.via.array.ptr[i], zone))); + } + } } else if (key == "layout") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ir/serialization/serializer.cpp` around lines 343 - 378, SerializeTensorView currently writes "pad" but omits TensorView::valid_shape, so valid_shape is lost on round-trip; update SerializeTensorView to also emit a "valid_shape" entry by serializing each dimension similar to how "stride" is handled (use SerializeNode on each element of tensor_view->valid_shape and place the resulting vector into tv_map["valid_shape"]), and ensure the deserializer (e.g., DeserializeTensorView or the corresponding parsing routine) is updated to read "valid_shape" when present to fully restore TensorView state.src/ir/op/tensor_ops/memory.cpp (1)
202-229:⚠️ Potential issue | 🟡 MinorConsider documenting or reconciling the asymmetry between
tensor.sliceandtile.sliceregarding view creation.When
pad_value != nullbut no 4thvalid_shapeargument is supplied (lines 224–228),tensor.sliceproduces aTensorTypewith aTensorViewcarrying onlypad(emptyvalid_shape, ND layout). However,tile.slicealways creates a view (regardless ofpad_valueorvalid_shapepresence), making the two ops diverge in their view-creation logic. The DSL-levelUserWarningnotes "pad_value has no effect" withoutvalid_shape, but the IR still materializes a view that downstreamif tensor_view.has_value()checks will pick up—differing from the no-pad/no-valid_shape path (line 229), which returns a view-less type.If this intentional deviation from
tile.sliceis by design, add a comment clarifying why. Otherwise, dropping thetensor_view_whenvalid_shapeis empty would keep the view-presence invariant consistent with the DSL warning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ir/op/tensor_ops/memory.cpp` around lines 202 - 229, The code creates a TensorView when pad_value != PadValue::null even if no valid_shape (4th arg) was supplied, which makes tensor.slice return a type-with-view in that case (contradicting the DSL warning and diverging from tile.slice); change the logic so that if no valid_shape is provided you do not construct a TensorView just because pad_value is set — return std::make_shared<TensorType>(new_shape, tensor_type->dtype_) (i.e., no tensor_view) when args.size() != 4 and pad_value != PadValue::null; update or add a short comment near the TensorView creation/return explaining that pad_value alone does not create a view to preserve the invariant checked by tensor_view.has_value().include/pypto/ir/type.h (1)
156-224:⚠️ Potential issue | 🟡 MinorBackward compatibility for missing
padfield is correctly implemented, but test coverage needs expansion.
PadValuehoisting andTensorView::padaddition are clean. Enum is now shared by bothTensorViewandTileView; new constructor params default toPadValue::null(ABI-compatible); reflection descriptor is updated.The deserializer at
src/ir/serialization/deserializer.cpp:310explicitly handles omittedpadfields ("Older serialized IR may omit "pad"; default stays PadValue::null."). The manual field-by-field deserialization inDeserializeTensorView()gracefully skips missing fields, retaining the default.However, the test at
tests/ut/ir/transforms/test_serialization.py:769only round-trips freshly-constructed objects, not actual pre-existing serialized payloads. To fully verify backward compatibility, add a fixture containing a pre-PR TensorView serialization (without thepadfield) and deserialize it to confirm it loads correctly withpaddefaulting tonull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/pypto/ir/type.h` around lines 156 - 224, The existing tests only round-trip new TensorView objects, so they don't verify backward compatibility for omitted pad; add a unit test fixture containing a pre-change serialized TensorView blob that lacks the "pad" field, then in the test call the deserializer (DeserializeTensorView) to load that blob and assert the resulting TensorView.pad equals PadValue::null; update tests/ut/ir/transforms/test_serialization.py to load the fixture, deserialize, and assert pad defaulting, keeping existing round-trip checks unchanged.
🧹 Nitpick comments (1)
python/bindings/modules/ir.cpp (1)
294-300: Make thePadValuedocstring view-neutral.
PadValuenow applies to bothTensorViewandTileView, so the Python binding help text should not call it tile-only.Proposed wording tweak
- nb::enum_<PadValue>(ir, "PadValue", "Tile pad mode enumeration") + nb::enum_<PadValue>(ir, "PadValue", "Pad mode enumeration for tensor and tile views")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/bindings/modules/ir.cpp` around lines 294 - 300, Update the PadValue enum docstring to be view-neutral: change the nb::enum_<PadValue>(ir, "PadValue", "Tile pad mode enumeration") description to a neutral phrase (e.g., "Padding mode enumeration used by TensorView and TileView" or simply "Padding mode enumeration") so the Python help text no longer implies it's tile-only; this affects the PadValue declaration associated with TensorView and TileView in ir.cpp.
🤖 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 `@include/pypto/ir/type.h`:
- Around line 156-224: The existing tests only round-trip new TensorView
objects, so they don't verify backward compatibility for omitted pad; add a unit
test fixture containing a pre-change serialized TensorView blob that lacks the
"pad" field, then in the test call the deserializer (DeserializeTensorView) to
load that blob and assert the resulting TensorView.pad equals PadValue::null;
update tests/ut/ir/transforms/test_serialization.py to load the fixture,
deserialize, and assert pad defaulting, keeping existing round-trip checks
unchanged.
In `@src/ir/op/tensor_ops/memory.cpp`:
- Around line 202-229: The code creates a TensorView when pad_value !=
PadValue::null even if no valid_shape (4th arg) was supplied, which makes
tensor.slice return a type-with-view in that case (contradicting the DSL warning
and diverging from tile.slice); change the logic so that if no valid_shape is
provided you do not construct a TensorView just because pad_value is set —
return std::make_shared<TensorType>(new_shape, tensor_type->dtype_) (i.e., no
tensor_view) when args.size() != 4 and pad_value != PadValue::null; update or
add a short comment near the TensorView creation/return explaining that
pad_value alone does not create a view to preserve the invariant checked by
tensor_view.has_value().
In `@src/ir/serialization/serializer.cpp`:
- Around line 343-378: SerializeTensorView currently writes "pad" but omits
TensorView::valid_shape, so valid_shape is lost on round-trip; update
SerializeTensorView to also emit a "valid_shape" entry by serializing each
dimension similar to how "stride" is handled (use SerializeNode on each element
of tensor_view->valid_shape and place the resulting vector into
tv_map["valid_shape"]), and ensure the deserializer (e.g., DeserializeTensorView
or the corresponding parsing routine) is updated to read "valid_shape" when
present to fully restore TensorView state.
---
Nitpick comments:
In `@python/bindings/modules/ir.cpp`:
- Around line 294-300: Update the PadValue enum docstring to be view-neutral:
change the nb::enum_<PadValue>(ir, "PadValue", "Tile pad mode enumeration")
description to a neutral phrase (e.g., "Padding mode enumeration used by
TensorView and TileView" or simply "Padding mode enumeration") so the Python
help text no longer implies it's tile-only; this affects the PadValue
declaration associated with TensorView and TileView in ir.cpp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 87089395-448a-47a5-906c-b7ccca4ba9a9
📒 Files selected for processing (22)
docs/en/dev/ir/02-types.mddocs/zh-cn/dev/ir/02-types.mdinclude/pypto/ir/transforms/utils/memref_utils.hinclude/pypto/ir/type.hpython/bindings/modules/ir.cpppython/pypto/ir/op/tensor_ops.pypython/pypto/ir/type.pypython/pypto/language/op/tensor_ops.pypython/pypto/pypto_core/ir.pyisrc/ir/op/tensor_ops/memory.cppsrc/ir/serialization/deserializer.cppsrc/ir/serialization/serializer.cppsrc/ir/transforms/convert_to_ssa_pass.cppsrc/ir/transforms/op_conversion_registry.cppsrc/ir/transforms/python_printer.cppsrc/ir/transforms/simplify_pass.cppsrc/ir/transforms/structural_equal.cppsrc/ir/type.cpptests/ut/ir/operators/test_op_registry.pytests/ut/ir/operators/test_tensor_ops.pytests/ut/ir/transforms/test_convert_tensor_to_tile_ops.pytests/ut/ir/transforms/test_serialization.py
There was a problem hiding this comment.
Pull request overview
Adds tensor-layer parity with the existing tile-layer padding semantics by introducing TensorView::pad and a pad_value kwarg on tensor.slice, then wires the new field through serialization, printing, equality, remap/transforms, Python bindings/stubs, and conversion to tile ops.
Changes:
- Add
PadValue padtoTensorView(defaultnull) and plumb it through C++/Python type APIs, printer, serializer/deserializer, and structural equality. - Extend
tensor.slice(..., pad_value=...)across C++ op registration/deduction, Python IR op wrapper, and DSL wrapper (including warning behavior). - Add unit tests and update EN/zh-cn IR type docs for the new field and kwarg.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ut/ir/transforms/test_serialization.py | Adds round-trip coverage for TensorView.pad in serialization. |
| tests/ut/ir/transforms/test_convert_tensor_to_tile_ops.py | Tests pad_value forwarding on the local-lowering path (tensor.slice → tile.slice). |
| tests/ut/ir/operators/test_tensor_ops.py | Adds tensor.slice pad_value behavior, defaults, validation, numeric sugar, and DSL warning tests. |
| tests/ut/ir/operators/test_op_registry.py | Verifies tensor.slice kwarg schema includes pad_value. |
| src/ir/type.cpp | Updates TensorView int-ctor to accept/initialize pad. |
| src/ir/transforms/structural_equal.cpp | Compares TensorView.pad during type structural equality. |
| src/ir/transforms/simplify_pass.cpp | Preserves TensorView.pad when rebuilding simplified views. |
| src/ir/transforms/python_printer.cpp | Prints TensorView(pad=...) when non-null; skips view when all defaults. |
| src/ir/transforms/op_conversion_registry.cpp | Forwards pad_value on tensor.slice→tile.slice conversions; notes drop on tile.load path. |
| src/ir/transforms/convert_to_ssa_pass.cpp | Preserves TensorView.pad when rebuilding SSA-substituted views. |
| src/ir/serialization/serializer.cpp | Serializes TensorView.pad using the same string encoding as TileView.pad. |
| src/ir/serialization/deserializer.cpp | Deserializes TensorView.pad with backward-compatible defaulting to null. |
| src/ir/op/tensor_ops/memory.cpp | Adds pad_value kwarg handling to tensor.slice type deduction and registers the attr. |
| python/pypto/pypto_core/ir.pyi | Updates TensorView stub to include pad field/constructor arg. |
| python/pypto/language/op/tensor_ops.py | Adds DSL pl.tensor.slice(..., pad_value=...) param and warning when valid_shape is absent. |
| python/pypto/ir/type.py | Updates _TensorViewMeta.__call__ to accept/forward pad and treat all-default as empty view. |
| python/pypto/ir/op/tensor_ops.py | Adds IR wrapper pad_value kwarg support with shared normalization. |
| python/bindings/modules/ir.cpp | Moves/binds PadValue before TensorView and adds TensorView.pad bindings/ctors. |
| include/pypto/ir/type.h | Introduces PadValue earlier and adds TensorView::pad + reflection descriptor. |
| include/pypto/ir/transforms/utils/memref_utils.h | Preserves TensorView.pad in RemapTensorViewExprs. |
| docs/zh-cn/dev/ir/02-types.md | Documents TensorView.pad and tensor.slice(..., pad_value=...) in zh-cn types doc. |
| docs/en/dev/ir/02-types.md | Documents TensorView.pad and tensor.slice(..., pad_value=...) in EN types doc. |
Comments suppressed due to low confidence (1)
src/ir/transforms/op_conversion_registry.cpp:239
- In the TensorType path of the
tensor.sliceconversion, the implementation returns atile.loadcall without propagatingpad_valueanywhere. This meanstensor.slice(..., pad_value=...)semantics are silently dropped for global tensors (while being preserved for local TileType inputs), which can lead to incorrect behavior downstream. Consider preserving the metadata by attachingpad_valueto the result (e.g., via an additional view op) or explicitly rejecting/diagnosingpad_valueon this lowering path untiltile.loadcan carry it.
std::vector<std::pair<std::string, std::any>> load_kwargs = {{"target_memory", MemorySpace::Vec},
{"transpose", false}};
auto load_call =
op_reg.Create("tile.load", {input, offset, shape, valid_shapes}, load_kwargs, span);
return ConversionResult{load_call};
…#1136 - Clarify TensorView::pad comment: "accesses outside valid_shape but within shape" instead of the misleading "inside valid_shape". - Rename PadValue Doxygen brief from "Tile pad enumeration" to "Pad mode enumeration (shared by TileView and TensorView)". - Update the PadValue nanobind docstring to reflect the same shared usage across tile and tensor views. No functional changes.
Summary
Third and final PR of the slice+fillpad unification series (follows #1129). Mirrors the tile-layer change at the tensor layer so
TensorView::padandtensor.slice(..., pad_value=...)become peers of the existingTileView::pad/tile.slice(..., pad_value=...).TensorView::pad: newPadValuefield, defaultPadValue::null. Peer of the existingTileView::pad, same enum and same encoding everywhere.tensor.sliceacceptspad_value: writes the kwarg into the outputTensorView::pad. Validation block matchestile.sliceexactly (same error wording).pl.tensor.sliceadds apad_valueparam and emitsUserWarningwhenpad_valueis set withoutvalid_shape(same message template aspl.tile.slice).convert_tensor_to_tile_ops_passforwardspad_valuethroughtensor.slice → tile.slice. Thetensor.slice → tile.loadpath leaves the kwarg dropped (tile.load doesn't carrypad_value); noted inline.paddefault tonull), Python printer,structural_equal, and thesimplify/convert_to_ssa/RemapTensorViewExprstype-remap paths all preserve the new field.ir.pyistub →_TensorViewMeta.__call__metaclass wrapper.PadValueenum moved above bothTensorViewandTileViewsince both now carry it.No changes to
tile.fillpad,tensor.fillpad,fillpad_inplace,set_validshape, or any codegen path — backend already handles the merged form.FuseFillpadIntoSlice(PR 2 of the series) is not in this PR.Testing
test_tensor_ops.py:test_tensor_slice_with_pad_value,test_tensor_slice_default_pad_is_null,test_tensor_slice_rejects_bad_pad_value,test_tensor_slice_accepts_numeric_sugar_pad_value,test_tensor_slice_pad_without_valid_shape_warnstest_op_registry.py:test_tensor_slice_pad_value_kwarg_schematest_convert_tensor_to_tile_ops.py:test_local_tensor_slice_with_pad_value_forwards_to_tile_slicetest_serialization.py:test_tensortype_tensorview_pad_survives_round_trippython -m pytest tests/ut/ -n auto --maxprocesses 8→ 4023 passed, 17 skippedtensor.slice()withoutpad_valuestill produces the originalTensorType(notensor_viewwhen neithervalid_shapenorpad_valueis set)pad = PadValue::nullRelated
Follows #1129 (tile-layer
pad_valuekwarg ontile.slice).