[mlir][vector] Always print the in_bounds attribute #96031
Conversation
This patch updates the printer for vector ops attributes so that the
`in_bounds` attribute, when present, is always printed (regardless of the
contents).
ATM, an attribute with all values equal `false`, e.g. `in_bounds =
{false, false}`, wouldn't be printed. This makes testing certain
behaviours impossible (i.e. to make sure that the attribute is correctly
preserved/transformed by patterns that modify it). See e.g. llvm#12345
Separately, it's not clear whether the absence of the `in_bounds`
attribute in the printed IR is meant to mean that:
* the attribute is absent,
* the attribute is present and set to all-true,
* the attribute is present and set to all-false.
By making sure that the attribute is always present, we are removing
this ambiguity.
banach-space
requested review from
dcaballe,
matthias-springer and
nicolasvasilache
as code owners
|
@llvm/pr-subscribers-mlir-linalg @llvm/pr-subscribers-mlir Author: Andrzej Warzyński (banach-space) ChangesThis patch updates the printer for vector ops attributes so that the ATM, an attribute with all values equal Separately, it's not clear whether the absence of the
By making sure that the attribute is always present, we are removing Full diff: https://github.com/llvm/llvm-project/pull/96031.diff 4 Files Affected:
diff --git a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
index 2bf4f16f96e6a..d6855d59204cd 100644
--- a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
+++ b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
@@ -3859,9 +3859,6 @@ static void printTransferAttrs(OpAsmPrinter &p, VectorTransferOpInterface op) {
elidedAttrs.push_back(TransferReadOp::getOperandSegmentSizeAttr());
if (op.getPermutationMap().isMinorIdentity())
elidedAttrs.push_back(op.getPermutationMapAttrName());
- // Elide in_bounds attribute if all dims are out-of-bounds.
- if (llvm::none_of(op.getInBoundsValues(), [](bool b) { return b; }))
- elidedAttrs.push_back(op.getInBoundsAttrName());
p.printOptionalAttrDict(op->getAttrs(), elidedAttrs);
}
diff --git a/mlir/test/Conversion/VectorToSCF/vector-to-scf.mlir b/mlir/test/Conversion/VectorToSCF/vector-to-scf.mlir
index e1babdd2f1f63..ea57e2afbaa2b 100644
--- a/mlir/test/Conversion/VectorToSCF/vector-to-scf.mlir
+++ b/mlir/test/Conversion/VectorToSCF/vector-to-scf.mlir
@@ -174,7 +174,7 @@ func.func @materialize_write(%M: index, %N: index, %O: index, %P: index) {
// CHECK: scf.for %[[I6:.*]] = %[[C0]] to %[[C1]] step %[[C1]] {
// CHECK: %[[S0:.*]] = affine.apply #[[$ADD]](%[[I2]], %[[I6]])
// CHECK: %[[VEC:.*]] = memref.load %[[VECTOR_VIEW3]][%[[I4]], %[[I5]], %[[I6]]] : memref<3x4x1xvector<5xf32>>
- // CHECK: vector.transfer_write %[[VEC]], %{{.*}}[%[[S3]], %[[S1]], %[[S0]], %[[I3]]] : vector<5xf32>, memref<?x?x?x?xf32>
+ // CHECK: vector.transfer_write %[[VEC]], %{{.*}}[%[[S3]], %[[S1]], %[[S0]], %[[I3]]] {in_bounds = [false]} : vector<5xf32>, memref<?x?x?x?xf32>
// CHECK: }
// CHECK: }
// CHECK: }
diff --git a/mlir/test/Dialect/Linalg/vectorization-with-patterns.mlir b/mlir/test/Dialect/Linalg/vectorization-with-patterns.mlir
index d7ff1ded9d933..7176dff9bc857 100644
--- a/mlir/test/Dialect/Linalg/vectorization-with-patterns.mlir
+++ b/mlir/test/Dialect/Linalg/vectorization-with-patterns.mlir
@@ -950,7 +950,7 @@ module attributes {transform.with_named_sequence} {
// CHECK-NOT: tensor.pad
// CHECK-DAG: %[[C0:.*]] = arith.constant 0 : index
// CHECK-DAG: %[[C5:.*]] = arith.constant 5.0
-// CHECK: %[[RESULT:.*]] = vector.transfer_read %[[ARG0]][%[[C0]], %[[C0]]], %[[C5]] : tensor<5x6xf32>, vector<7x9xf32>
+// CHECK: %[[RESULT:.*]] = vector.transfer_read %[[ARG0]][%[[C0]], %[[C0]]], %[[C5]] {in_bounds = [false, false]} : tensor<5x6xf32>, vector<7x9xf32>
// CHECK: return %[[RESULT]]
func.func @pad_and_transfer_read(%arg0: tensor<5x6xf32>) -> vector<7x9xf32> {
%c0 = arith.constant 0 : index
@@ -984,7 +984,7 @@ func.func private @make_vector() -> vector<7x9xf32>
// CHECK-NOT: tensor.pad
// CHECK: %[[C0:.*]] = arith.constant 0 : index
// CHECK: %[[VEC0:.*]] = call @make_vector() : () -> vector<7x9xf32>
-// CHECK: %[[RESULT:.*]] = vector.transfer_write %[[VEC0]], %[[ARG0]][%[[C0]], %[[C0]]] : vector<7x9xf32>, tensor<5x6xf32>
+// CHECK: %[[RESULT:.*]] = vector.transfer_write %[[VEC0]], %[[ARG0]][%[[C0]], %[[C0]]] {in_bounds = [false, false]} : vector<7x9xf32>, tensor<5x6xf32>
// CHECK: return %[[RESULT]]
func.func @pad_and_transfer_write_static(
%arg0: tensor<5x6xf32>) -> tensor<5x6xf32> {
@@ -1021,7 +1021,7 @@ func.func private @make_vector() -> vector<7x9xf32>
// CHECK: %[[C0:.*]] = arith.constant 0 : index
// CHECK: %[[SUB:.*]] = tensor.extract_slice %[[ARG0]][0, 0] [%[[SIZE]], 6] [1, 1] : tensor<?x?xf32> to tensor<?x6xf32>
// CHECK: %[[VEC0:.*]] = call @make_vector() : () -> vector<7x9xf32>
-// CHECK: %[[RESULT:.*]] = vector.transfer_write %[[VEC0]], %[[SUB]][%[[C0]], %[[C0]]] : vector<7x9xf32>, tensor<?x6xf32>
+// CHECK: %[[RESULT:.*]] = vector.transfer_write %[[VEC0]], %[[SUB]][%[[C0]], %[[C0]]] {in_bounds = [false, false]} : vector<7x9xf32>, tensor<?x6xf32>
// CHECK: return %[[RESULT]]
func.func @pad_and_transfer_write_dynamic_static(
%arg0: tensor<?x?xf32>, %size: index, %padding: index) -> tensor<?x6xf32> {
diff --git a/mlir/test/Dialect/Vector/ops.mlir b/mlir/test/Dialect/Vector/ops.mlir
index c868c881d079a..afe62a2427fb0 100644
--- a/mlir/test/Dialect/Vector/ops.mlir
+++ b/mlir/test/Dialect/Vector/ops.mlir
@@ -78,7 +78,7 @@ func.func @vector_transfer_ops(%arg0: memref<?x?xf32>,
vector.transfer_write %1, %arg0[%c3, %c3] {permutation_map = affine_map<(d0, d1)->(d1, d0)>} : vector<3x7xf32>, memref<?x?xf32>
// CHECK: vector.transfer_write %{{.*}}, %{{.*}}[%[[C3]], %[[C3]]] : vector<1x1x4x3xf32>, memref<?x?xvector<4x3xf32>>
vector.transfer_write %4, %arg1[%c3, %c3] {permutation_map = affine_map<(d0, d1)->(d0, d1)>} : vector<1x1x4x3xf32>, memref<?x?xvector<4x3xf32>>
- // CHECK: vector.transfer_write %{{.*}}, %{{.*}}[%[[C3]], %[[C3]]] : vector<1x1x4x3xf32>, memref<?x?xvector<4x3xf32>>
+ // CHECK: vector.transfer_write %{{.*}}, %{{.*}}[%[[C3]], %[[C3]]] {in_bounds = [false, false]} : vector<1x1x4x3xf32>, memref<?x?xvector<4x3xf32>>
vector.transfer_write %5, %arg1[%c3, %c3] {in_bounds = [false, false]} : vector<1x1x4x3xf32>, memref<?x?xvector<4x3xf32>>
// CHECK: vector.transfer_write %{{.*}}, %{{.*}}[%[[C3]], %[[C3]]] : vector<5x24xi8>, memref<?x?xvector<4x3xi32>>
vector.transfer_write %6, %arg2[%c3, %c3] : vector<5x24xi8>, memref<?x?xvector<4x3xi32>>
@@ -135,7 +135,7 @@ func.func @vector_transfer_ops_tensor(%arg0: tensor<?x?xf32>,
%9 = vector.transfer_write %1, %arg0[%c3, %c3] {permutation_map = affine_map<(d0, d1)->(d1, d0)>} : vector<3x7xf32>, tensor<?x?xf32>
// CHECK: vector.transfer_write %{{.*}}, %{{.*}}[%[[C3]], %[[C3]]] : vector<1x1x4x3xf32>, tensor<?x?xvector<4x3xf32>>
%10 = vector.transfer_write %4, %arg1[%c3, %c3] {permutation_map = affine_map<(d0, d1)->(d0, d1)>} : vector<1x1x4x3xf32>, tensor<?x?xvector<4x3xf32>>
- // CHECK: vector.transfer_write %{{.*}}, %{{.*}}[%[[C3]], %[[C3]]] : vector<1x1x4x3xf32>, tensor<?x?xvector<4x3xf32>>
+ // CHECK: vector.transfer_write %{{.*}}, %{{.*}}[%[[C3]], %[[C3]]] {in_bounds = [false, false]} : vector<1x1x4x3xf32>, tensor<?x?xvector<4x3xf32>>
%11 = vector.transfer_write %5, %arg1[%c3, %c3] {in_bounds = [false, false]} : vector<1x1x4x3xf32>, tensor<?x?xvector<4x3xf32>>
// CHECK: vector.transfer_write %{{.*}}, %{{.*}}[%[[C3]], %[[C3]]] : vector<5x24xi8>, tensor<?x?xvector<4x3xi32>>
%12 = vector.transfer_write %6, %arg2[%c3, %c3] : vector<5x24xi8>, tensor<?x?xvector<4x3xi32>>
|
Adds more tests to:
* vector-transfer-permutation-lowering.mlir
Specifically, adds tests for:
* out-of-bounds access for the `TransferWritePermutationLowering`
pattern
* in-bounds access for `TransferWriteNonPermutationLowering` +
`TransferWritePermutationLowering`
Also renames `@permutation_with_mask_xfer_write_fixed_width` as
`@xfer_write_non_transposing_permutation_map`.
This is a part of a larger effort to make sure that all key cases for
patterns under populateVectorTransferPermutationMapLoweringPatterns
(*) are tested. I also want to make sure that tests use consistent
function and variable names.
(*) transform.apply_patterns.vector.transfer_permutation_patterns in
TD parlance)
Depends on llvm#96031
|
+1 on removing the ambiguity. Do we also update the examples in op descriptions? |
Not sure, it's an optional attribute:
With this change, |
|
I think it would be better if we make the attribute mandatory with default values set to false. I believe that's the way we've been interpreting the lack of attribute. I would still avoid printing the default attribute to make the op less verbose since this is ops are already hard to read. |
|
Ultimately, we talked about removing this attribute in favor of simplifying this op and use masking instead but... that hasn't happened yet... |
I am afraid that both would be more intrusive with various implications. I can look into this later, but for now I just want to be able to add more tests in #96033 :) I feel that this change is already an improvement 😅
Right, but we need to agree what the default should be :) IIUC, right now the default is "no attribute", which is treated as "in bounds". |
There was a problem hiding this comment.
I think always displaying it is fine 👍 -- though I've never really understood the utility of the in_bounds attribute. It's mostly just passed around (and not really used). There's MaterializeTransferMask which simply replaces out-of-bounds transfer_reads/writes with in-bound ones with a mask, which makes it seem like it mostly duplicates the functionality of a mask, so I question if it's worth the extra complexity.
(removing the attribute is obviously out of scope for this PR though)
|
I see, the attribute is optional. I think always displaying it when it presents is an improvement. Because it is not trivial to distinguish the case when it is not present.
Since that there are no active changes about it, I'm +1 on always printing the values. |
I didn't quite get this part?
This is a design problem: optional attributes shouldn't mean the same thing as a "default valued" one. Here a possible fix to remove the ambiguity could also be to make it non-optional (but default valued). |
Take this example: func.func @xfer_write_transposing_permutation_map_out_of_bounds(
%arg0: vector<4x8xi16>,
%mem: memref<2x2x?x?xi16>) {
%c0 = arith.constant 0 : index
vector.transfer_write %arg0, %mem[%c0, %c0, %c0, %c0] {
in_bounds = [false, false],
permutation_map = affine_map<(d0, d1, d2, d3) -> (d3, d2)>
} : vector<4x8xi16>, memref<2x2x?x?xi16>
return
}Under func.func @xfer_write_transposing_permutation_map_out_of_bounds(%arg0: vector<4x8xi16>, %arg1: memref<2x2x?x?xi16>) {
%c0 = arith.constant 0 : index
%0 = vector.transpose %arg0, [1, 0] : vector<4x8xi16> to vector<8x4xi16>
vector.transfer_write %0, %arg1[%c0, %c0, %c0, %c0] : vector<8x4xi16>, memref<2x2x?x?xi16>
return
}The transformation is safe, but we can't verify that
This would also be an improvement. I'm happy to take this route if that's the preference. I'd do it in a follow-up PR though (if that's OK) |
Just a note that the semantics are preserved even if the attribute is not, as both the lack of an attribute and an all-false attribute have the same meaning. llvm-project/mlir/include/mlir/Interfaces/VectorInterfaces.td Lines 243 to 246 in 4145ad2 |
I don't think it does, because how is it just different from what follows:
Which seems like the only ambiguity really, which is stemming I believe from having this attribute considered both optional and having a default value! (pick one and you don't have any ambiguity). |
I think this is a case where implementation "convenience" made its way to the representation itself: IIRC, xfer ops are created without any
Really? Wow, that's really wrong! We can't assume that accesses are in-bounds by default. That would lead to correctness issues... Do you have a pointer to code where this assumption is made?
I'm not sure since that's making the issue even more explicit. I don't see a way to make this better that by making the attribute mandatory. With the printing change we might be able to test something a bit better but the underlying situation is concerning. Perhaps a good course of action would be to: 1) fix no-attribute == in-bounds assumption (this would let you test what you want without making the default printing explicit) and, 2) make the attribute mandatory, with default value set to Would that work? |
This is not the case, as shown in the snippet linked in #96031 (comment), no in-bounds attributes is interpreted as the dim being out-of-bounds. |
Ben is right and I was wrong - thanks for pointing it out and apologies for the confusion! (I was looking at something that made me believe that what I said was correct, but after double-checking I see that i was mistaken) 🙏🏻 |
As per Ben's comment, I was wrong and this doesn't require fixing. I'm still unable to test what I want, but I can work around it.
Note, this will require updating a lot of tests. I've prepared a draft PR in which I refactored just one of the affected tests (to illustrate): There's 159 test files that use Vector xfer Ops, so I'll need to update a fair bit. This is also likely to have quite an impact on downstream. CC @hanhanW |
@joker-eph 👍🏻 This will make the attribute mandatory: #97049 |
|
Closing in favour of: #97049 (already merged) |
Adds more tests to:
* vector-transfer-permutation-lowering.mlir
Specifically, adds tests for:
* out-of-bounds access for the `TransferWritePermutationLowering`
pattern
* in-bounds access for `TransferWriteNonPermutationLowering` +
`TransferWritePermutationLowering`
Also renames `@permutation_with_mask_xfer_write_fixed_width` as
`@xfer_write_non_transposing_permutation_map`.
This is a part of a larger effort to make sure that all key cases for
patterns under populateVectorTransferPermutationMapLoweringPatterns
(*) are tested. I also want to make sure that tests use consistent
function and variable names.
(*) transform.apply_patterns.vector.transfer_permutation_patterns in
TD parlance)
Depends on llvm#96031
This patch updates the printer for vector ops attributes so that the
in_boundsattribute, when present, is always printed (regardless of thecontents).
ATM, an attribute with all values equal
false, e.g.in_bounds = {false, false},wouldn't be printed. This makes testing certain behaviours impossible
(i.e. to make sure that the attribute is correctly preserved/transformed
by patterns that modify it). See e.g. #96033.
Separately, it's not clear whether the absence of the
in_boundsattribute in the printed IR is meant to mean that:
By making sure that the attribute is always present, we are removing
this ambiguity.