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

[Han-Chung Wang]

hanhanW wrote:

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

Honestly, I don't know which is better and what the rationale is. Maybe different contributor has different preference. If I had to choose, I'd not perform the replacement. Because it provides more control to other users. I'm not sure if the replacement would become an issue or not when people build complicated patterns and dialect conversion or type converter is involved.

> > Like what I mentioned above, I'd suggest returning std::optional which makes it clearer.
> 
> Wouldn’t `FailureOr` be more appropriate here? Especially if we’re aligning with similar transformation interfaces elsewhere.

Yes, it can be `FailureOr`. I somehow like `std::optional` better for no reasons. I don't know how to line up `FailureOr` in pattern rewriting and feel that most codes would wrap it with `failed()`; it loses the error message. E.g.,

```cpp
if (failed(vectorize(...)) {
  return failure();
}
```

But yeah, `FailureOr` seems more appropriate in this case.

> Based on the discussion, the following signature seems sufficient to serve both upstream and downstream needs:
> 
> ```c++
> 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?

Thanks, it sounds good to me!

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