[Mlir-commits] [mlir] [Linalg] Update Conv Decomposition patterns to work with generic convolution ops as well (PR #174196)

[Andrzej Warzyński]

================
@@ -106,12 +106,17 @@ getReassociationMapForFoldingUnitDims(ArrayRef<OpFoldResult> mixedSizes);
 // Convolution matcher utility
 //===----------------------------------------------------------------------===//
 
-/// Given a linalg `op` this function returns true if it is a convolution op of
-/// type `ConvOpTy` and populates `dilations` and `strides` with values inferred
-/// from the indexing maps.
+/// A struct containing dilations and strides inferred from convolution ops.
+struct DilationsAndStrides {
+  SmallVector<int64_t> dilations;
+  SmallVector<int64_t> strides;
+};
+
+/// Given a linalg `op` this function returns DilationsAndStrides if it is a
+/// convolution op of type `ConvOpTy`, otherwise returns std::nullopt. The
+/// dilations and strides are inferred from the indexing maps.
 template <typename ConvOpTy>
-bool isaConvolutionOpOfType(LinalgOp op, SmallVector<int64_t> *dilations,
-                            SmallVector<int64_t> *strides);
+std::optional<DilationsAndStrides> isaConvolutionOpOfType(LinalgOp op);
----------------
banach-space wrote:

Thanks for the explanation!

> 2. Or, revert to the previous implementation (pointer arguments of dilations/strides)

I was just about to ask this - is the change actually needed? Based on your summary:

> -- This required the following updates to the isaConvolutionOfType API :- 
> 1. Allow to return a DilationsAndStrides struct instead of bool.

this implies the API change is necessary for the PR to work. However, per your explanation above, that doesn’t seem to be the case, so the summary is misleading.

Please update the summary so that it accurately reflects what the PR implements and, more importantly, why. As noted in the MLIR contributing guidelines (https://mlir.llvm.org/getting_started/Contributing/#commit-messages):

> Prefer describing why the change is implemented rather than what it does.

Finally, regarding the API change itself:

> I'd prefer option 1.

This is an API update that isn’t required for decomposition patterns to work with generic ops, i.e. it’s orthogonal to this PR. Please extract it into a dedicated PR so we can discuss it independently.

Thanks for contributing!

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