[PATCH] D74491: [mlir][Linalg] NFC - Refactor in preparation for automatic Linalg "named" ops.

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar added a comment.

Wanted to give some feedback before next meeting, but I didn't review the C++ code changes in depth.



================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:137
     //========================================================================//
+    InterfaceMethod<
+      "Query the reference iterator types attribute for this named op. "
----------------
You probably want to move these out into a different file. It is rare but if linalg ops get included along with other interfaces then it would result in generating the interfaces 2x and have some non-intuitive behavior :) Keeping op interfaces in separate file so that the separate mlir-tblgen invocations for op def and op interface def can be used keeps things simple.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:138
+    InterfaceMethod<
+      "Query the reference iterator types attribute for this named op. "
+      "Typically this would be a static method but in order to allow "
----------------
This isn't very descriptive, e.g., what it the result being returned here?. Could we reformulate it something like Returns the reference iterator attribute ?

I'm also not sure what Type signify here.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:141
+      "rank-polymorphic ops, this needs to be per object instance.",
+      "llvm::Optional<SmallVector<StringRef, 8>>", "referenceIteratorTypes"
+    >,
----------------
What does a return of none signify?


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:157
     >,
+    InterfaceMethod<"Query the input or output indexing map at index `i`.",
+      "AffineMap", "getIndexingMap", (ins "unsigned":$i)
----------------
Same here and below. "Query" makes me think of a layer of indirection.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:256
+    // Return the attribute if it is present.
+    if (auto attr = this->getOperation()->getAttr("indexing_maps"))
+      return attr.template cast<ArrayAttr>();
----------------
Could we move this to a constant on the trait? Or would the trait generate an accessor for it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74491