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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini 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
----------------
The doc should define what is a "pooling function" and what does it mean to "pool" 


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:425
+
+    TODO(hanchung): Figure out a better way to handle window dimensions, i.e.,
+    eliminate the fake memref.
----------------
Please remove username from TODO (this is a google internal practice).

Also note that this doc will be automatically published on the website when you push this.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:500
+        };
+    }
+  }];
----------------
This is quite a block of C++, can you move this all out of ODS?


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76414