[PATCH] D76414: [mlir][Linalg] Introduce linalg.pooling_min/max/sum op.

[Han-Chung Wang via Phabricator via llvm-commits]

hanchung added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:410
+  let description = [{
+    Applies a pooling function to all elements in each window of the input
+    multi-dimensional array, producing an output multi-dimensional array with
----------------
mravishankar wrote:
> mehdi_amini wrote:
> > The doc should define what is a "pooling function" and what does it mean to "pool" 
> You should probably use this description (https://www.tensorflow.org/api_docs/python/tf/nn/pool) . This is similar to how the conv op is defined.
Yes, this is a good reference. :)


================
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) :
----------------
mravishankar wrote:
> nicolasvasilache wrote:
> > 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.
> > 
> > 
> wouldnt that be a new named op for every pooling operation. This seems better to me.
Both are okay to me. I think maybe a "named op" should have more concrete behavior. Otherwise it's like a generic op.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:474
+      // The window loops has the same number loops with output dimensions.
+      unsigned nWin = nPar;
+      SmallVector<StringRef, 8> iters(nPar, getParallelIteratorTypeName());
----------------
mravishankar wrote:
> That seems strange. Looking at the documentation (https://www.tensorflow.org/api_docs/python/tf/nn/pool) this doesnt seem to be the case.
I think it's should be the case:
window_shape: Sequence of N ints >= 1

Also, if this is not the case, we must have another attributes to specify the corresponding dimensions.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:500
+        };
+    }
+  }];
----------------
mehdi_amini wrote:
> This is quite a block of C++, can you move this all out of ODS?
I'm following other ops style here. I'm happy to move all of them out of ODS, but can we keep it as this way for now?

Nicolas, do you have any comment/suggestion here?


================
Comment at: mlir/test/Dialect/Linalg/loops.mlir:259
+  linalg.pooling { strides = [2, 1] } %arg0, %arg1, %arg2 {
+    ^bb(%a: f32, %b: f32) :
+      %0 = addf %a, %b : f32
----------------
mehdi_amini wrote:
> Have you looked into the syntax of other operations (like the GPU launch or for loops) which elide the block and integrate the block arg in the syntax itself?
Good to know it! I looked into the syntax and thought it's much better than this way. However, the op doesn't take a region now. I'll follow it if I need a region next time. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76414