llvm / llvm/llvm-project

[MLIR][Affine] `affine-loop-invariant-code-motion` hoists a region op without checking what its region captures

Open
#216,545 2 comments 0 reactions 0 assignees View on GitHub
mlir:affine
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

`isOpLoopInvariant` recurses into the regions of exactly three operations — `affine.if`, `affine.for`, `affine.parallel` — and treats every other region-carrying operation as if its regions were empty, examining only `op.getOperands()`. An `scf.for` whose bounds, step and initial `iter_args` are all loop-invariant therefore looks hoistable even when its body reads a value defined inside the loop, and the hoisted operation is placed before the `affine.for` with its region still referring to a value that stayed behind.

## Reproducer

### small.mlir
```mlir
func.func @f(%m: memref<4xi64>, %init: i64, %outside: i64) -> i64 {
%c0 = arith.constant 0 : i32
%c4 = arith.constant 4 : i32
%c2 = arith.constant 2 : i32
%one = arith.constant 1 : i64
%r = affine.for %i = 0 to 4 iter_args(%acc = %init) -> (i64) {
%v = affine.load %m[%i] : memref<4xi64>
%s = scf.for %j = %c0 to %c4 step %c2 iter_args(%a = %outside) -> (i64) : i32 {
%o = arith.ori %v, %one : i64
%n = arith.addi %a, %o : i64
scf.yield %n : i64
}
%acc2 = arith.addi %acc, %s : i64
affine.yield %acc2 : i64
}
return %r : i64
}
```

### To reproduce

```
$ mlir-opt small.mlir -o /dev/null
(input parses and verifies)

$ mlir-opt small.mlir --affine-loop-invariant-code-motion -o /dev/null
small.mlir:12:12: error: operand #0 does not dominate this use
%o = arith.ori %v, %one : i64
^
small.mlir:8:10: note: operand defined here (op is neither in a parent nor in a child region)
%v = affine.load %m[%i] : memref<4xi64>
```

With the verifier switched off the transformation is plain — the `scf.for` now sits *above* the `affine.for` and reads `%6`, which is defined below it, inside the loop body:

```bash
$ mlir-opt small.mlir --affine-loop-invariant-code-motion --verify-each=false -o -
```

```mlir
%4 = "scf.for"(%0, %1, %2, %arg2) ({
^bb0(%arg5: i32, %arg6: i64):
%8 = "arith.ori"(%6, %3) : (i64, i64) -> i64 // %6 is defined below, in the loop
%9 = "arith.addi"(%arg6, %8) : (i64, i64) -> i64
"scf.yield"(%9) : (i64) -> ()
}) : (i32, i32, i32, i64) -> i64
%5 = "affine.for"(%arg1) ({
^bb0(%arg3: index, %arg4: i64):
%6 = "affine.load"(%arg0, %arg3) : (memref<4xi64>, index) -> i64
%7 = "arith.addi"(%arg4, %4) : (i64, i64) -> i64
"affine.yield"(%7) : (i64) -> ()
}) : (i64) -> i64
```

### Actual Output:

```
error: operand #0 does not dominate this use
```

### Expected Output:

```
(the pass either hoists legally or declines; the module still verifies)
```

Contributor guide

Open the contributing guide

Research direction

Start by reproducing the failure with small.mlir using mlir-opt --affine-loop-invariant-code-motion, then inspect isOpLoopInvariant and how the pass handles region-carrying operations. Add coverage for the nested scf.for case and verify that the transformed module remains valid, either by legal hoisting or by declining to hoist it.

Written by the indexing model from the issue text.

Assessment

Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.