[mlir][affine] Add Check for 'affine.for' Bound Map Results #127105
Conversation
|
@llvm/pr-subscribers-mlir Author: Ayokunle Amodu (ayokunle321) ChangesRelated to issue #120001. I learnt that we shouldn’t assert on IR that can be constructed and passes the verifier. In this case, it appears that the verifier is insufficiently strict and should be tightened. Since the maps used in the affine.for op below do not have results, the IR verification should fail however it does not. Minimal example: #map = affine_map<() -> ()> This gets past the verifier, but I added a check to see that it causes an error if an affine.for has a bound map with no result. Open to any comments or clarifications :) Full diff: https://github.com/llvm/llvm-project/pull/127105.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index 147f5dd7a24b6..06e26e887b050 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -1903,6 +1903,12 @@ LogicalResult AffineForOp::verifyRegions() {
getUpperBoundMap().getNumDims())))
return failure();
+ if (getLowerBoundMap().getNumResults() < 1)
+ return emitOpError("expected lower bound map to have at least one result");
+
+ if (getUpperBoundMap().getNumResults() < 1)
+ return emitOpError("expected upper bound map to have at least one result");
+
unsigned opNumResults = getNumResults();
if (opNumResults == 0)
return success();
diff --git a/mlir/test/Dialect/Affine/invalid.mlir b/mlir/test/Dialect/Affine/invalid.mlir
index 44e484b9ba598..5a3243cb074c1 100644
--- a/mlir/test/Dialect/Affine/invalid.mlir
+++ b/mlir/test/Dialect/Affine/invalid.mlir
@@ -523,3 +523,25 @@ func.func @dynamic_dimension_index() {
}) : () -> ()
return
}
+
+// -----
+
+#map = affine_map<() -> ()>
+#map1 = affine_map<() -> (1)>
+func.func @no_lower_bound() {
+ // expected-error@+1 {{'affine.for' op expected lower bound map to have at least one result}}
+ affine.for %i = max #map() to min #map1() {
+ }
+ return
+}
+
+// -----
+
+#map = affine_map<() -> ()>
+#map1 = affine_map<() -> (1)>
+func.func @no_upper_bound() {
+ // expected-error@+1 {{'affine.for' op expected upper bound map to have at least one result}}
+ affine.for %i = max #map1() to min #map() {
+ }
+ return
+}
|
|
@llvm/pr-subscribers-mlir-affine Author: Ayokunle Amodu (ayokunle321) ChangesRelated to issue #120001. I learnt that we shouldn’t assert on IR that can be constructed and passes the verifier. In this case, it appears that the verifier is insufficiently strict and should be tightened. Since the maps used in the affine.for op below do not have results, the IR verification should fail however it does not. Minimal example: #map = affine_map<() -> ()> This gets past the verifier, but I added a check to see that it causes an error if an affine.for has a bound map with no result. Open to any comments or clarifications :) Full diff: https://github.com/llvm/llvm-project/pull/127105.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index 147f5dd7a24b6..06e26e887b050 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -1903,6 +1903,12 @@ LogicalResult AffineForOp::verifyRegions() {
getUpperBoundMap().getNumDims())))
return failure();
+ if (getLowerBoundMap().getNumResults() < 1)
+ return emitOpError("expected lower bound map to have at least one result");
+
+ if (getUpperBoundMap().getNumResults() < 1)
+ return emitOpError("expected upper bound map to have at least one result");
+
unsigned opNumResults = getNumResults();
if (opNumResults == 0)
return success();
diff --git a/mlir/test/Dialect/Affine/invalid.mlir b/mlir/test/Dialect/Affine/invalid.mlir
index 44e484b9ba598..5a3243cb074c1 100644
--- a/mlir/test/Dialect/Affine/invalid.mlir
+++ b/mlir/test/Dialect/Affine/invalid.mlir
@@ -523,3 +523,25 @@ func.func @dynamic_dimension_index() {
}) : () -> ()
return
}
+
+// -----
+
+#map = affine_map<() -> ()>
+#map1 = affine_map<() -> (1)>
+func.func @no_lower_bound() {
+ // expected-error@+1 {{'affine.for' op expected lower bound map to have at least one result}}
+ affine.for %i = max #map() to min #map1() {
+ }
+ return
+}
+
+// -----
+
+#map = affine_map<() -> ()>
+#map1 = affine_map<() -> (1)>
+func.func @no_upper_bound() {
+ // expected-error@+1 {{'affine.for' op expected upper bound map to have at least one result}}
+ affine.for %i = max #map1() to min #map() {
+ }
+ return
+}
|
ayokunle321
changed the title
|
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ayokunle321
changed the title
Related to issue #120001.
I learnt that we shouldn’t assert on IR that can be constructed and passes the verifier. In this case, it appears that the verifier is insufficiently strict and should be tightened. Since the maps used in the
affine.forop do not have results, I believe the IR verification should fail however it does not.Minimal example:
#map = affine_map<() -> ()>module {func.func @main() {affine.for %arg0 = max #map() to min #map() {}return}}This gets past the verifier, but I added a check to see that it causes an error if an affine.for has a bound map with no result.
@krzysz00 @matthias-springer @kazutakahirata @MaheshRavishankar Open to any comments or clarifications :)