Skip to content

Commit

Permalink
[mlir][SCF] Do not verify step size of scf.for
Browse files Browse the repository at this point in the history
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
```
  • Loading branch information
matthias-springer committed Jan 15, 2024
1 parent 08e4386 commit d65132c
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 18 deletions.
4 changes: 0 additions & 4 deletions mlir/lib/Dialect/SCF/IR/SCF.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
8 changes: 6 additions & 2 deletions mlir/test/Dialect/SCF/for-loop-peeling.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

12 changes: 0 additions & 12 deletions mlir/test/Dialect/SCF/invalid.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -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) (
Expand Down

0 comments on commit d65132c

Please sign in to comment.