-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[mlir][vector] Prevent folding of OOB values in insert/extract #135498
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
Conversation
@llvm/pr-subscribers-mlir-vector @llvm/pr-subscribers-mlir Author: Fehr Mathieu (math-fehr) ChangesOut of bound position values should not be folded in vector.extract and vector.insert operations, as only in bounds constants and -1 are valid. Full diff: https://github.com/llvm/llvm-project/pull/135498.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
index 5a3983699d5a3..0031608e2c9d5 100644
--- a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
+++ b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
@@ -1996,6 +1996,12 @@ static Value extractInsertFoldConstantOp(OpType op, AdaptorType adaptor,
std::vector<int64_t> staticPosition = op.getStaticPosition().vec();
OperandRange dynamicPosition = op.getDynamicPosition();
ArrayRef<Attribute> dynamicPositionAttr = adaptor.getDynamicPosition();
+ ArrayRef<int64_t> vectorShape;
+ if constexpr (std::is_same_v<OpType, ExtractOp>) {
+ vectorShape = op.getSourceVectorType().getShape();
+ } else if constexpr (std::is_same_v<OpType, InsertOp>) {
+ vectorShape = op.getDestVectorType().getShape();
+ }
// If the dynamic operands is empty, it is returned directly.
if (!dynamicPosition.size())
@@ -2012,9 +2018,13 @@ static Value extractInsertFoldConstantOp(OpType op, AdaptorType adaptor,
Attribute positionAttr = dynamicPositionAttr[index];
Value position = dynamicPosition[index++];
if (auto attr = mlir::dyn_cast_if_present<IntegerAttr>(positionAttr)) {
- staticPosition[i] = attr.getInt();
- opChange = true;
- continue;
+ int64_t value = attr.getInt();
+ // Do not fold if the value is out of bounds.
+ if (value >= 0 && value < vectorShape[i]) {
+ staticPosition[i] = attr.getInt();
+ opChange = true;
+ continue;
+ }
}
operands.push_back(position);
}
diff --git a/mlir/test/Dialect/Vector/canonicalize.mlir b/mlir/test/Dialect/Vector/canonicalize.mlir
index b7db8ec834be7..b0f502a0b7c36 100644
--- a/mlir/test/Dialect/Vector/canonicalize.mlir
+++ b/mlir/test/Dialect/Vector/canonicalize.mlir
@@ -3233,3 +3233,41 @@ func.func @fold_insert_constant_indices(%arg : vector<4x1xi32>) -> vector<4x1xi3
%res = vector.insert %1, %arg[%0, %0] : i32 into vector<4x1xi32>
return %res : vector<4x1xi32>
}
+
+// -----
+
+// Check that out of bounds indices are not folded for vector.insert
+
+// CHECK-LABEL: @fold_insert_oob
+// CHECK-SAME: %[[ARG:.*]]: vector<4x1x2xi32>) -> vector<4x1x2xi32> {
+// CHECK: %[[OOB1:.*]] = arith.constant -2 : index
+// CHECK: %[[OOB2:.*]] = arith.constant 2 : index
+// CHECK: %[[VAL:.*]] = arith.constant 1 : i32
+// CHECK: %[[RES:.*]] = vector.insert %[[VAL]], %[[ARG]] [0, %[[OOB1]], %[[OOB2]]] : i32 into vector<4x1x2xi32>
+// CHECK: return %[[RES]] : vector<4x1x2xi32>
+func.func @fold_insert_oob(%arg : vector<4x1x2xi32>) -> vector<4x1x2xi32> {
+ %0 = arith.constant 0 : index
+ %-2 = arith.constant -2 : index
+ %2 = arith.constant 2 : index
+ %1 = arith.constant 1 : i32
+ %res = vector.insert %1, %arg[%0, %-2, %2] : i32 into vector<4x1x2xi32>
+ return %res : vector<4x1x2xi32>
+}
+
+// -----
+
+// Check that out of bounds indices are not folded for vector.extract
+
+// CHECK-LABEL: @fold_extract_oob
+// CHECK-SAME: %[[ARG:.*]]: vector<4x1x2xi32>) -> i32 {
+// CHECK: %[[OOB1:.*]] = arith.constant -2 : index
+// CHECK: %[[OOB2:.*]] = arith.constant 2 : index
+// CHECK: %[[RES:.*]] = vector.extract %[[ARG]][0, %[[OOB1]], %[[OOB2]]] : i32 from vector<4x1x2xi32>
+// CHECK: return %[[RES]] : i32
+func.func @fold_extract_oob(%arg : vector<4x1x2xi32>) -> i32 {
+ %0 = arith.constant 0 : index
+ %-2 = arith.constant -2 : index
+ %2 = arith.constant 2 : index
+ %res = vector.extract %arg[%0, %-2, %2] : i32 from vector<4x1x2xi32>
+ return %res : i32
+}
|
Out of bound position values should not be folded in vector.extract and vector.insert operations, as only in bounds constants and -1 are valid.
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.
Nice, thanks for fixing this!
Can you also add the issue url to the PR description? |
Co-authored-by: Mehdi Amini <[email protected]>
This is a minor follow-up to llvm#135498. It ensures that operations like the following are not treated as out-of-bounds accesses and can be folded correctly: ```mlir %c_neg_1 = arith.constant -1 : index %0 = vector.insert %value_to_store, %dest[%c_neg_1] : vector<5xf32> into vector<4x5xf32>O` %1 = vector.extract %src[%c_neg_1, 0] : f32 from vector<4x5xf32> ``` In addition to adding tests for the case above, this PR also relocates the tests from llvm#135498 to be alongside existing tests for the `vector.{insert|extract}` folder, and reformats them to follow: * https://mlir.llvm.org/getting_started/TestingGuide/ For example: * The "no_fold" prefix is now used to label negative tests. * Redundant check lines have been removed (e.g., CHECK: vector.insert is sufficient to verify that folding did not occur).
This is a minor follow-up to #135498. It ensures that operations like the following are not treated as out-of-bounds accesses and can be folded correctly (*): ```mlir %c_neg_1 = arith.constant -1 : index %0 = vector.insert %value_to_store, %dest[%c_neg_1] : vector<5xf32> into vector<4x5xf32> %1 = vector.extract %src[%c_neg_1, 0] : f32 from vector<4x5xf32> ``` In addition to adding tests for the case above, this PR also relocates the tests from #135498 to be alongside existing tests for the `vector.{insert|extract}` folder, and reformats them to follow: * https://mlir.llvm.org/getting_started/TestingGuide/ For example: * The "no_fold" prefix is now used to label negative tests. * Redundant check lines have been removed (e.g., CHECK: vector.insert is sufficient to verify that folding did not occur). (*) As per https://mlir.llvm.org/docs/Dialects/Vector/#vectorinsert-vectorinsertop, these are poison values.
…135498) Out of bound position values should not be folded in vector.extract and vector.insert operations, as only in bounds constants and -1 are valid. Fixes llvm#134516
…6579) This is a minor follow-up to llvm#135498. It ensures that operations like the following are not treated as out-of-bounds accesses and can be folded correctly (*): ```mlir %c_neg_1 = arith.constant -1 : index %0 = vector.insert %value_to_store, %dest[%c_neg_1] : vector<5xf32> into vector<4x5xf32> %1 = vector.extract %src[%c_neg_1, 0] : f32 from vector<4x5xf32> ``` In addition to adding tests for the case above, this PR also relocates the tests from llvm#135498 to be alongside existing tests for the `vector.{insert|extract}` folder, and reformats them to follow: * https://mlir.llvm.org/getting_started/TestingGuide/ For example: * The "no_fold" prefix is now used to label negative tests. * Redundant check lines have been removed (e.g., CHECK: vector.insert is sufficient to verify that folding did not occur). (*) As per https://mlir.llvm.org/docs/Dialects/Vector/#vectorinsert-vectorinsertop, these are poison values.
…6579) This is a minor follow-up to llvm#135498. It ensures that operations like the following are not treated as out-of-bounds accesses and can be folded correctly (*): ```mlir %c_neg_1 = arith.constant -1 : index %0 = vector.insert %value_to_store, %dest[%c_neg_1] : vector<5xf32> into vector<4x5xf32> %1 = vector.extract %src[%c_neg_1, 0] : f32 from vector<4x5xf32> ``` In addition to adding tests for the case above, this PR also relocates the tests from llvm#135498 to be alongside existing tests for the `vector.{insert|extract}` folder, and reformats them to follow: * https://mlir.llvm.org/getting_started/TestingGuide/ For example: * The "no_fold" prefix is now used to label negative tests. * Redundant check lines have been removed (e.g., CHECK: vector.insert is sufficient to verify that folding did not occur). (*) As per https://mlir.llvm.org/docs/Dialects/Vector/#vectorinsert-vectorinsertop, these are poison values.
…135498) Out of bound position values should not be folded in vector.extract and vector.insert operations, as only in bounds constants and -1 are valid. Fixes llvm#134516
…6579) This is a minor follow-up to llvm#135498. It ensures that operations like the following are not treated as out-of-bounds accesses and can be folded correctly (*): ```mlir %c_neg_1 = arith.constant -1 : index %0 = vector.insert %value_to_store, %dest[%c_neg_1] : vector<5xf32> into vector<4x5xf32> %1 = vector.extract %src[%c_neg_1, 0] : f32 from vector<4x5xf32> ``` In addition to adding tests for the case above, this PR also relocates the tests from llvm#135498 to be alongside existing tests for the `vector.{insert|extract}` folder, and reformats them to follow: * https://mlir.llvm.org/getting_started/TestingGuide/ For example: * The "no_fold" prefix is now used to label negative tests. * Redundant check lines have been removed (e.g., CHECK: vector.insert is sufficient to verify that folding did not occur). (*) As per https://mlir.llvm.org/docs/Dialects/Vector/#vectorinsert-vectorinsertop, these are poison values.
…6579) This is a minor follow-up to llvm#135498. It ensures that operations like the following are not treated as out-of-bounds accesses and can be folded correctly (*): ```mlir %c_neg_1 = arith.constant -1 : index %0 = vector.insert %value_to_store, %dest[%c_neg_1] : vector<5xf32> into vector<4x5xf32> %1 = vector.extract %src[%c_neg_1, 0] : f32 from vector<4x5xf32> ``` In addition to adding tests for the case above, this PR also relocates the tests from llvm#135498 to be alongside existing tests for the `vector.{insert|extract}` folder, and reformats them to follow: * https://mlir.llvm.org/getting_started/TestingGuide/ For example: * The "no_fold" prefix is now used to label negative tests. * Redundant check lines have been removed (e.g., CHECK: vector.insert is sufficient to verify that folding did not occur). (*) As per https://mlir.llvm.org/docs/Dialects/Vector/#vectorinsert-vectorinsertop, these are poison values.
Out of bound position values should not be folded in vector.extract and vector.insert operations, as only in bounds constants and -1 are valid.
Fixes #134516