-
Couldn't load subscription status.
- Fork 15k
[mlir][emitc] Fix emitc.for verification crash #163754
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
base: main
Are you sure you want to change the base?
Conversation
This PR adds block arguments check to prevent a crash in `emitc.for` verifier.
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-emitc Author: Longsheng Mou (CoTinker) ChangesThis PR adds block arguments check to prevent a crash in Full diff: https://github.com/llvm/llvm-project/pull/163754.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index 4754f0bfe895e..77a56fb463243 100644
--- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
+++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
@@ -584,6 +584,10 @@ void ForOp::print(OpAsmPrinter &p) {
LogicalResult ForOp::verifyRegions() {
// Check that the body defines as single block argument for the induction
// variable.
+ if (getBody()->getNumArguments() == 0)
+ return emitOpError("expected body to have a single block argument for the "
+ "induction variable");
+
if (getInductionVar().getType() != getLowerBound().getType())
return emitOpError(
"expected induction variable to be same type as bounds and step");
diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir
index 5f594fb08c43f..f30a614a93de0 100644
--- a/mlir/test/Dialect/EmitC/invalid_ops.mlir
+++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir
@@ -876,3 +876,28 @@ func.func @test_do(%arg0 : !emitc.ptr<i32>) {
return
}
+
+// -----
+
+func.func @test_for_none_block_argument(%arg0: index) {
+ // expected-error@+1 {{expected body to have a single block argument for the induction variable}}
+ "emitc.for"(%arg0, %arg0, %arg0) (
+ {
+ emitc.yield
+ }
+ ) : (index, index, index) -> ()
+ return
+}
+
+// -----
+
+func.func @test_for_unmatch_type(%arg0: index) {
+ // expected-error@+1 {{expected induction variable to be same type as bounds}}
+ "emitc.for"(%arg0, %arg0, %arg0) (
+ {
+ ^bb0(%i0 : f32):
+ emitc.yield
+ }
+ ) : (index, index, index) -> ()
+ return
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!
| LogicalResult ForOp::verifyRegions() { | ||
| // Check that the body defines as single block argument for the induction | ||
| // variable. | ||
| if (getBody()->getNumArguments() == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (getBody()->getNumArguments() == 0) | |
| if (getBody()->getNumArguments() != 1) |
|
|
||
| // ----- | ||
|
|
||
| func.func @test_for_none_block_argument(%arg0: index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding a third case with more than one argument.
This PR adds block arguments check to prevent a crash in
emitc.forverifier. Fixes #159950.