-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[mlir][linalg] Restrict linalg.pack to not have artificial padding. #149624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,7 +106,9 @@ def Linalg_PackOp : Linalg_RelayoutOp<"pack", [ | |
result tensor in the order in which they appear, i.e. | ||
`shape(result)[rank(result) + i] = inner_tiles[i]` for `0 <= i < k`. | ||
- The following relationship for the tiled dimensions holds: | ||
`shape(result)[inner_dims_pos[i]] = shape(source)[inner_dims_pos[i]] / inner_tiles[i]`. | ||
`shape(result)[inner_dims_pos[i]] = shape(source)[inner_dims_pos[i]] / inner_tiles[i]`, | ||
where (⌈/⌉ indicates CeilDiv). | ||
|
||
|
||
Example: If `inner_tiles = [16, 32]`, the result tensor has a shape of | ||
`...x16x32`. If `inner_dims_pos = [0, 1]`, the 0th source dimension is tiled | ||
|
@@ -150,9 +152,17 @@ def Linalg_PackOp : Linalg_RelayoutOp<"pack", [ | |
|
||
`padding_value` specifies a padding value at the boundary on non-perfectly | ||
divisible dimensions. Padding is optional: | ||
- If absent, it is UB if the tile does not perfectly divide the dimension. | ||
- If absent, it is assumed that for all inner tiles, | ||
`shape(source)[inner_dims_pos[i]] % inner_tiles[i] == 0`, i.e. all inner | ||
tiles divide perfectly the corresponding outer dimension in the result | ||
tensor. It is UB if the tile does not perfectly divide the dimension. | ||
- If present, it will pad along high dimensions (high-padding) to make the | ||
tile complete. | ||
tile complete. Note that it is not allowed to have artificial padding that | ||
is not strictly required by linalg.pack (i.e., padding past what is needed | ||
to complete the last tile along each packed dimension). It is UB if extra | ||
padding is requested. | ||
Comment on lines
+162
to
+163
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Shouldn't that be verification error? And then restore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's not possible to enforce that with dynamic source.
It could remain there to reinforce the message. But no strong preference here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enforcing that is more like a dynamic check, but not a static check. I.e., it can only be asserted during runtime for most cases, IMO. E.g., you don't do any out-of-bound checks for tensor.extract_slice for dynamic sizes/dims, but you do out-of-bound checks when the op has static sizes and offsets. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restored the UB for the previous point, because it also happens in dynamic shape that I forgot. I also added one more sentence about why they are UB. Does it look better? |
||
It is not possible to verify the requirements statically with dynamic | ||
shapes, so they are treated as UB. | ||
|
||
Example: | ||
```mlir | ||
|
@@ -167,6 +177,15 @@ def Linalg_PackOp : Linalg_RelayoutOp<"pack", [ | |
// | ||
// Note: Only tiled dimensions can be padded. | ||
``` | ||
|
||
Invalid example that has artificial padding: | ||
```mlir | ||
%0 = linalg.pack %src padding_value(%cst : f32) inner_dims_pos = [0] | ||
inner_tiles = [8] into %dest | ||
: tensor<9xf32> -> tensor<3x8xf32> | ||
// \ | ||
// expect tensor<2x8xf32> because CeilDiv(9, 8) = 2 | ||
``` | ||
}]; | ||
let arguments = (ins AnyRankedTensor:$source, | ||
AnyRankedTensor:$dest, | ||
|
hanhanW marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also update:
as
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, updated. Thanks for pointing it out!