[Mlir-commits] [mlir] [mlir] Expose linalg vectorization without replacement (PR #144158)

[Andrzej Warzyński]

banach-space wrote:

Thank you both for the additional context — that’s exactly what I was missing. Special thanks to @hanhanW for the detailed and generous overview of related discussions; I found it extremely helpful 🙏🏻

---

To quickly clarify my earlier point about “dead code”:
What I meant is that, with this change, there’s nothing upstream preventing us from reverting this:
```cpp
LogicalResult mlir::linalg::vectorize(...) {
  SmallVector<Value> results;
  LogicalResult vectorizeResult = mlir::linalg::vectorize(results, ...);
}
```
back into something like:
```cpp
LogicalResult mlir::linalg::vectorize(...) {
  SmallVector<Value> results;
  // inline mlir::linalg::vectorize(results, ...);
}
```
In other words, the new overload of `vectorize` (and the indirection it introduces) _doesn’t provide any distinct, reusable functionality_  (from the point of view) upstream. Hence my concern - poorly scoped abstractions can accumulate and become hard to reason about over time. (Admittedly, “dead code” wasn’t the best way to describe that - sorry for the confusion.)

----

> I believe this is not uncommon here in MLIR. The TilingInterface utilities, for example, don't do any replacement:

Thanks for pointing this out - my bad for not paying closer attention to similar patterns elsewhere. It does seem like this change is trying to bring the vectorize API more in line with the structure used in other transformations, e.g.:

* [tileUsingSCF](https://github.com/llvm/llvm-project/blob/577199f9221ebc805a69372a2b19f4c8ebaf1daf/mlir/include/mlir/Dialect/SCF/Transforms/TileUsingInterface.h#L152-L156), used by [transform::TileUsingForOp::apply](https://github.com/llvm/llvm-project/blob/577199f9221ebc805a69372a2b19f4c8ebaf1daf/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp#L3099-L3244)
* [mlir::linalg::generalizeNamedOp](https://github.com/llvm/llvm-project/blob/577199f9221ebc805a69372a2b19f4c8ebaf1daf/mlir/lib/Dialect/Linalg/Transforms/Generalization.cpp#L53-L75), used by [transform::GeneralizeOp::applyToOne](https://github.com/llvm/llvm-project/blob/577199f9221ebc805a69372a2b19f4c8ebaf1daf/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp#L1178-L1194)

That said, as @hanhanW mentioned, `generalizeNamedOp` does perform the replacement inline - [see here](https://github.com/llvm/llvm-project/blob/577199f9221ebc805a69372a2b19f4c8ebaf1daf/mlir/lib/Dialect/Linalg/Transforms/Generalization.cpp#L73). 

That seems inconsistent, and I’d love to understand the rationale. Clarifying that would help confirm that this change is indeed a step toward a more uniform transformation interface - which I agree is a worthwhile goal.
--- 

> Like what I mentioned above, I'd suggest returning std::optional<VectorizationResult> which makes it clearer.

Wouldn’t `FailureOr` be more appropriate here? Especially if we’re aligning with similar transformation interfaces elsewhere.

---

> I'm happy to chat with you offline or we can prepare RFC together, and hopefully (4) won't become a blocker for the change.

That sounds great — I’d be happy to sync offline as well.

---

All in all, I’m happy for you to move forward with this change. That said, my one request would be to ensure that **only a single** `vectorize` **entry point** is exposed upstream.

Based on the discussion, the following signature seems sufficient to serve both upstream and downstream needs:

```cpp
FailureOr<VectorizationResult>
vectorize(RewriterBase &rewriter,
          Operation *op,
           ArrayRef<int64_t> inputVectorSizes = {},
           ArrayRef<bool> inputScalableVecDims = {},
           bool vectorizeNDExtract = false,
           bool flatten1DDepthwiseConv = false);
```

This would also:
* Bring us closer to a design similar to `tileUsingSCF`,
* Avoid maintaining multiple high-level entry points into the vectorizer.

How does this sound?

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