Skip to content

Conversation

@dcaballe
Copy link
Contributor

This PR moves extractInsertFoldConstantOp earlier in the folder lists of vector.extract and vector.insert. Many folders require having non-dynamic indices so extractInsertFoldConstantOp is a requirement for them to trigger.

This PR moves `extractInsertFoldConstantOp` earlier in the folder lists of
`vector.extract` and `vector.insert`. Many folders require having non-dynamic
indices so `extractInsertFoldConstantOp` is a requirement for them to trigger.
@llvmbot
Copy link
Member

llvmbot commented May 20, 2025

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-vector

Author: Diego Caballero (dcaballe)

Changes

This PR moves extractInsertFoldConstantOp earlier in the folder lists of vector.extract and vector.insert. Many folders require having non-dynamic indices so extractInsertFoldConstantOp is a requirement for them to trigger.


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

1 Files Affected:

  • (modified) mlir/lib/Dialect/Vector/IR/VectorOps.cpp (+9-5)
diff --git a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
index bbb366b01fa6e..cf2df1f24f91f 100644
--- a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
+++ b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
@@ -2143,11 +2143,16 @@ OpFoldResult ExtractOp::fold(FoldAdaptor adaptor) {
   // mismatch).
   if (getNumIndices() == 0 && getVector().getType() == getResult().getType())
     return getVector();
+  if (auto res = foldPoisonSrcExtractOp(adaptor.getVector()))
+    return res;
+  // Fold `arith.constant` indices into the `vector.extract` operation. Make
+  // sure that patterns requiring constant indices are added after this fold.
+  SmallVector<Value> operands = {getVector()};
+  if (auto val = extractInsertFoldConstantOp(*this, adaptor, operands))
+    return val;
   if (auto res = foldPoisonIndexInsertExtractOp(
           getContext(), adaptor.getStaticPosition(), kPoisonIndex))
     return res;
-  if (auto res = foldPoisonSrcExtractOp(adaptor.getVector()))
-    return res;
   if (auto res = foldDenseElementsAttrSrcExtractOp(*this, adaptor.getVector()))
     return res;
   if (succeeded(foldExtractOpFromExtractChain(*this)))
@@ -2166,9 +2171,6 @@ OpFoldResult ExtractOp::fold(FoldAdaptor adaptor) {
     return val;
   if (auto val = foldScalarExtractFromFromElements(*this))
     return val;
-  SmallVector<Value> operands = {getVector()};
-  if (auto val = extractInsertFoldConstantOp(*this, adaptor, operands))
-    return val;
   return OpFoldResult();
 }
 
@@ -3145,6 +3147,8 @@ OpFoldResult vector::InsertOp::fold(FoldAdaptor adaptor) {
   // (type mismatch).
   if (getNumIndices() == 0 && getValueToStoreType() == getType())
     return getValueToStore();
+  // Fold `arith.constant` indices into the `vector.insert` operation. Make
+  // sure that patterns requiring constant indices are added after this fold.
   SmallVector<Value> operands = {getValueToStore(), getDest()};
   if (auto val = extractInsertFoldConstantOp(*this, adaptor, operands))
     return val;

@dcaballe dcaballe requested a review from linuxlonelyeagle May 20, 2025 22:55
@dcaballe dcaballe merged commit fd8bc37 into llvm:main May 21, 2025
15 checks passed
@banach-space
Copy link
Contributor

Many folders require having non-dynamic indices so extractInsertFoldConstantOp is a requirement for them to trigger

To me this implies this is a non-nfc and that we should be able to test it.

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.

5 participants