Skip to content

[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

Merged
merged 3 commits into from
Apr 18, 2025

Conversation

math-fehr
Copy link
Contributor

@math-fehr math-fehr commented Apr 12, 2025

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

@llvmbot
Copy link
Member

llvmbot commented Apr 12, 2025

@llvm/pr-subscribers-mlir-vector

@llvm/pr-subscribers-mlir

Author: Fehr Mathieu (math-fehr)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/135498.diff

2 Files Affected:

  • (modified) mlir/lib/Dialect/Vector/IR/VectorOps.cpp (+13-3)
  • (modified) mlir/test/Dialect/Vector/canonicalize.mlir (+38)
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.
Copy link
Member

@kuhar kuhar left a 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!

@kuhar
Copy link
Member

kuhar commented Apr 17, 2025

Can you also add the issue url to the PR description?

@math-fehr math-fehr merged commit 2721f5a into llvm:main Apr 18, 2025
11 checks passed
@math-fehr math-fehr deleted the math-fehr/vector-insert-oob branch April 18, 2025 03:53
banach-space added a commit to banach-space/llvm-project that referenced this pull request Apr 21, 2025
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).
banach-space added a commit that referenced this pull request Apr 24, 2025
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.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…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
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…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.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…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.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…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
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…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.
Ankur-0429 pushed a commit to Ankur-0429/llvm-project that referenced this pull request May 9, 2025
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mlir][vector] Error when running --canonicalize with out of bounds positions in vector.extract
4 participants