[mlir][SCF] Do not verify step size of scf.for
#78141
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
An op verifier should verify only local properties. This commit removes the verification of `scf.for` step sizes. (Verifiers can check attributes but should not follow SSA values.) This verification could reject IR that is actually valid, e.g.:
```mlir
scf.if %always_false {
// Branch is never entered.
scf.for ... step %c0 { ... }
}
```
This commit fixes `for-loop-peeling.mlir` when running with `MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS`:
```
within split at llvm-project/mlir/test/Dialect/SCF/for-loop-peeling.mlir:293 offset :9:3: note: see current operation:
"scf.for"(%0, %3, %2) ({
^bb0(%arg1: index):
%4 = "arith.index_cast"(%arg1) : (index) -> i64
"memref.store"(%4, %arg0) : (i64, memref<i64>) -> ()
"scf.yield"() : () -> ()
}) {__peeled_loop__} : (index, index, index) -> ()
LLVM ERROR: IR failed to verify after folding
```
|
@llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) ChangesAn op verifier should verify only local properties. This commit removes the verification of scf.if %always_false {
// Branch is never entered.
scf.for ... step %c0 { ... }
}This commit fixes Full diff: https://github.com/llvm/llvm-project/pull/78141.diff 3 Files Affected:
diff --git a/mlir/lib/Dialect/SCF/IR/SCF.cpp b/mlir/lib/Dialect/SCF/IR/SCF.cpp
index 5570c2ec688c8a..cdc0b6f1696ae9 100644
--- a/mlir/lib/Dialect/SCF/IR/SCF.cpp
+++ b/mlir/lib/Dialect/SCF/IR/SCF.cpp
@@ -332,10 +332,6 @@ void ForOp::build(OpBuilder &builder, OperationState &result, Value lb,
}
LogicalResult ForOp::verify() {
- IntegerAttr step;
- if (matchPattern(getStep(), m_Constant(&step)) && step.getInt() <= 0)
- return emitOpError("constant step operand must be positive");
-
// Check that the number of init args and op results is the same.
if (getInitArgs().size() != getNumResults())
return emitOpError(
diff --git a/mlir/test/Dialect/SCF/for-loop-peeling.mlir b/mlir/test/Dialect/SCF/for-loop-peeling.mlir
index aa8fd7ec7ef84d..f59b79603b4899 100644
--- a/mlir/test/Dialect/SCF/for-loop-peeling.mlir
+++ b/mlir/test/Dialect/SCF/for-loop-peeling.mlir
@@ -292,15 +292,19 @@ func.func @regression(%arg0: memref<i64>, %arg1: index) {
// -----
-// Check that this doesn't crash but trigger the verifier.
+// Regression test: Make sure that we do not crash.
+
+// CHECK-LABEL: func @zero_step(
+// CHECK: scf.for
+// CHECK: scf.for
func.func @zero_step(%arg0: memref<i64>) {
%c0 = arith.constant 0 : index
%c1 = arith.constant 1 : index
%foldto0 = arith.subi %c1, %c1 : index
- // expected-error @+1 {{'scf.for' op constant step operand must be positive}}
scf.for %arg2 = %c0 to %c1 step %foldto0 {
%2 = arith.index_cast %arg2 : index to i64
memref.store %2, %arg0[] : memref<i64>
}
return
}
+
diff --git a/mlir/test/Dialect/SCF/invalid.mlir b/mlir/test/Dialect/SCF/invalid.mlir
index fac9d825568f72..337eb9eeb8fa57 100644
--- a/mlir/test/Dialect/SCF/invalid.mlir
+++ b/mlir/test/Dialect/SCF/invalid.mlir
@@ -32,18 +32,6 @@ func.func @loop_for_mismatch(%arg0: i32, %arg1: index) {
// -----
-func.func @loop_for_step_positive(%arg0: index) {
- // expected-error@+2 {{constant step operand must be positive}}
- %c0 = arith.constant 0 : index
- "scf.for"(%arg0, %arg0, %c0) ({
- ^bb0(%arg1: index):
- scf.yield
- }) : (index, index, index) -> ()
- return
-}
-
-// -----
-
func.func @loop_for_one_region(%arg0: index) {
// expected-error@+1 {{requires one region}}
"scf.for"(%arg0, %arg0, %arg0) (
|
|
@llvm/pr-subscribers-mlir-scf Author: Matthias Springer (matthias-springer) ChangesAn op verifier should verify only local properties. This commit removes the verification of scf.if %always_false {
// Branch is never entered.
scf.for ... step %c0 { ... }
}This commit fixes Full diff: https://github.com/llvm/llvm-project/pull/78141.diff 3 Files Affected:
diff --git a/mlir/lib/Dialect/SCF/IR/SCF.cpp b/mlir/lib/Dialect/SCF/IR/SCF.cpp
index 5570c2ec688c8a..cdc0b6f1696ae9 100644
--- a/mlir/lib/Dialect/SCF/IR/SCF.cpp
+++ b/mlir/lib/Dialect/SCF/IR/SCF.cpp
@@ -332,10 +332,6 @@ void ForOp::build(OpBuilder &builder, OperationState &result, Value lb,
}
LogicalResult ForOp::verify() {
- IntegerAttr step;
- if (matchPattern(getStep(), m_Constant(&step)) && step.getInt() <= 0)
- return emitOpError("constant step operand must be positive");
-
// Check that the number of init args and op results is the same.
if (getInitArgs().size() != getNumResults())
return emitOpError(
diff --git a/mlir/test/Dialect/SCF/for-loop-peeling.mlir b/mlir/test/Dialect/SCF/for-loop-peeling.mlir
index aa8fd7ec7ef84d..f59b79603b4899 100644
--- a/mlir/test/Dialect/SCF/for-loop-peeling.mlir
+++ b/mlir/test/Dialect/SCF/for-loop-peeling.mlir
@@ -292,15 +292,19 @@ func.func @regression(%arg0: memref<i64>, %arg1: index) {
// -----
-// Check that this doesn't crash but trigger the verifier.
+// Regression test: Make sure that we do not crash.
+
+// CHECK-LABEL: func @zero_step(
+// CHECK: scf.for
+// CHECK: scf.for
func.func @zero_step(%arg0: memref<i64>) {
%c0 = arith.constant 0 : index
%c1 = arith.constant 1 : index
%foldto0 = arith.subi %c1, %c1 : index
- // expected-error @+1 {{'scf.for' op constant step operand must be positive}}
scf.for %arg2 = %c0 to %c1 step %foldto0 {
%2 = arith.index_cast %arg2 : index to i64
memref.store %2, %arg0[] : memref<i64>
}
return
}
+
diff --git a/mlir/test/Dialect/SCF/invalid.mlir b/mlir/test/Dialect/SCF/invalid.mlir
index fac9d825568f72..337eb9eeb8fa57 100644
--- a/mlir/test/Dialect/SCF/invalid.mlir
+++ b/mlir/test/Dialect/SCF/invalid.mlir
@@ -32,18 +32,6 @@ func.func @loop_for_mismatch(%arg0: i32, %arg1: index) {
// -----
-func.func @loop_for_step_positive(%arg0: index) {
- // expected-error@+2 {{constant step operand must be positive}}
- %c0 = arith.constant 0 : index
- "scf.for"(%arg0, %arg0, %c0) ({
- ^bb0(%arg1: index):
- scf.yield
- }) : (index, index, index) -> ()
- return
-}
-
-// -----
-
func.func @loop_for_one_region(%arg0: index) {
// expected-error@+1 {{requires one region}}
"scf.for"(%arg0, %arg0, %arg0) (
|
kuhar
approved these changes
Jan 15, 2024
justinfargnoli
pushed a commit
to justinfargnoli/llvm-project
that referenced
this pull request
Jan 28, 2024
An op verifier should verify only local properties. This commit removes
the verification of `scf.for` step sizes. (Verifiers can check
attributes but should not follow SSA values.) This verification could
reject IR that is actually valid, e.g.:
```mlir
scf.if %always_false {
// Branch is never entered.
scf.for ... step %c0 { ... }
}
```
This commit fixes `for-loop-peeling.mlir` when running with
`MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS`:
```
within split at llvm-project/mlir/test/Dialect/SCF/for-loop-peeling.mlir:293 offset :9:3: note: see current operation:
"scf.for"(%0, %3, %2) ({
^bb0(%arg1: index):
%4 = "arith.index_cast"(%arg1) : (index) -> i64
"memref.store"(%4, %arg0) : (i64, memref<i64>) -> ()
"scf.yield"() : () -> ()
}) {__peeled_loop__} : (index, index, index) -> ()
LLVM ERROR: IR failed to verify after folding
```
Note: `%2` is `arith.constant 0 : index`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An op verifier should verify only local properties. This commit removes the verification of
scf.forstep sizes. (Verifiers can check attributes but should not follow SSA values.) This verification could reject IR that is actually valid, e.g.:This commit fixes
for-loop-peeling.mlirwhen running withMLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS:Note:
%2isarith.constant 0 : index.