Skip to content

Conversation

amd-eochoalo
Copy link
Contributor

@amd-eochoalo amd-eochoalo commented Sep 16, 2025

This PR moves the patterns that unroll vector.to_elements and vector.from_elements into the file with other vector unrolling operations. This PR also adds these unrolling patterns into the populateVectorUnrollPatterns. And renames populateVectorToElementsLoweringPatterns populateVectorFromElementsLoweringPatterns to populateVectorToElementsUnrollPatterns populateVectorFromElementsUnrollPatterns.

@amd-eochoalo amd-eochoalo changed the title [mlir] Add vector.{to_elements,from_elements} unrolling to VectorToSPIRV [mlir] Add vector.{to_elements,from_elements} unrolling to test-spirv-vector-unrolling Sep 16, 2025
@amd-eochoalo amd-eochoalo changed the title [mlir] Add vector.{to_elements,from_elements} unrolling to test-spirv-vector-unrolling [mlir] Add vector.{to_elements,from_elements} unrolling to unrollVectorsInFuncBodies Sep 16, 2025
@amd-eochoalo amd-eochoalo marked this pull request as ready for review September 16, 2025 16:14
@llvmbot
Copy link
Member

llvmbot commented Sep 16, 2025

@llvm/pr-subscribers-mlir-vector

@llvm/pr-subscribers-mlir-spirv

Author: Erick Ochoa Lopez (amd-eochoalo)

Changes

This patch adds the unrolling patterns for vector.to_elements and vector.from_elements to the action unrollVectorsInFuncBodies. This affects the passes test-convert-to-spriv (when running with option run-vector-unrolling=true ) and test-spirv-vector-unrolling.


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

2 Files Affected:

  • (modified) mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp (+2)
  • (modified) mlir/test/Conversion/ConvertToSPIRV/vector-unroll.mlir (+44)
