[mlir][vector] Fix parser of vector.transfer_read #133721

douyixuan · 2025-03-31T13:55:52Z

This PR adds a check in the parser to prevent a crash when vector.transfer_read fails to create minor identity permutation. map.
Fixes #132851

a.mlir

module {
  func.func @test_vector.transfer_read(%arg1: memref<?xindex>) -> vector<3x4xi32> {
    %c3_i32 = arith.constant 3 : i32
    %0 = vector.transfer_read %arg1[%c3_i32, %c3_i32], %c3_i32 : memref<?xindex>, vector<3x4xi32>
    return %0 : vector<3x4xi32>
  }
}

github-actions · 2025-03-31T13:56:11Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-03-31T13:56:52Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-vector

Author: Cedric (douyixuan)

Changes

This PR adds a check in the parser to prevent a crash when vector.transfer_read fails to create minor identity permutation. map.
Fixes #132851

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

2 Files Affected:

(modified) mlir/lib/Dialect/Vector/IR/VectorOps.cpp (+8-1)
(modified) mlir/test/Dialect/Vector/invalid.mlir (+9)

diff --git a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
index 5a3983699d5a3..d99c8ef0680f4 100644
--- a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
+++ b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
@@ -151,7 +151,7 @@ static bool isSupportedCombiningKind(CombiningKind combiningKind,
 }
 
 AffineMap mlir::vector::getTransferMinorIdentityMap(ShapedType shapedType,
-                                                    VectorType vectorType) {
+                                               VectorType vectorType) {
   int64_t elementVectorRank = 0;
   VectorType elementVectorType =
       llvm::dyn_cast<VectorType>(shapedType.getElementType());
@@ -164,6 +164,9 @@ AffineMap mlir::vector::getTransferMinorIdentityMap(ShapedType shapedType,
     return AffineMap::get(
         /*numDims=*/0, /*numSymbols=*/0,
         getAffineConstantExpr(0, shapedType.getContext()));
+  if (shapedType.getRank() < vectorType.getRank() - elementVectorRank) {
+    return AffineMap::get(shapedType.getContext());
+  }
   return AffineMap::getMinorIdentityMap(
       shapedType.getRank(), vectorType.getRank() - elementVectorRank,
       shapedType.getContext());
@@ -4260,6 +4263,10 @@ ParseResult TransferReadOp::parse(OpAsmParser &parser, OperationState &result) {
   AffineMap permMap;
   if (!permMapAttr) {
     permMap = getTransferMinorIdentityMap(shapedType, vectorType);
+    if (permMap.isEmpty()) {
+      return parser.emitError(typesLoc,
+                              "failed to create minor identity permutation map");
+    }
     result.attributes.set(permMapAttrName, AffineMapAttr::get(permMap));
   } else {
     permMap = llvm::cast<AffineMapAttr>(permMapAttr).getValue();
diff --git a/mlir/test/Dialect/Vector/invalid.mlir b/mlir/test/Dialect/Vector/invalid.mlir
index ea6d0021391fb..667e1615212f4 100644
--- a/mlir/test/Dialect/Vector/invalid.mlir
+++ b/mlir/test/Dialect/Vector/invalid.mlir
@@ -525,6 +525,15 @@ func.func @test_vector.transfer_read(%arg0: memref<?x?xvector<2x3xf32>>) {
 
 // -----
 
+func.func @test_vector.transfer_read(%arg1: memref<?xindex>) -> vector<3x4xi32> {
+  %c3_i32 = arith.constant 3 : i32
+  // expected-error@+1 {{failed to create minor identity permutation map}}
+  %0 = vector.transfer_read %arg1[%c3_i32, %c3_i32], %c3_i32 : memref<?xindex>, vector<3x4xi32>
+  return %0 : vector<3x4xi32>
+}
+
+// -----
+
 func.func @test_vector.transfer_write(%arg0: memref<?x?xf32>) {
   %c3 = arith.constant 3 : index
   %cst = arith.constant 3.0 : f32

mlir/lib/Dialect/Vector/IR/VectorOps.cpp

CoTinker · 2025-04-02T01:22:32Z

mlir/lib/Dialect/Vector/IR/VectorOps.cpp

@@ -164,6 +164,10 @@ AffineMap mlir::vector::getTransferMinorIdentityMap(ShapedType shapedType,
    return AffineMap::get(
        /*numDims=*/0, /*numSymbols=*/0,
        getAffineConstantExpr(0, shapedType.getContext()));
+  if (shapedType.getRank() < vectorType.getRank() - elementVectorRank) {
+    return AffineMap(); // Not enough dimensions in the shaped type to form a


The comments couldn't in same line with code.

Does elementVectorRank matter here? If yes, could you write a test to showcase this?

Separately, given that this is effectively checking some basic pre-conditions, could you move it all the way to the top?

Yes, a test would be like this, a memref with a 1D vector, elementVectorRank is 1. There are several in test/Dialect/Vector/vector-transfer-to-vector-load-store.mlir.

func.func @test_vector.transfer_read(%arg1: memref<?xvector<2xindex>>) -> vector<2xindex> { %c0 = arith.constant 0 : index %c1 = arith.constant dense<[0, 0]> : vector<2xindex> %vector = vector.transfer_read %arg1[%c0], %c1 : memref<?xvector<2xindex>>, vector<2xindex> return %vector : vector<2xindex> }

OK, could you add tests for this in invalid.mlir? Thanks!

Actually, the provided MLIR is valid, and the elementVectorRank calculation is tested in test/Dialect/Vector/vector-transfer-to-vector-load-store.mlir. Additionally, it is verified by the test case related to this issue, where the elementVectorRank is 0. I decided not to include this test this time.

module { func.func @test_vector.transfer_read(%arg1: memref<?xindex>) -> vector<3x4xi32> { %c3_i32 = arith.constant 3 : i32 %0 = vector.transfer_read %arg1[%c3_i32, %c3_i32], %c3_i32 : memref<?xindex>, vector<3x4xi32> return %0 : vector<3x4xi32> } }

mlir/lib/Dialect/Vector/IR/VectorOps.cpp

banach-space

Thanks! I've left some suggestions.

banach-space · 2025-04-02T12:40:15Z

mlir/lib/Dialect/Vector/IR/VectorOps.cpp

@@ -164,6 +164,10 @@ AffineMap mlir::vector::getTransferMinorIdentityMap(ShapedType shapedType,
    return AffineMap::get(
        /*numDims=*/0, /*numSymbols=*/0,
        getAffineConstantExpr(0, shapedType.getContext()));
+  if (shapedType.getRank() < vectorType.getRank() - elementVectorRank) {
+    return AffineMap(); // Not enough dimensions in the shaped type to form a


Does elementVectorRank matter here? If yes, could you write a test to showcase this?

Separately, given that this is effectively checking some basic pre-conditions, could you move it all the way to the top?

mlir/lib/Dialect/Vector/IR/VectorOps.cpp

mlir/test/Dialect/Vector/invalid.mlir

dcaballe

Thanks for fixing this! LGTM

mlir/test/Dialect/Vector/invalid.mlir

banach-space · 2025-04-02T20:17:16Z

mlir/lib/Dialect/Vector/IR/VectorOps.cpp

+    if (!permMap) {
+      return parser.emitError(
+          typesLoc, "failed to create a minor identity map, source rank is less than required for vector rank");


OK, based on your reply elementVectorRank, this would make more sense to me (apologies about formatting):

Suggested change

if (!permMap) {

return parser.emitError(

typesLoc, "failed to create a minor identity map, source rank is less than required for vector rank");

if (!permMap) {

int64_t elementVectorRank = 0;

VectorType elementVectorType = llvm::dyn_cast<VectorType>(shapedType.getElementType());

if (elementVectorType)

elementVectorRank += elementVectorType.getRank();

if (shapedType.getRank() < vectorType.getRank() - elementVectorRank)

return parser.emitError(

typesLoc, "expecting a custom permutation_map when source rank is less than required for vector rank");

Initially, I removed the elementVectorRank calculation since the snippet was repetitive. I noticed there might be other instances where an invalid permMap is returned from getPermutationMapAttrName. Following your suggestion, I added the snippet, which seems more reasonable now.

@banach-space Could you please help review this PR?

github-actions · 2025-04-03T01:36:19Z

✅ With the latest revision this PR passed the C/C++ code formatter.

banach-space

Thanks for the updates, just a few final nits.

mlir/lib/Dialect/Vector/IR/VectorOps.cpp

banach-space · 2025-04-07T17:33:44Z

mlir/lib/Dialect/Vector/IR/VectorOps.cpp

+                                "expected a custom permutation_map when source "
+                                "rank is less than required for vector rank");


Suggested change

"expected a custom permutation_map when source "

"rank is less than required for vector rank");

"expected a custom permutation_map when rank(source) != rank (destination)";

Same comment for the transfer_write part. This way these conditions are identical and can later be extracted to a helper function (I can do it in a separate PR).

banach-space · 2025-04-07T17:34:12Z

mlir/test/Dialect/Vector/invalid.mlir

@@ -525,6 +525,15 @@ func.func @test_vector.transfer_read(%arg0: memref<?x?xvector<2x3xf32>>) {

 // -----

+func.func @test_vector.transfer_read(%arg1: memref<?xindex>) -> vector<3x4xi32> {


Let's not mix types: index -> i32. Same comment for @test_vector.transfer_write.

The test case associated with this issue utilizes i32 instead of index, while the transfer_write test case serves as its inverse. Should I include an additional test case specifically for i32?

From the point of view of the issue that you are fixing, the underlying element type is not relevant. Hence, this should be totally sufficient:

func.func @test_vector.transfer_read(%arg1: memref<?xi32>) -> vector<3x4xi32> { %c3 = arith.constant 3 : index // expected-error@+1 {{expected a custom permutation_map when rank(source) != rank(destination)}} %0 = vector.transfer_read %arg1[%c3, %c3], %c3 : memref<?xi32>, vector<3x4xi32> return %0 : vector<3x4xi32> }

Should I include an additional test case specifically for i32?

One test case per Op (using either i32 or index for all element types) will be sufficient.

Thanks for asking!

mlir/lib/Dialect/Vector/IR/VectorOps.cpp

[mlir][vector] Fix parser of vector.transfer_read

8675a21

douyixuan requested review from banach-space, dcaballe, nicolasvasilache and Groverkss as code owners March 31, 2025 13:55

llvmbot added mlir:vectorops mlir mlir:vector labels Mar 31, 2025

[mlir][vector] Fix parser of vector.transfer_read

38f8b62

CoTinker reviewed Apr 1, 2025

View reviewed changes

mlir/lib/Dialect/Vector/IR/VectorOps.cpp Outdated Show resolved Hide resolved

CoTinker requested a review from joker-eph April 1, 2025 03:21

[mlir][vector] Fix parser of vector.transfer_read

6755d1d

CoTinker reviewed Apr 2, 2025

View reviewed changes

mlir/lib/Dialect/Vector/IR/VectorOps.cpp Outdated Show resolved Hide resolved

[mlir][vector] Fix parser of vector.transfer_read

ba2f392

banach-space reviewed Apr 2, 2025

View reviewed changes

[mlir][vector] Fix parser of vector.transfer_read

3a8d23c

dcaballe approved these changes Apr 2, 2025

View reviewed changes

mlir/test/Dialect/Vector/invalid.mlir Outdated Show resolved Hide resolved

banach-space reviewed Apr 2, 2025

View reviewed changes

[mlir][vector] Fix parser of vector.transfer_read

f671fe9

douyixuan mentioned this pull request Apr 3, 2025

[mlir] vector.transfer_read and vector.transfer_write allow different source and destination element type #134268

Open

douyixuan requested review from banach-space and CoTinker April 3, 2025 16:19

banach-space reviewed Apr 7, 2025

View reviewed changes

[mlir][vector] Fix parser of vector.transfer_read

bbe35d6

CoTinker reviewed Apr 9, 2025

View reviewed changes

mlir/lib/Dialect/Vector/IR/VectorOps.cpp Outdated Show resolved Hide resolved

[mlir][vector] Fix parser of vector.transfer_read

2a920e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mlir][vector] Fix parser of vector.transfer_read #133721

[mlir][vector] Fix parser of vector.transfer_read #133721

douyixuan commented Mar 31, 2025 •

edited

Loading

github-actions bot commented Mar 31, 2025

llvmbot commented Mar 31, 2025 •

edited

Loading

CoTinker Apr 2, 2025

banach-space Apr 2, 2025

douyixuan Apr 2, 2025 •

edited

Loading

banach-space Apr 2, 2025

douyixuan Apr 3, 2025 •

edited

Loading

banach-space left a comment

banach-space Apr 2, 2025

dcaballe left a comment

banach-space Apr 2, 2025

douyixuan Apr 3, 2025 •

edited

Loading

github-actions bot commented Apr 3, 2025 •

edited

Loading

banach-space left a comment

banach-space Apr 7, 2025

banach-space Apr 7, 2025

douyixuan Apr 8, 2025

banach-space Apr 8, 2025

		"expected a custom permutation_map when source "
		"rank is less than required for vector rank");

		@@ -525,6 +525,15 @@ func.func @test_vector.transfer_read(%arg0: memref<?x?xvector<2x3xf32>>) {

		// -----

		func.func @test_vector.transfer_read(%arg1: memref<?xindex>) -> vector<3x4xi32> {

[mlir][vector] Fix parser of vector.transfer_read #133721

Are you sure you want to change the base?

[mlir][vector] Fix parser of vector.transfer_read #133721

Conversation

douyixuan commented Mar 31, 2025 • edited Loading

github-actions bot commented Mar 31, 2025

llvmbot commented Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douyixuan Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douyixuan Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

banach-space left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcaballe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douyixuan Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 3, 2025 • edited Loading

banach-space left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douyixuan commented Mar 31, 2025 •

edited

Loading

llvmbot commented Mar 31, 2025 •

edited

Loading

douyixuan Apr 2, 2025 •

edited

Loading

douyixuan Apr 3, 2025 •

edited

Loading

douyixuan Apr 3, 2025 •

edited

Loading

github-actions bot commented Apr 3, 2025 •

edited

Loading