[Mlir-commits] [mlir] [Linalg] Add basic infra to add matchers for linalg.*conv*/*pool* ops (PR #163724)

[Abhishek Varma]

================
@@ -110,6 +110,15 @@ GenericOp makeMemRefCopyOp(OpBuilder &b, Location loc, Value from, Value to);
 std::optional<SmallVector<ReassociationIndices>>
 getReassociationMapForFoldingUnitDims(ArrayRef<OpFoldResult> mixedSizes);
 
+//===----------------------------------------------------------------------===//
+// Convolution matcher utility
+//===----------------------------------------------------------------------===//
+
+template <typename ConvOpTy>
+bool isaConvolutionOpOfType(LinalgOp op,
+                            SmallVector<int64_t> *dilations = nullptr,
+                            SmallVector<int64_t> *strides = nullptr);
----------------
Abhishek-Varma wrote:

> One if stmt is not a big deal, but there is a lot going on here - I'd rather we focus on the problem at hand than worry about generality. WDYT?

Sure, I understand your point and I've made an update to reflect the suggested change.

I usually prefer generalizing an implementation earlier on the best we can.
For instance while scoping out work required to update :-
1. [DownscaleSizeOneWindowed2DConvolution](https://github.com/llvm/llvm-project/blob/580fdeb6ff55fcd54be16ed8555eaaa6a9aee1c0/mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp#L1383)  to work with `linalg.generic`, and
2. [Vectorization](https://github.com/llvm/llvm-project/blob/af82c1a67b3a1dfc05b6149e68caa30103c15ce8/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp#L4236-L4242) to also use `linalg.generic`,

I figured that our API we'll need to have `dilations`/`strides` optional.
A similar rationale for not using `assert(isaConvolutionOpInterface(op)...)` - as these will be necessary checks that the caller of our API will have to ensure anyway.

Two ways to go about this :-
```
1. But as discussed earlier we can remove these in the follow-up patches to make the API as well as
   the caller sites cleaner as the problem we're trying to deal with through the current patch
   doesn't warrant such changes.
```
OR 
```
2. Again, even without removing these in the follow-up patch, I guess I can make it work. Example :-
    - I can add a `if (isaConvolutionOpInterface(op))` before invoking our API at the above caller sites.
    - I can use a dummy `SmallVector<int64_t> dilations, strides;` before invoking our API at the above
      caller sites.
```

But the question then would be if we would actually want those callers to take on the extra effort.

Note: Just penning down my thoughts to prefetch/scope our plan better for the above caller sites in follow-up PRs that I'll take up. Doesn't require immediate response/attention, but we're heading there sooner.

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