[PATCH] D76414: [mlir][Linalg] Introduce linalg.pooling op.

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:273
+  code commonUtils = [{
+    std::string getLibraryCallName() {
+      return generateLibraryCallName(getOperation());
----------------
I would keep the `libraryCallName #`.
If / when it changes in the future we won't have to track it down.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:444
+    ```mlir
+      linalg.pooling { strides = [2, 1, 2] } %arg0, %arg1, %arg2 {
+        ^bb(%a: f32, %b: f32) :
----------------
If we go through the trouble of creating a new "named op" for pooling, I would not specify a region.

Instead I would have a `linalg.pooling_max`, `linalg.pooling_min`, `linalg.pooling.sum`, ... with no regions that just apply the proper operation when lowering to loops.




================
Comment at: mlir/lib/Conversion/LinalgToLLVM/LinalgToLLVM.cpp:527
   // attribute values such as kernel striding and dilation.
   patterns.insert<CopyTransposeConversion, LinalgOpConversion<ConvOp>,
+                  LinalgOpConversion<PoolingOp>, LinalgOpConversion<CopyOp>,
----------------
let's use 
```
//clang-format off
...
//clang-format on
```

and 1 pattern per line if you don't mind, so we have fewer reflowing diff in the longer term.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76414/new/

https://reviews.llvm.org/D76414