[Mlir-commits] [mlir] [mlir][Vector] Reject invalid inline yields (PR #206218)

[Alessandro Potenza]

https://github.com/alepot55 commented:

I built `11e915f2b75d` and ran the repro from #206083 plus three variants of it, changing only the terminator:

```mlir
llvm.func @callee(%arg0: vector<8xf32>) {
  %0 = math.atanh %arg0 : vector<8xf32>
  vector.yield %arg0 : vector<8xf32>     // <- gpu.yield / linalg.yield / affine.yield
}
llvm.func @caller(%arg0: vector<8xf32>) attributes {llvm.emit_c_interface} {
  llvm.call @callee(%arg0) : (vector<8xf32>) -> ()
  llvm.return
}
```

`mlir-opt -inline`, verbatim:

| terminator | result |
| --- | --- |
| `vector.yield` | `must implement handleTerminator in the case of one inlined block`, UNREACHABLE |
| `gpu.yield` | the same UNREACHABLE |
| `linalg.yield` | `'linalg.yield' op expected parent op with LinalgOp interface` |
| `affine.yield` | `'affine.yield' op only terminates affine.if/for/parallel regions` |

Two things follow.

**Vector is not alone.** `GPUInlinerInterface` in `GPUDialect.cpp` is the same code this PR is changing, down to the comment:

```cpp
/// All gpu dialect ops can be inlined.
bool isLegalToInline(Operation *, Region *, bool, IRMapping &) const final {
  return true;
}
```

GPU has no `handleTerminator` either, and `gpu.yield` has exactly one parent in the tree, `gpu.warp_execute_on_lane_0`. Whatever shape this lands in, GPU needs the same change; I am happy to send that patch once the shape here is settled.

**The tree has already answered this twice, and not in an inliner interface.** Scanning `mlir/include/mlir/Dialect` for ops that declare both `ReturnLike` and `Terminator` and no `HasParent`/`ParentOneOf` turns up six: `spirv.Return`, `spirv.ReturnValue`, `gpu.yield`, `linalg.yield`, `vector.yield`, `affine.yield`. The two in that list that decline the input above do it in their own verifier, not in the inliner: `AffineYieldOp::verify()` rejects any parent outside `affine.if/for/parallel`, and `linalg.yield` requires a parent implementing `LinalgOp`.

That is worth weighing here, because a verifier makes the IR invalid for every consumer, whereas this patch teaches one pass to decline and leaves the same IR to be built by anything else. `vector.yield`'s own definition says the semantics "is defined by the parent operation", and `vector.mask` is the only op in tree using it as a terminator, so the constraint exists to be written down.

The argument the other way is real too: a verifier breaks out-of-tree ops that use `vector.yield` as their terminator, and this patch does not. That trade is for @banach-space and @dcaballe to call, not me. But it should be called deliberately, since the answer also decides what happens to `gpu.yield`.

Separately, and only because it will hold this up regardless of the technical outcome: @PragmaTwice's question from 24 July about the AI Tool Use Policy is still unanswered on this PR.

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