[PATCH] D73826: [mlir][Linalg] Provide a Tablegen backend to specify named Linalg ops declaratively

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:56-57
+// Named Linalg ops, implemented as special configurations of generic ops.
+// At the moment these are not declarative and require a bunch of C++ code.
+// These should be migrated to the declarative configurations as they evolve.
 ////////////////////////////////////////////////////////////////////////////////
----------------
Should this comment be outside of the "section heading" block?


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:290-296
+      auto ctx = getContext();
+      unsigned rank = output().getType().cast<ShapedType>().getRank();
+      if (rank == 0)
+        return SmallVector<AffineMap, 8>{
+          AffineMap::get(0, 0, {getAffineConstantExpr(0, ctx)})};
+      return SmallVector<AffineMap, 8>{
+        AffineMap::getMultiDimIdentityMap(rank, ctx)};
----------------
Why not have a trait that declares these and then have you backend generate the definitions? (else it seems that to know you have this function you need to hardcode the names vs casting to the op interface)


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:327
+
+def MatvecOp : LinalgNamedStructured_Op<"matvec", [
+     NInputs<2>,
----------------
TableGen (and therefore ODS) does not have namespaces, please follow what we are doign everywhere else and prefacing the dialect as namespace. Else one can't write patterns using these ops without concern of collisions.

(true above and below too)


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td:44
+        reduction loop}],
+      "unsigned", "hasSingleReductionLoop">,
+
----------------
Why unsigned but comment says true?


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td:54
+    InterfaceMethod<"Return the input view at the given index.",
+      "Value ", "getInput", (ins "unsigned":$i)
+    >,
----------------
space after value? (same below too)


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td:166
+    StaticInterfaceMethod<[{
+        Create an operation of the current type with the given location,
+        operands, and attributes.
----------------
Is the only advantage here that you don't need to specify type? If so, why not use infer type op interface?


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td:179
+        Clone the current operation with the given location and operands. This
+        is used to abstract away the optional underlying region creation.
+      }],
----------------
So this is different from clone in that you can specify location too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73826