[Mlir-commits] [mlir] [mlir][SCF] Do not verify step size of `scf.for` (PR #78141)

[llvmlistbot at llvm.org]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir-scf

Author: Matthias Springer (matthias-springer)

<details>
<summary>Changes</summary>

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
```

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


3 Files Affected:

- (modified) mlir/lib/Dialect/SCF/IR/SCF.cpp (-4) 
- (modified) mlir/test/Dialect/SCF/for-loop-peeling.mlir (+6-2) 
- (modified) mlir/test/Dialect/SCF/invalid.mlir (-12) 


``````````diff
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 at +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 at +1 {{requires one region}}
   "scf.for"(%arg0, %arg0, %arg0) (

``````````

</details>


https://github.com/llvm/llvm-project/pull/78141