[PATCH] D79135: [mlir][Linalg] Add support to lower named ops to loops.

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

Looks good... some minor comments.



================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:357-358
+
+    // Padded conv involves a affine.max in the memory acces which is not
+    // allowed by affine.load. Override to always use an StdIndecexValue.
+    StdIndexedValue F(convOp.filter()), I(convOp.input()), O(convOp.output());
----------------
A couple of typos here: access and StdIndexedValue


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:358
+    // Padded conv involves a affine.max in the memory acces which is not
+    // allowed by affine.load. Override to always use an StdIndecexValue.
+    StdIndexedValue F(convOp.filter()), I(convOp.input()), O(convOp.output());
----------------
I might have missed this but a test case exercising this path is probably missing. 


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:689
+///   2. One operand + returns a single AffineDimExpr
+///   3. One operands + returns a single AffineSymbolExpr
 //
----------------
Nit: typo operands -> operand.


================
Comment at: mlir/test/Dialect/Linalg/loops.mlir:3
 // RUN: mlir-opt %s -convert-linalg-to-parallel-loops | FileCheck --check-prefix=CHECKPARALLEL %s
+// RUN: mlir-opt %s -convert-linalg-to-affine-loops --disable-pass-threading | FileCheck --check-prefix=CHECKAFFINE %s
 
----------------
Nit: I think you have a separate file affine.mlir for -convert-linalg-to-affine-loops?


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1574
 
   std::string mapsStr;
   llvm::raw_string_ostream mapsStringStream(mapsStr);
----------------
A comment here and perhaps above will help. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79135