[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:

I don't like this change :)

The name, `isaConvolutionOpOfType`, strongly implies that the result should be a `bool`. If you want to extend this matcher then the name should be updated accordingly. I don't have any good suggestions myself (naming is hard). Instead, I would keep the original method as is **and** implement a new method to compute dilations and strides (e.g. `getDilationsAndStrides`). Wouldn't the latter be straightforward? Or perhaps I am missing something?

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