diff --git a/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp b/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
index 49f4ce8de7c76..98e294b40456f 100644
--- a/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
+++ b/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
@@ -1495,6 +1495,8 @@ LogicalResult mlir::spirv::unrollVectorsInFuncBodies(Operation *op) {
     RewritePatternSet patterns(context);
     auto options = vector::UnrollVectorOptions().setNativeShapeFn(
         [](auto op) { return mlir::spirv::getNativeVectorShape(op); });
+    vector::populateVectorFromElementsLoweringPatterns(patterns);
+    vector::populateVectorToElementsLoweringPatterns(patterns);
     populateVectorUnrollPatterns(patterns, options);
     if (failed(applyPatternsGreedily(op, std::move(patterns))))
       return failure();
diff --git a/mlir/test/Conversion/ConvertToSPIRV/vector-unroll.mlir b/mlir/test/Conversion/ConvertToSPIRV/vector-unroll.mlir
index c85f4334ff2e5..0957f67690b97 100644
--- a/mlir/test/Conversion/ConvertToSPIRV/vector-unroll.mlir
+++ b/mlir/test/Conversion/ConvertToSPIRV/vector-unroll.mlir
@@ -96,3 +96,47 @@ func.func @transpose(%arg0 : vector<2x3xi32>) -> (vector<3x2xi32>) {
   %0 = vector.transpose %arg0, [1, 0] : vector<2x3xi32> to vector<3x2xi32>
   return %0 : vector<3x2xi32>
 }
+
+// -----
+
+// In order to verify that the pattern is applied,
+// we need to make sure that the the 2d vector does not
+// come from the parameters. Otherwise, the pattern
+// in unrollVectorsInSignatures which splits the 2d vector
+// parameter will take precedent. Similarly, let's avoid
+// returning a vector as another pattern would take precendence.
+
+// CHECK-LABEL: @unroll_to_elements_2d
+func.func @unroll_to_elements_2d() -> (f32, f32, f32, f32) {
+  %1 = "test.op"() : () -> (vector<2x2xf32>)
+  // CHECK: %[[VEC2D:.+]] = "test.op"
+  // CHECK: %[[VEC0:.+]] = vector.extract %[[VEC2D]][0] : vector<2xf32> from vector<2x2xf32>
+  // CHECK: %[[VEC1:.+]] = vector.extract %[[VEC2D]][1] : vector<2xf32> from vector<2x2xf32>
+  // CHECK: %[[RES0:.+]]:2 = vector.to_elements %[[VEC0]]
+  // CHECK: %[[RES1:.+]]:2 = vector.to_elements %[[VEC1]]
+  %2:4 = vector.to_elements %1 : vector<2x2xf32>
+  return %2#0, %2#1, %2#2, %2#3 : f32, f32, f32, f32
+}
+
+// -----
+
+// In order to verify that the pattern is applied,
+// we need to make sure that the the 2d vector is used
+// by an operation and that extracts are not folded away.
+// In other words we can't use "test.op" nor return the
+// value `%0 = vector.from_elements`
+
+// CHECK-LABEL: @unroll_from_elements_2d
+// CHECK-SAME: (%[[ARG0:.+]]: f32, %[[ARG1:.+]]: f32, %[[ARG2:.+]]: f32, %[[ARG3:.+]]: f32)
+func.func @unroll_from_elements_2d(%arg0: f32, %arg1: f32, %arg2: f32, %arg3: f32) -> (vector<2x2xf32>) {
+  // CHECK: %[[VEC0:.+]] = vector.from_elements %[[ARG0]], %[[ARG1]] : vector<2xf32>
+  // CHECK: %[[VEC1:.+]] = vector.from_elements %[[ARG2]], %[[ARG3]] : vector<2xf32>
+  %0 = vector.from_elements %arg0, %arg1, %arg2, %arg3 : vector<2x2xf32>
+
+  // CHECK: %[[RES0:.+]] = arith.addf %[[VEC0]], %[[VEC0]]
+  // CHECK: %[[RES1:.+]] = arith.addf %[[VEC1]], %[[VEC1]]
+  %1 = arith.addf %0, %0 : vector<2x2xf32>
+
+  // return %[[RES0]], %%[[RES1]] : vector<2xf32>, vector<2xf32>
+  return %1 : vector<2x2xf32>
+}

@llvmbot
Copy link
Member

llvmbot commented Sep 16, 2025

@llvm/pr-subscribers-mlir

Author: Erick Ochoa Lopez (amd-eochoalo)

Changes

This patch adds the unrolling patterns for vector.to_elements and vector.from_elements to the action unrollVectorsInFuncBodies. This affects the passes test-convert-to-spriv (when running with option run-vector-unrolling=true ) and test-spirv-vector-unrolling.


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

2 Files Affected:

  • (modified) mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp (+2)
  • (modified) mlir/test/Conversion/ConvertToSPIRV/vector-unroll.mlir (+44)
diff --git a/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp b/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
index 49f4ce8de7c76..98e294b40456f 100644
--- a/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
+++ b/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
@@ -1495,6 +1495,8 @@ LogicalResult mlir::spirv::unrollVectorsInFuncBodies(Operation *op) {
     RewritePatternSet patterns(context);
     auto options = vector::UnrollVectorOptions().setNativeShapeFn(
         [](auto op) { return mlir::spirv::getNativeVectorShape(op); });
+    vector::populateVectorFromElementsLoweringPatterns(patterns);
+    vector::populateVectorToElementsLoweringPatterns(patterns);
     populateVectorUnrollPatterns(patterns, options);
     if (failed(applyPatternsGreedily(op, std::move(patterns))))
       return failure();
diff --git a/mlir/test/Conversion/ConvertToSPIRV/vector-unroll.mlir b/mlir/test/Conversion/ConvertToSPIRV/vector-unroll.mlir
index c85f4334ff2e5..0957f67690b97 100644
--- a/mlir/test/Conversion/ConvertToSPIRV/vector-unroll.mlir
+++ b/mlir/test/Conversion/ConvertToSPIRV/vector-unroll.mlir
@@ -96,3 +96,47 @@ func.func @transpose(%arg0 : vector<2x3xi32>) -> (vector<3x2xi32>) {
   %0 = vector.transpose %arg0, [1, 0] : vector<2x3xi32> to vector<3x2xi32>
   return %0 : vector<3x2xi32>
 }
+
+// -----
+
+// In order to verify that the pattern is applied,
+// we need to make sure that the the 2d vector does not
+// come from the parameters. Otherwise, the pattern
+// in unrollVectorsInSignatures which splits the 2d vector
+// parameter will take precedent. Similarly, let's avoid
+// returning a vector as another pattern would take precendence.
+
+// CHECK-LABEL: @unroll_to_elements_2d
+func.func @unroll_to_elements_2d() -> (f32, f32, f32, f32) {
+  %1 = "test.op"() : () -> (vector<2x2xf32>)
+  // CHECK: %[[VEC2D:.+]] = "test.op"
+  // CHECK: %[[VEC0:.+]] = vector.extract %[[VEC2D]][0] : vector<2xf32> from vector<2x2xf32>
+  // CHECK: %[[VEC1:.+]] = vector.extract %[[VEC2D]][1] : vector<2xf32> from vector<2x2xf32>
+  // CHECK: %[[RES0:.+]]:2 = vector.to_elements %[[VEC0]]
+  // CHECK: %[[RES1:.+]]:2 = vector.to_elements %[[VEC1]]
+  %2:4 = vector.to_elements %1 : vector<2x2xf32>
+  return %2#0, %2#1, %2#2, %2#3 : f32, f32, f32, f32
+}
+
+// -----
+
+// In order to verify that the pattern is applied,
+// we need to make sure that the the 2d vector is used
+// by an operation and that extracts are not folded away.
+// In other words we can't use "test.op" nor return the
+// value `%0 = vector.from_elements`
+
+// CHECK-LABEL: @unroll_from_elements_2d
+// CHECK-SAME: (%[[ARG0:.+]]: f32, %[[ARG1:.+]]: f32, %[[ARG2:.+]]: f32, %[[ARG3:.+]]: f32)
+func.func @unroll_from_elements_2d(%arg0: f32, %arg1: f32, %arg2: f32, %arg3: f32) -> (vector<2x2xf32>) {
+  // CHECK: %[[VEC0:.+]] = vector.from_elements %[[ARG0]], %[[ARG1]] : vector<2xf32>
+  // CHECK: %[[VEC1:.+]] = vector.from_elements %[[ARG2]], %[[ARG3]] : vector<2xf32>
+  %0 = vector.from_elements %arg0, %arg1, %arg2, %arg3 : vector<2x2xf32>
+
+  // CHECK: %[[RES0:.+]] = arith.addf %[[VEC0]], %[[VEC0]]
+  // CHECK: %[[RES1:.+]] = arith.addf %[[VEC1]], %[[VEC1]]
+  %1 = arith.addf %0, %0 : vector<2x2xf32>
+
+  // return %[[RES0]], %%[[RES1]] : vector<2xf32>, vector<2xf32>
+  return %1 : vector<2x2xf32>
+}

%0 = vector.transpose %arg0, [1, 0] : vector<2x3xi32> to vector<3x2xi32>
return %0 : vector<3x2xi32>
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like these are the only tests that check for unrolling, although it does not check the conversion from vector to spirv, it just checks that the unrolling was applied. We could also start a new file that runs test-convert-to-spirv with run-vector-unrolling=true as an integration test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we need specific vector unrolling test for SPIR-V but we should add independent test to test/Dialect/Vector/.... I think we have vector-unroll-options.mlir for unrolling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tasked with making sure that vector.to_elements and vector.from_elements are unrolled during the test-convert-to-spirv lowering, so I think that adding tests here makes sense.

@amd-eochoalo amd-eochoalo changed the title [mlir] Add vector.{to_elements,from_elements} unrolling to unrollVectorsInFuncBodies [mlir] Move vector.{to_elements,from_elements} unrolling to VectorUnroll.cpp Sep 16, 2025
Comment on lines 898 to 899
UnrollStorePattern, UnrollBroadcastPattern, UnrollFromElements,
UnrollToElements>(patterns.getContext(), options, benefit);
Copy link
Contributor Author

@amd-eochoalo amd-eochoalo Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
UnrollStorePattern, UnrollBroadcastPattern, UnrollFromElements,
UnrollToElements>(patterns.getContext(), options, benefit);
UnrollStorePattern, UnrollBroadcastPattern>(
patterns.getContext(), options, benefit);

If it is more convenient I can make this a non-functional change first by removing these patterns and removing the test. Then I can make another PR which adds the patterns.

Copy link
Contributor

@dcaballe dcaballe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG, a few comments. Thanks!

// Transform N-D vector.from_elements to 1-D vector.from_elements before
// conversion.
vector::populateVectorFromElementsLoweringPatterns(patterns);
vector::populateVectorFromElementsUnrollPatterns(patterns);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to add just the unroll patterns for vector.from_elements here? It sounds a more generic unrolling for all the ops should have happen before calling this pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept these since we have transform dialect operations that call these functions. I think I can follow up this PR and remove them, but I would rather keep this PR simple.

%0 = vector.transpose %arg0, [1, 0] : vector<2x3xi32> to vector<3x2xi32>
return %0 : vector<3x2xi32>
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we need specific vector unrolling test for SPIR-V but we should add independent test to test/Dialect/Vector/.... I think we have vector-unroll-options.mlir for unrolling.

@banach-space
Copy link
Contributor

This PR moves the patterns that unroll vector.to_elements and vector.from_elements into the file with other vector unrolling operations.

+1, thanks!

@amd-eochoalo amd-eochoalo merged commit 8b9c70d into llvm:main Sep 18, 2025
9 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Sep 18, 2025

LLVM Buildbot has detected a new failure on builder ppc64le-mlir-rhel-clang running on ppc64le-mlir-rhel-test while building mlir at step 6 "test-build-check-mlir-build-only-check-mlir".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/29793

Here is the relevant piece of the build log for the reference
Step 6 (test-build-check-mlir-build-only-check-mlir) failure: 1200 seconds without output running [b'ninja', b'check-mlir'], attempting to kill
...
PASS: MLIR :: mlir-tblgen/llvm-intrinsics.td (3487 of 3498)
PASS: MLIR :: Pass/pipeline-options-parsing.mlir (3488 of 3498)
PASS: MLIR-Unit :: IR/./MLIRIRTests/0/129 (3489 of 3498)
PASS: MLIR-Unit :: IR/./MLIRIRTests/38/129 (3490 of 3498)
PASS: MLIR-Unit :: Interfaces/./MLIRInterfacesTests/13/22 (3491 of 3498)
PASS: MLIR-Unit :: Interfaces/./MLIRInterfacesTests/11/22 (3492 of 3498)
PASS: MLIR-Unit :: Pass/./MLIRPassTests/10/13 (3493 of 3498)
PASS: MLIR-Unit :: Interfaces/./MLIRInterfacesTests/12/22 (3494 of 3498)
PASS: MLIR-Unit :: IR/./MLIRIRTests/39/129 (3495 of 3498)
PASS: MLIR :: mlir-reduce/dce-test.mlir (3496 of 3498)
command timed out: 1200 seconds without output running [b'ninja', b'check-mlir'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=2342.098277

kimsh02 pushed a commit to kimsh02/llvm-project that referenced this pull request Sep 19, 2025
…roll.cpp` (llvm#159118)

This PR moves the patterns that unroll vector.to_elements and
vector.from_elements into the file with other vector unrolling
operations. This PR also adds these unrolling patterns into the
`populateVectorUnrollPatterns`. And renames
`populateVectorToElementsLoweringPatterns`
`populateVectorFromElementsLoweringPatterns` to
`populateVectorToElementsUnrollPatterns`
`populateVectorFromElementsUnrollPatterns`.
SeongjaeP pushed a commit to SeongjaeP/llvm-project that referenced this pull request Sep 23, 2025
…roll.cpp` (llvm#159118)

This PR moves the patterns that unroll vector.to_elements and
vector.from_elements into the file with other vector unrolling
operations. This PR also adds these unrolling patterns into the
`populateVectorUnrollPatterns`. And renames
`populateVectorToElementsLoweringPatterns`
`populateVectorFromElementsLoweringPatterns` to
`populateVectorToElementsUnrollPatterns`
`populateVectorFromElementsUnrollPatterns`.
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.

6 participants