Skip to content
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

[mlir][tosa]-Edit the verifier of tosa constShapeOp #126962

Merged

Conversation

amirBish
Copy link
Contributor

Add verification for rank 1 for the elements' attribute of the tosa const_shape operation.

Add verification for rank 1 for the elements' attribute
of the tosa const_shape operation.
@llvmbot
Copy link
Member

llvmbot commented Feb 12, 2025

@llvm/pr-subscribers-mlir

Author: Amir Bishara (amirBish)

Changes

Add verification for rank 1 for the elements' attribute of the tosa const_shape operation.


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

2 Files Affected:

  • (modified) mlir/lib/Dialect/Tosa/IR/TosaOps.cpp (+4)
  • (modified) mlir/test/Dialect/Tosa/invalid.mlir (+8)
diff --git a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
index 4928be38476a9..e782838ad97f9 100644
--- a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
+++ b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
@@ -2618,6 +2618,10 @@ OpTrait::tosa::verifyTosaShapeOperatorWithSameRanks(Operation *op) {
 //===----------------------------------------------------------------------===//
 
 LogicalResult tosa::ConstShapeOp::verify() {
+  // check one dimensional rank
+  auto valuesRank = getValue().getType().getRank();
+  if (valuesRank != 1)
+    return emitOpError("expect elements in attribute value with rank 1");
   // check that number of elements in value attr equal to rank of result shape
   auto count = getValue().getNumElements();
   auto rank = (cast<tosa::shapeType>(getResult().getType())).getRank();
diff --git a/mlir/test/Dialect/Tosa/invalid.mlir b/mlir/test/Dialect/Tosa/invalid.mlir
index 913191be86f85..ff874af5c5f07 100644
--- a/mlir/test/Dialect/Tosa/invalid.mlir
+++ b/mlir/test/Dialect/Tosa/invalid.mlir
@@ -1087,6 +1087,14 @@ func.func @test_const_shape_value() -> !tosa.shape<5> {
 
 // -----
 
+func.func @test_const_shape_value() -> !tosa.shape<4> {
+  // expected-error@+1 {{'tosa.const_shape' op expect elements in attribute value with rank 1}}
+  %cst = tosa.const_shape {value = dense<[[1, 2], [3, 4]]> : tensor<2x2xindex>} : () -> !tosa.shape<4>
+  return %cst : !tosa.shape<4>
+}
+
+// -----
+
 func.func @test_sub_with_unequal_operand_ranks(%arg0: tensor<1x21x3xf32>, %arg1: tensor<1x13x21x3xf32>) -> tensor<1x13x21x3xf32> {
   // expected-error@+1 {{'tosa.sub' op operands don't have matching ranks}}
   %0 = tosa.sub %arg0, %arg1 : (tensor<1x21x3xf32>, tensor<1x13x21x3xf32>) -> tensor<1x13x21x3xf32>

@llvmbot
Copy link
Member

llvmbot commented Feb 12, 2025

@llvm/pr-subscribers-mlir-tosa

Author: Amir Bishara (amirBish)

Changes

Add verification for rank 1 for the elements' attribute of the tosa const_shape operation.


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

2 Files Affected:

  • (modified) mlir/lib/Dialect/Tosa/IR/TosaOps.cpp (+4)
  • (modified) mlir/test/Dialect/Tosa/invalid.mlir (+8)
diff --git a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
index 4928be38476a9..e782838ad97f9 100644
--- a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
+++ b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
@@ -2618,6 +2618,10 @@ OpTrait::tosa::verifyTosaShapeOperatorWithSameRanks(Operation *op) {
 //===----------------------------------------------------------------------===//
 
 LogicalResult tosa::ConstShapeOp::verify() {
+  // check one dimensional rank
+  auto valuesRank = getValue().getType().getRank();
+  if (valuesRank != 1)
+    return emitOpError("expect elements in attribute value with rank 1");
   // check that number of elements in value attr equal to rank of result shape
   auto count = getValue().getNumElements();
   auto rank = (cast<tosa::shapeType>(getResult().getType())).getRank();
diff --git a/mlir/test/Dialect/Tosa/invalid.mlir b/mlir/test/Dialect/Tosa/invalid.mlir
index 913191be86f85..ff874af5c5f07 100644
--- a/mlir/test/Dialect/Tosa/invalid.mlir
+++ b/mlir/test/Dialect/Tosa/invalid.mlir
@@ -1087,6 +1087,14 @@ func.func @test_const_shape_value() -> !tosa.shape<5> {
 
 // -----
 
+func.func @test_const_shape_value() -> !tosa.shape<4> {
+  // expected-error@+1 {{'tosa.const_shape' op expect elements in attribute value with rank 1}}
+  %cst = tosa.const_shape {value = dense<[[1, 2], [3, 4]]> : tensor<2x2xindex>} : () -> !tosa.shape<4>
+  return %cst : !tosa.shape<4>
+}
+
+// -----
+
 func.func @test_sub_with_unequal_operand_ranks(%arg0: tensor<1x21x3xf32>, %arg1: tensor<1x13x21x3xf32>) -> tensor<1x13x21x3xf32> {
   // expected-error@+1 {{'tosa.sub' op operands don't have matching ranks}}
   %0 = tosa.sub %arg0, %arg1 : (tensor<1x21x3xf32>, tensor<1x13x21x3xf32>) -> tensor<1x13x21x3xf32>

@amirBish
Copy link
Contributor Author

amirBish commented Feb 12, 2025

@Jerry-Ge this change is regarding this thread https://discourse.llvm.org/t/questions-regarding-tosa-shape-type/84431
would be glad for your review

@amirBish amirBish requested a review from sjarus February 12, 2025 19:52
Copy link
Contributor

@sjarus sjarus left a comment

Choose a reason for hiding this comment

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

LGTM!

@Tai78641 if you have no further comment, I'll go ahead and merge this.

// check one dimensional rank
auto valuesRank = getValue().getType().getRank();
if (valuesRank != 1)
return emitOpError("expect elements in attribute value with rank 1");
Copy link
Contributor

Choose a reason for hiding this comment

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

this is great! thank you. I was just about to work on similar checks.
if you don't mind, would you please add an additional check that
the getValue().getType() has element type which is an index type?

Copy link
Contributor Author

@amirBish amirBish Feb 12, 2025

Choose a reason for hiding this comment

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

It's already being validated with the type of value being declared in the td file.

take a look at mlir/include/mlir/Dialect/Tosa/IR/TosaShapeOps.td line 70:
let arguments = (ins IndexElementsAttr : $value);

@Tai78641
Copy link
Contributor

LGTM

@sjarus sjarus merged commit 51c847d into llvm:main Feb 12, 2025
11 checks passed
flovent pushed a commit to flovent/llvm-project that referenced this pull request Feb 13, 2025
Add verification for rank 1 for the elements' attribute of the tosa
const_shape operation.
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 14, 2025
Add verification for rank 1 for the elements' attribute of the tosa
const_shape operation.
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025
Add verification for rank 1 for the elements' attribute of the tosa
const_shape operation.
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.

4 participants