Skip to content

Commit 60010b3

Browse files
authored
[mlir][linalg] Clarify comments and remove outdated TODO (nfc) (#171695)
The TODO being removed was originally added at my request in: * #160246 Upon revisiting the issue, it becomes clear that implementing the TODO would require supporting IR of the following form (note the static tile size `8` and the corresponding dynamic dimension in the result type): ```mlir %pack = linalg.pack %src padding_value(%c5 : i32) inner_dims_pos = [0, 1] inner_tiles = [8, 1] into %dest : tensor<?x?xi32> -> tensor<1x1x?x1xi32> ``` Allowing this would be invalid, as discussed here: * https://discourse.llvm.org/t/tensor-ops-with-dynamic-sizes-which-behaviour-is-more-correct
1 parent 5123d36 commit 60010b3

File tree

2 files changed

+13
-7
lines changed

2 files changed

+13
-7
lines changed

mlir/include/mlir/Dialect/Linalg/IR/LinalgRelayoutOps.td

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,14 @@ class Linalg_RelayoutOp<string mnemonic, list<Trait> traits = []> :
5858
/// tile factors.
5959
DenseMap<int64_t, OpFoldResult> getDimAndTileMapping();
6060

61-
// TODO: Return the folded result.
62-
/// Return the tile sizes as OpFoldResult. Will return the Value
63-
/// of the constant Op, not the constant Attribute.
64-
/// E.g., for %size = arith.constant 1 : i32 will return %size,
65-
/// not 1.
61+
/// Return the tile sizes as OpFoldResult(s). Note, for Ops that simply
62+
/// define constants (e.g. `arith.constant`), this method returns
63+
/// Value/result of the Op (as opposed to the corresponding constant
64+
/// Attribute). E.g., for:
65+
/// %size = arith.constant 1 : i32
66+
/// it will return %size, not 1. This is intended - replacing an SSA value
67+
/// with a constant attribute would require updating the corresponding dim
68+
/// from dynamic to static.
6669
SmallVector<OpFoldResult> getMixedTiles();
6770

6871
/// Return the tile sizes as `int64_t`. If a tile size is dynamic

mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1219,8 +1219,11 @@ LogicalResult DecomposeOuterUnitDimsPackOpPattern::matchAndRewrite(
12191219
}
12201220
shapeForEmptyOp.append(packOp.getMixedTiles());
12211221

1222-
// getMixedTiles() may contain Values pointing to constant ops, not the
1223-
// constant attributes. Replace them with a true OpFoldResult.
1222+
// getMixedTiles() may contain Values pointing to constant ops (as opposed to
1223+
// constant attributes with the corresponding value). Replace those with
1224+
// attributes. This is to match the behaviour in
1225+
// `getPackOpSourceOrPaddedSource`, which replaces constant SSA values with
1226+
// attributes.
12241227
llvm::transform(shapeForEmptyOp, shapeForEmptyOp.begin(),
12251228
[&](OpFoldResult ofr) {
12261229
if (auto val = llvm::dyn_cast<Value>(ofr))

0 commit comments

Comments
 (0)