feat(ir): Added tensor.expand_clone ops#851
Open
wuzhf9 wants to merge 1 commit intohw-native-sys:mainfrom
Open
feat(ir): Added tensor.expand_clone ops#851wuzhf9 wants to merge 1 commit intohw-native-sys:mainfrom
wuzhf9 wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
|
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 adds a new unified op Changes
Sequence DiagramsequenceDiagram
participant Client as IR Input
participant PassMgr as Pass Manager
participant SubPass as SubstituteTiles Pass
participant Visitor as IR Visitor
participant Builder as IR Builder
participant Output as Transformed IR
Client->>PassMgr: submit function with tile.expand_clone
PassMgr->>SubPass: run SubstituteTiles
activate SubPass
SubPass->>Visitor: traverse statements
activate Visitor
loop for each tile.expand_clone call
Visitor->>Visitor: validate call (arity, types, literal shape)
Visitor->>Visitor: compute broadcast axis (≤1) using type-inference helpers
Visitor->>Builder: request replacement IR
activate Builder
alt broadcast axis present
Builder->>Builder: emit `tile.create` (dest)
Builder->>Builder: emit `for i in 0..N` loop (INDEX)
Builder->>Builder: per-iter offsets with loop var
Builder->>Builder: emit `tile.assemble` inside loop, yield
else no broadcast
Builder->>Builder: emit `tile.create`
Builder->>Builder: emit single `tile.assemble` with zeros offsets
end
Builder-->>Visitor: return replacement statements
deactivate Builder
Visitor->>Visitor: splice replacement into AST
end
Visitor-->>SubPass: traversal complete
deactivate Visitor
SubPass-->>Output: transformed IR (no tile.expand_clone)
deactivate SubPass
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the expand_clone operation for both tensors and tiles, allowing for data expansion via cloning rather than standard broadcasting. It includes the necessary IR operation registrations, Python bindings, and a new SubstituteTiles pass that lowers tile.expand_clone into a combination of tile.create, tile.assemble, and for loops. Feedback highlights a potential crash in the lowering pass when handling the optional valid_shape argument and suggests adding the NoNestedCalls property to the pass requirements to align with the current implementation's assumptions.
| int temp_var_id_ = 0; | ||
|
|
||
| StmtPtr RewriteExpandClone(const CallPtr& call, const VarPtr& result_var, const Span& span) { | ||
| CHECK(call->args_.size() == 2) << "SubstituteTiles: tile.expand_clone expects 2 arguments"; |
Contributor
There was a problem hiding this comment.
The tile.expand_clone operation lowering currently only supports 2 arguments, but the tensor.expand_clone operation (which is converted to tile.expand_clone) supports an optional 3rd argument valid_shape. If a tensor operation with valid_shape is converted, this pass will crash. Please update the lowering to handle the optional 3rd argument or ensure it is handled during conversion.
Comment on lines
+88
to
+90
| inline const PassProperties kSubstituteTilesProperties{ | ||
| .required = {IRProperty::SSAForm, IRProperty::IncoreTileOps, IRProperty::NormalizedStmtStructure}, | ||
| .produced = {IRProperty::SSAForm, IRProperty::IncoreTileOps, IRProperty::NormalizedStmtStructure}}; |
Contributor
There was a problem hiding this comment.
The SubstituteTiles pass implementation in substitute_tiles_pass.cpp only overrides VisitStmt_ and assumes that tile.expand_clone calls are not nested within other expressions. Therefore, this pass should explicitly require the NoNestedCalls property to ensure correctness.
Suggested change
| inline const PassProperties kSubstituteTilesProperties{ | |
| .required = {IRProperty::SSAForm, IRProperty::IncoreTileOps, IRProperty::NormalizedStmtStructure}, | |
| .produced = {IRProperty::SSAForm, IRProperty::IncoreTileOps, IRProperty::NormalizedStmtStructure}}; | |
| inline const PassProperties kSubstituteTilesProperties{ | |
| .required = {IRProperty::SSAForm, IRProperty::IncoreTileOps, IRProperty::NormalizedStmtStructure, IRProperty::NoNestedCalls}, | |
| .produced = {IRProperty::SSAForm, IRProperty::IncoreTileOps, IRProperty::NormalizedStmtStructure}}; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/ir/op/tile_ops/broadcast.cpp (1)
153-164: Improve CHECK message formatting for readability.Line 153, Line 158, and Line 163 miss a space after
op_name, producing messages liketile.expand_clonerequires ....✏️ Suggested tweak
- CHECK(args.size() == 2) << op_name << "requires exactly 2 arguments (input, shape), but got " + CHECK(args.size() == 2) << op_name << " requires exactly 2 arguments (input, shape), but got " << args.size(); ... - CHECK(tile_type) << op_name << "requires first argument to be a TileType, but got " + CHECK(tile_type) << op_name << " requires first argument to be a TileType, but got " << args[0]->GetType()->TypeName(); ... - CHECK(shape_tuple_type) << op_name << "requires shape to be TupleType, but got " + CHECK(shape_tuple_type) << op_name << " requires shape to be TupleType, but got " << args[1]->GetType()->TypeName();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ir/op/tile_ops/broadcast.cpp` around lines 153 - 164, The CHECK messages concatenating op_name lack a separating space, producing messages like "tile.expand_clonerequires..."; update the three CHECK calls that reference op_name (the one asserting args.size()==2, the one validating tile_type via As<TileType>, and the one validating shape_tuple_type via As<TupleType>) to include a space after op_name in the formatted string (e.g., change '" << op_name << "requires' to '" << op_name << " requires"') so the operator name and the rest of the message are separated for readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/zh-cn/dev/passes/00-pass_manager.md`:
- Line 71: The table row for SubstituteTiles is missing NormalizedStmtStructure
which causes a mismatch with the implementation: update the table row for
"SubstituteTiles" so its input and output columns include
"NormalizedStmtStructure" to match the kSubstituteTilesProperties (which lists
NormalizedStmtStructure as required/produced); ensure the document's SSAForm,
IncoreTileOps entries reflect the same required/produced fields as the code.
In `@python/pypto/language/op/tensor_ops.py`:
- Around line 605-617: The public function expand_clone currently only accepts
shape but the IR op _ir_ops.expand_clone also supports an optional valid_shape;
update the expand_clone signature to accept valid_shape:
Optional[Sequence[IntLike]] = None, normalize it (e.g. via _normalize_intlike)
and pass it to _ir_ops.expand_clone alongside the normalized shape, and update
the docstring/type hints accordingly; keep existing use of input.unwrap() and
return Tensor(expr=call_expr).
In `@python/pypto/language/op/unified_ops.py`:
- Around line 298-304: The unified expand_clone currently only accepts shape so
callers cannot pass a tensor-level valid_shape; update the expand_clone
signature to accept an optional valid_shape (e.g. valid_shape:
Optional[Sequence[IntLike]] = None) and pass it through to the tensor
implementation (_tensor.expand_clone(input, shape, valid_shape)); keep calling
_tile.expand_clone(input, shape) for Tile or forward valid_shape if Tile
supports it. Also update the Type hints and the error message path in
expand_clone to reflect the new parameter so callers can supply valid_shape when
operating on Tensors.
In `@src/ir/op/tensor_ops/broadcast.cpp`:
- Around line 176-180: DeduceTensorExpandCloneType was changed to accept an
optional third valid_shape arg but downstream code (DeduceTileExpandCloneType
and SubstituteTiles::RewriteExpandClone) still expects exactly 2 args, causing a
mismatch; revert the tensor-level op to require exactly 2 arguments for now:
change the argument check in DeduceTensorExpandCloneType back to args.size()==2
and remove/document any mentions of a 3-arg signature (also apply the same
revert to the other two sites you noted around 246-252 and 359-369), or
alternatively fully thread the third argument through DeduceTileExpandCloneType
and SubstituteTiles::RewriteExpandClone before exposing valid_shape — reference
DeduceTileExpandCloneType and SubstituteTiles::RewriteExpandClone when making
the consistent change.
In `@src/ir/transforms/op_conversion_registry.cpp`:
- Line 152: The current RegisterSimple("tensor.expand_clone",
"tile.expand_clone") must be replaced with a custom conversion because
DeduceTensorExpandCloneType accepts 2 or 3 args while tile.expand_clone only
accepts 2; implement a RegisterCustom conversion for "tensor.expand_clone" that
inspects the Relay Call (or Expr) argument count: if args.size() == 2, lower to
a tile.expand_clone call with the two arguments; if args.size() == 3 either (a)
emit a clear CHECK/LOGGED_ERROR rejecting the 3-arg form (e.g.
"tensor.expand_clone with valid_shape is not supported in lowering") or (b)
propagate the third valid_shape through and construct a tile.expand_clone
variant that accepts it (update type deduction accordingly). Reference
DeduceTensorExpandCloneType to keep type behavior consistent and replace the
RegisterSimple usage with this custom handler.
In `@src/ir/transforms/substitute_tiles_pass.cpp`:
- Around line 49-62: ExtractConstShape currently forces a literal MakeTuple of
ConstInt and crashes on the dynamic tuple form allowed by
DeduceTileExpandCloneType; update the rewrite to accept the runtime tuple shape
instead of hard-failing. In practice, change ExtractConstShape to (1) accept a
MakeTuple whose elements may be ConstInt OR runtime integer scalar expressions
produced by DeduceTileExpandCloneType (e.g., TupleGetItemExpr / index/int scalar
Exprs), push those elements into the returned vector without CHECKing ConstInt,
and only error if shape is not a MakeTuple at all or the tuple is empty; or
alternatively, move a validation into DeduceTileExpandCloneType to reject
non-constant shapes earlier. Reference: function ExtractConstShape and
DeduceTileExpandCloneType / TupleGetItemExpr handling in
src/ir/op/tile_ops/broadcast.cpp.
In `@tests/ut/ir/operators/test_tensor_ops.py`:
- Around line 1479-1489: The test uses an invalid target shape [8, 16, 8] for
expand_clone given the input tensor_var shape [4, 1, 8]; update the target shape
passed to ir.op.tensor.expand_clone to a valid non-broadcast match (e.g., [4,
16, 8]) so non-broadcast axes align with the input, and keep the rest of the
assertions (call, call.op.name, result_type checks) unchanged.
---
Nitpick comments:
In `@src/ir/op/tile_ops/broadcast.cpp`:
- Around line 153-164: The CHECK messages concatenating op_name lack a
separating space, producing messages like "tile.expand_clonerequires..."; update
the three CHECK calls that reference op_name (the one asserting args.size()==2,
the one validating tile_type via As<TileType>, and the one validating
shape_tuple_type via As<TupleType>) to include a space after op_name in the
formatted string (e.g., change '" << op_name << "requires' to '" << op_name << "
requires"') so the operator name and the rest of the message are separated for
readability.
🪄 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: c59c10aa-6e7d-4d77-9130-6aff7d4abc27
📒 Files selected for processing (26)
CMakeLists.txtdocs/en/dev/passes/00-pass_manager.mddocs/zh-cn/dev/passes/00-pass_manager.mdinclude/pypto/ir/transforms/pass_properties.hinclude/pypto/ir/transforms/passes.hinclude/pypto/ir/type_inference.hpython/bindings/modules/passes.cpppython/pypto/ir/op/tensor_ops.pypython/pypto/ir/op/tile_ops.pypython/pypto/ir/pass_manager.pypython/pypto/language/__init__.pypython/pypto/language/op/__init__.pypython/pypto/language/op/tensor_ops.pypython/pypto/language/op/tile_ops.pypython/pypto/language/op/unified_ops.pypython/pypto/pypto_core/passes.pyisrc/ir/op/tensor_ops/broadcast.cppsrc/ir/op/tensor_ops/transform.cppsrc/ir/op/tile_ops/broadcast.cppsrc/ir/op/tile_ops/transform.cppsrc/ir/op/type_inference.cppsrc/ir/transforms/op_conversion_registry.cppsrc/ir/transforms/substitute_tiles_pass.cpptests/ut/ir/operators/test_tensor_ops.pytests/ut/ir/operators/test_tile_ops.pytests/ut/ir/transforms/test_substitute_tiles.py
| | OutlineIncoreScopes | TypeChecked, SSAForm | SplitIncoreOrch | — | | ||
| | OutlineClusterScopes | TypeChecked, SSAForm | ClusterOutlined | — | | ||
| | ConvertTensorToTileOps | SplitIncoreOrch | IncoreTileOps | — | | ||
| | SubstituteTiles | SSAForm, IncoreTileOps | SSAForm, IncoreTileOps | — | |
There was a problem hiding this comment.
SubstituteTiles 属性表与实现不一致。
Line 71 缺少 NormalizedStmtStructure，而代码中的 kSubstituteTilesProperties 把它列为 required/produced。建议同步文档表格字段。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/zh-cn/dev/passes/00-pass_manager.md` at line 71, The table row for
SubstituteTiles is missing NormalizedStmtStructure which causes a mismatch with
the implementation: update the table row for "SubstituteTiles" so its input and
output columns include "NormalizedStmtStructure" to match the
kSubstituteTilesProperties (which lists NormalizedStmtStructure as
required/produced); ensure the document's SSAForm, IncoreTileOps entries reflect
the same required/produced fields as the code.
Comment on lines
+605
to
+617
| def expand_clone(input: Tensor, shape: Sequence[IntLike]) -> Tensor: | ||
| """Clone and expand input to target shape. | ||
|
|
||
| Args: | ||
| input: Input tensor | ||
| shape: Target shape dimensions | ||
|
|
||
| Returns: | ||
| Tensor wrapping the expand_clone operation | ||
| """ | ||
| input_expr = input.unwrap() | ||
| call_expr = _ir_ops.expand_clone(input_expr, _normalize_intlike(shape)) | ||
| return Tensor(expr=call_expr) |
There was a problem hiding this comment.
Expose valid_shape in the public tensor expand_clone API.
At Line 605, the language wrapper only accepts shape, but the IR op supports optional valid_shape. This blocks valid dynamic-shape use cases from the DSL surface.
Proposed fix
-def expand_clone(input: Tensor, shape: Sequence[IntLike]) -> Tensor:
+def expand_clone(
+ input: Tensor,
+ shape: Sequence[IntLike],
+ *,
+ valid_shape: Sequence[IntLike] | None = None,
+) -> Tensor:
@@
- call_expr = _ir_ops.expand_clone(input_expr, _normalize_intlike(shape))
+ normalized_valid_shape = None if valid_shape is None else _normalize_intlike(valid_shape)
+ call_expr = _ir_ops.expand_clone(input_expr, _normalize_intlike(shape), normalized_valid_shape)
return Tensor(expr=call_expr)🧰 Tools
🪛 Ruff (0.15.7)
[error] 605-605: Function argument input is shadowing a Python builtin
(A002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/pypto/language/op/tensor_ops.py` around lines 605 - 617, The public
function expand_clone currently only accepts shape but the IR op
_ir_ops.expand_clone also supports an optional valid_shape; update the
expand_clone signature to accept valid_shape: Optional[Sequence[IntLike]] =
None, normalize it (e.g. via _normalize_intlike) and pass it to
_ir_ops.expand_clone alongside the normalized shape, and update the
docstring/type hints accordingly; keep existing use of input.unwrap() and return
Tensor(expr=call_expr).
Comment on lines
+298
to
+304
| def expand_clone(input: T, shape: Sequence[IntLike]) -> T: | ||
| """Clone and expand input to target shape, dispatched by input type.""" | ||
| if isinstance(input, Tensor): | ||
| return _tensor.expand_clone(input, shape) | ||
| if isinstance(input, Tile): | ||
| return _tile.expand_clone(input, shape) | ||
| raise TypeError(f"expand_clone: expected Tensor or Tile, got {type(input).__name__}") |
There was a problem hiding this comment.
Unified expand_clone should support tensor valid_shape.
At Line 298-304, pl.expand_clone(...) only accepts shape, so callers using unified ops still cannot pass tensor valid_shape even if tensor-level support is added.
Proposed fix
-def expand_clone(input: T, shape: Sequence[IntLike]) -> T:
+def expand_clone(
+ input: T,
+ shape: Sequence[IntLike],
+ valid_shape: Sequence[IntLike] | None = None,
+) -> T:
@@
- if isinstance(input, Tensor):
- return _tensor.expand_clone(input, shape)
+ if isinstance(input, Tensor):
+ return _tensor.expand_clone(input, shape, valid_shape=valid_shape)
if isinstance(input, Tile):
+ if valid_shape is not None:
+ raise TypeError("expand_clone: valid_shape is only supported for Tensor inputs")
return _tile.expand_clone(input, shape)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def expand_clone(input: T, shape: Sequence[IntLike]) -> T: | |
| """Clone and expand input to target shape, dispatched by input type.""" | |
| if isinstance(input, Tensor): | |
| return _tensor.expand_clone(input, shape) | |
| if isinstance(input, Tile): | |
| return _tile.expand_clone(input, shape) | |
| raise TypeError(f"expand_clone: expected Tensor or Tile, got {type(input).__name__}") | |
| def expand_clone( | |
| input: T, | |
| shape: Sequence[IntLike], | |
| valid_shape: Sequence[IntLike] | None = None, | |
| ) -> T: | |
| """Clone and expand input to target shape, dispatched by input type.""" | |
| if isinstance(input, Tensor): | |
| return _tensor.expand_clone(input, shape, valid_shape=valid_shape) | |
| if isinstance(input, Tile): | |
| if valid_shape is not None: | |
| raise TypeError("expand_clone: valid_shape is only supported for Tensor inputs") | |
| return _tile.expand_clone(input, shape) | |
| raise TypeError(f"expand_clone: expected Tensor or Tile, got {type(input).__name__}") |
🧰 Tools
🪛 Ruff (0.15.7)
[error] 298-298: Function argument input is shadowing a Python builtin
(A002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/pypto/language/op/unified_ops.py` around lines 298 - 304, The unified
expand_clone currently only accepts shape so callers cannot pass a tensor-level
valid_shape; update the expand_clone signature to accept an optional valid_shape
(e.g. valid_shape: Optional[Sequence[IntLike]] = None) and pass it through to
the tensor implementation (_tensor.expand_clone(input, shape, valid_shape));
keep calling _tile.expand_clone(input, shape) for Tile or forward valid_shape if
Tile supports it. Also update the Type hints and the error message path in
expand_clone to reflect the new parameter so callers can supply valid_shape when
operating on Tensors.
wuzhf9
force-pushed
the
issue679
branch
2 times, most recently
from
0ebbe63 to
8550a5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/ir/op/type_inference.cpp (1)
286-296: Consider overflow protection for extremely large shapes.The multiplication
product *= const_dim->value_has no overflow check. While unlikely for typical tensor shapes, this could silently wrap for pathological inputs. The usage inDeduceTileReshapeTypechecksproduct > 0, which won't detect overflow that wraps to a positive value.If this is a concern, consider adding overflow detection:
🔧 Optional: Add overflow detection
int64_t ComputeShapeProduct(const std::vector<ExprPtr>& shape) { int64_t product = 1; for (const auto& dim : shape) { auto const_dim = As<ConstInt>(dim); if (!const_dim) { return -1; // Dynamic shape, cannot compute product } + if (const_dim->value_ <= 0) { + return -1; // Invalid dimension + } + // Check for overflow before multiplication + if (product > std::numeric_limits<int64_t>::max() / const_dim->value_) { + return -1; // Would overflow + } product *= const_dim->value_; } return product; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ir/op/type_inference.cpp` around lines 286 - 296, ComputeShapeProduct may silently overflow when multiplying many or huge dimensions; modify ComputeShapeProduct to detect overflow before doing product *= const_dim->value_ (use INT64_MAX / const_dim->value_ check or an equivalent safe multiply) and return -1 if overflow is detected (treat overflow as dynamic/unknown) so callers like DeduceTileReshapeType that test product > 0 won't be misled; update the function that iterates over shape (ComputeShapeProduct and the use of ConstInt::value_) to perform this pre-multiply check and include <limits> as needed.src/ir/transforms/substitute_tiles_pass.cpp (1)
106-107: Consider visitingshape_exprfor consistency withtile_src.
tile_srcis visited viaVisitExpr, butshape_expris used directly. WhileExtractConstShapecurrently requires all-constant elements (making remapping moot), this could become a subtle bug ifExtractConstShapeis relaxed in the future to support symbolic dimensions. Visiting both arguments would make the code more resilient to future changes.auto tile_src = VisitExpr(call->args_[0]); - auto shape_expr = call->args_[1]; + auto shape_expr = VisitExpr(call->args_[1]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ir/transforms/substitute_tiles_pass.cpp` around lines 106 - 107, The code visits the first call argument (tile_src) with VisitExpr but uses call->args_[1] (shape_expr) directly; update the SubstituteTilesPass (in substitute_tiles_pass.cpp) to visit shape_expr via VisitExpr as well (e.g., replace direct use of call->args_[1] with VisitExpr(call->args_[1])) so both operands are normalized before calling ExtractConstShape; this keeps behavior consistent with tile_src and prevents future bugs if ExtractConstShape is extended to handle symbolic dimensions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ir/op/tensor_ops/broadcast.cpp`:
- Around line 189-190: The CHECK error message concatenates op_name directly
with "requires" causing a missing space; update the CHECK in broadcast.cpp that
references shape_tuple_type and op_name (and uses
args[1]->GetType()->TypeName()) to include a space between op_name and
"requires" (e.g., change the string to " requires shape to be TupleType, but got
") so the logged message is properly spaced.
- Around line 195-199: The error messages in broadcast.cpp concatenate op_name
and the rest of the message (e.g., op_name << "shape tuple element..."),
producing missing spaces; update the two CHECK log messages that reference
scalar_type, shape_tuple_type->types_[i]->TypeName(), and
scalar_type->dtype_.ToString() so they insert a space after op_name (e.g.,
op_name << " shape tuple element " ...) and ensure consistent spacing before the
rest of the text to produce readable messages.
- Around line 179-180: The CHECK message in src/ir/op/tensor_ops/broadcast.cpp
concatenates op_name with the following string, producing no space (e.g.,
"tensor.expand_clonerequires..."); update the CHECK call that uses op_name to
include a separating space in the error string (e.g., add a trailing space in
the literal or insert " << ' ' << ") so the output becomes "op_name requires 2
or 3 arguments..." while leaving the existing CHECK(args.size() == 2 ||
args.size() == 3) and variable names unchanged.
- Line 247: The CHECK call that emits the message concatenates op_name and the
literal without a separating space: in the CHECK(valid_shape_tuple) << op_name
<< "valid_shape (3rd argument) must be a MakeTuple"; expression, insert a space
or separator (e.g., << " " << or << ": " <<) between op_name and the message so
the logged output reads correctly; update the CHECK usage in broadcast.cpp (the
CHECK(valid_shape_tuple) line referencing op_name) accordingly.
- Around line 184-185: The error message built in the CHECK(tensor_type)
statement concatenates op_name directly to the string "requires..." with no
space; update the CHECK(tensor_type) << op_name << "requires first argument..."
expression to include a space between op_name and the rest of the message (e.g.,
insert a " " between op_name and the literal or prepend the literal with a
leading space) so the emitted message reads "<op_name> requires first argument
to be a TensorType, but got ...".
---
Nitpick comments:
In `@src/ir/op/type_inference.cpp`:
- Around line 286-296: ComputeShapeProduct may silently overflow when
multiplying many or huge dimensions; modify ComputeShapeProduct to detect
overflow before doing product *= const_dim->value_ (use INT64_MAX /
const_dim->value_ check or an equivalent safe multiply) and return -1 if
overflow is detected (treat overflow as dynamic/unknown) so callers like
DeduceTileReshapeType that test product > 0 won't be misled; update the function
that iterates over shape (ComputeShapeProduct and the use of ConstInt::value_)
to perform this pre-multiply check and include <limits> as needed.
In `@src/ir/transforms/substitute_tiles_pass.cpp`:
- Around line 106-107: The code visits the first call argument (tile_src) with
VisitExpr but uses call->args_[1] (shape_expr) directly; update the
SubstituteTilesPass (in substitute_tiles_pass.cpp) to visit shape_expr via
VisitExpr as well (e.g., replace direct use of call->args_[1] with
VisitExpr(call->args_[1])) so both operands are normalized before calling
ExtractConstShape; this keeps behavior consistent with tile_src and prevents
future bugs if ExtractConstShape is extended to handle symbolic dimensions.
🪄 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: 5feb745e-9c27-4b20-8706-9eab6aa56c4e
📒 Files selected for processing (26)
CMakeLists.txtdocs/en/dev/passes/00-pass_manager.mddocs/zh-cn/dev/passes/00-pass_manager.mdinclude/pypto/ir/transforms/pass_properties.hinclude/pypto/ir/transforms/passes.hinclude/pypto/ir/type_inference.hpython/bindings/modules/passes.cpppython/pypto/ir/op/tensor_ops.pypython/pypto/ir/op/tile_ops.pypython/pypto/ir/pass_manager.pypython/pypto/language/__init__.pypython/pypto/language/op/__init__.pypython/pypto/language/op/tensor_ops.pypython/pypto/language/op/tile_ops.pypython/pypto/language/op/unified_ops.pypython/pypto/pypto_core/passes.pyisrc/ir/op/tensor_ops/broadcast.cppsrc/ir/op/tensor_ops/transform.cppsrc/ir/op/tile_ops/broadcast.cppsrc/ir/op/tile_ops/transform.cppsrc/ir/op/type_inference.cppsrc/ir/transforms/op_conversion_registry.cppsrc/ir/transforms/substitute_tiles_pass.cpptests/ut/ir/operators/test_tensor_ops.pytests/ut/ir/operators/test_tile_ops.pytests/ut/ir/transforms/test_substitute_tiles.py
✅ Files skipped from review due to trivial changes (5)
- src/ir/transforms/op_conversion_registry.cpp
- include/pypto/ir/transforms/pass_properties.h
- tests/ut/ir/operators/test_tensor_ops.py
- include/pypto/ir/type_inference.h
- tests/ut/ir/operators/test_tile_ops.py
🚧 Files skipped from review as they are similar to previous changes (10)
- docs/en/dev/passes/00-pass_manager.md
- python/bindings/modules/passes.cpp
- CMakeLists.txt
- include/pypto/ir/transforms/passes.h
- python/pypto/pypto_core/passes.pyi
- python/pypto/language/op/tile_ops.py
- python/pypto/ir/op/tensor_ops.py
- src/ir/op/tensor_ops/transform.cpp
- src/ir/op/tile_ops/broadcast.cpp
- docs/zh-cn/dev/passes/00-pass_manager.md
Comment on lines
+184
to
+185
| CHECK(tensor_type) << op_name << "requires first argument to be a TensorType, but got " | ||
| << args[0]->GetType()->TypeName(); |
There was a problem hiding this comment.
Missing space in error message.
Same issue as above - op_name should be followed by a space.
Proposed fix
- CHECK(tensor_type) << op_name << "requires first argument to be a TensorType, but got "
+ CHECK(tensor_type) << op_name << " requires first argument to be a TensorType, but got "📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CHECK(tensor_type) << op_name << "requires first argument to be a TensorType, but got " | |
| << args[0]->GetType()->TypeName(); | |
| CHECK(tensor_type) << op_name << " requires first argument to be a TensorType, but got " | |
| << args[0]->GetType()->TypeName(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ir/op/tensor_ops/broadcast.cpp` around lines 184 - 185, The error message
built in the CHECK(tensor_type) statement concatenates op_name directly to the
string "requires..." with no space; update the CHECK(tensor_type) << op_name <<
"requires first argument..." expression to include a space between op_name and
the rest of the message (e.g., insert a " " between op_name and the literal or
prepend the literal with a leading space) so the emitted message reads
"<op_name> requires first argument to be a TensorType, but got ...".
src/ir/op/tensor_ops/broadcast.cpp
Outdated
Comment on lines
+189
to
+190
| CHECK(shape_tuple_type) << op_name << "requires shape to be TupleType, but got " | ||
| << args[1]->GetType()->TypeName(); |
There was a problem hiding this comment.
Missing space in error message.
Proposed fix
- CHECK(shape_tuple_type) << op_name << "requires shape to be TupleType, but got "
+ CHECK(shape_tuple_type) << op_name << " requires shape to be TupleType, but got "📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CHECK(shape_tuple_type) << op_name << "requires shape to be TupleType, but got " | |
| << args[1]->GetType()->TypeName(); | |
| CHECK(shape_tuple_type) << op_name << " requires shape to be TupleType, but got " | |
| << args[1]->GetType()->TypeName(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ir/op/tensor_ops/broadcast.cpp` around lines 189 - 190, The CHECK error
message concatenates op_name directly with "requires" causing a missing space;
update the CHECK in broadcast.cpp that references shape_tuple_type and op_name
(and uses args[1]->GetType()->TypeName()) to include a space between op_name and
"requires" (e.g., change the string to " requires shape to be TupleType, but got
") so the logged message is properly spaced.
src/ir/op/tensor_ops/broadcast.cpp
Outdated
Comment on lines
+195
to
+199
| CHECK(scalar_type) << op_name << "shape tuple element " << i << " must be ScalarType, but got " | ||
| << shape_tuple_type->types_[i]->TypeName(); | ||
| CHECK(scalar_type->dtype_.IsInt()) | ||
| << op_name << "shape tuple element " << i << " must have integer dtype, but got " | ||
| << scalar_type->dtype_.ToString(); |
There was a problem hiding this comment.
Missing spaces in error messages for shape tuple element validation.
Proposed fix
- CHECK(scalar_type) << op_name << "shape tuple element " << i << " must be ScalarType, but got "
+ CHECK(scalar_type) << op_name << " shape tuple element " << i << " must be ScalarType, but got "
<< shape_tuple_type->types_[i]->TypeName();
- CHECK(scalar_type->dtype_.IsInt())
- << op_name << "shape tuple element " << i << " must have integer dtype, but got "
+ CHECK(scalar_type->dtype_.IsInt())
+ << op_name << " shape tuple element " << i << " must have integer dtype, but got "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ir/op/tensor_ops/broadcast.cpp` around lines 195 - 199, The error
messages in broadcast.cpp concatenate op_name and the rest of the message (e.g.,
op_name << "shape tuple element..."), producing missing spaces; update the two
CHECK log messages that reference scalar_type,
shape_tuple_type->types_[i]->TypeName(), and scalar_type->dtype_.ToString() so
they insert a space after op_name (e.g., op_name << " shape tuple element " ...)
and ensure consistent spacing before the rest of the text to produce readable
messages.
src/ir/op/tensor_ops/broadcast.cpp
Outdated
| // If valid_shape is provided as 3rd argument, store it in TensorView | ||
| if (args.size() == 3) { | ||
| auto valid_shape_tuple = As<MakeTuple>(args[2]); | ||
| CHECK(valid_shape_tuple) << op_name << "valid_shape (3rd argument) must be a MakeTuple"; |
There was a problem hiding this comment.
Missing space in error message.
Proposed fix
- CHECK(valid_shape_tuple) << op_name << "valid_shape (3rd argument) must be a MakeTuple";
+ CHECK(valid_shape_tuple) << op_name << " valid_shape (3rd argument) must be a MakeTuple";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CHECK(valid_shape_tuple) << op_name << "valid_shape (3rd argument) must be a MakeTuple"; | |
| CHECK(valid_shape_tuple) << op_name << " valid_shape (3rd argument) must be a MakeTuple"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ir/op/tensor_ops/broadcast.cpp` at line 247, The CHECK call that emits
the message concatenates op_name and the literal without a separating space: in
the CHECK(valid_shape_tuple) << op_name << "valid_shape (3rd argument) must be a
MakeTuple"; expression, insert a space or separator (e.g., << " " << or << ": "
<<) between op_name and the message so the logged output reads correctly; update
the CHECK usage in broadcast.cpp (the CHECK(valid_shape_tuple) line referencing
op_name) accordingly.
19f8526 to
ed4a806
Compare
wuzhf9
changed the title
# Summary - Add tensor.expand_clone - Implement per-dimension broadcast behavior: - **dim0**: load once, loop over dst.size(0), store at [i, 0, 0] - **dim1**: per-row load [1,1,n], create [1,k,n], col_expand, store at [i, 0, 0] - **dim2**: load with valid_shape=[m,k,1], row_expand, store once at [0,0,0] - **no broadcast**: direct tile.store of loaded input into target - Ensure expand_clone is treated as self-loading in ConvertTensorToTileOps. - Update unit/runtime tests and expected IR to reflect the store-based semantics. # Behavior Notes - Expand clone allows at most one broadcast dimension; all other dims must match. - Broadcasting uses **tensor-level expand_clone** (no tile API), writing results into the provided target tensor. # Tests - Update IR conversion tests for expand_clone (dim0/1/2). - Update unit tests for expand_clone (dim0/1/2). - Update runtime expand_clone tests to cover dim0/1/2 via tensor.expand_clone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Related Issues
Fixes #679