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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:137
     //========================================================================//
+    InterfaceMethod<
+      "Query the reference iterator types attribute for this named op. "
----------------
jpienaar wrote:
> 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.
yes I am doing that in the next commit along with redoing the tablegen part.
I'm considering this as followup work, please shout if you disagree. 


================
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 "
----------------
jpienaar wrote:
> 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.
I rephrased and renamed, thanks!



================
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"
+    >,
----------------
jpienaar wrote:
> What does a return of none signify?
updated, thanks!


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:368
+    llvm::Optional<SmallVector<AffineMap, 8>> referenceIndexingMaps() {
+      llvm_unreachable("NYI referenceIndexingMaps for MatvecOp");
     }
----------------
mravishankar wrote:
> Curious why. Isnt it 
> `affine_map<(d0, d1) -> (d0, d1)>, affine_map<(d0, d1) -> (d0)>, affine_map<(d0, d1) -> (d1)>`?
Right now it is still encoded in different pieces in C++.
The idea is I will give a declarative tablgen form to autogenerate.

If using the parser is acceptable for this it could work too but the problem it that:
1. it can't be a static method if we want rank polymorphism
2. it is recomputed at each call so calling the Parser seems overkill.

anyway, fun discussions that are bigger than just this NFC revision.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:392
+    llvm::Optional<SmallVector<AffineMap, 8>> referenceIndexingMaps() {
+      llvm_unreachable("NYI referenceIndexingMaps for MatmulOp");
     }
----------------
mravishankar wrote:
> Same as above. Is there some information missing that blocks this?
same as above too :)


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:234
+    auto name = this->getOperation()->getName().getStringRef();
+    if (!maybeReferenceIteratorTypes && name != "generic" &&
+        name != "indexed_generic") {
----------------
mravishankar wrote:
> I think you dont need this. You already have an unreachable in referenceIteratorTypes
This is setting the ground for the next revision, I agree it is redundant right now.


================
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>();
----------------
jpienaar wrote:
> Could we move this to a constant on the trait? Or would the trait generate an accessor for it?
I am not sure what a constant on the trait would mean here.
The "referenceIndexingMaps" will be a constant in the trait after the next revision and tablegen additions.

This property is the one that is used for all transformations.
For named ops it will not be set but specified and autogenerated with Tablegen. 


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:266
+    auto name = this->getOperation()->getName().getStringRef();
+    if (!maybeReferenceIndexingMaps && name != "generic" &&
+        name != "indexed_generic") {
----------------
mravishankar wrote:
> Same here. You probably dont need this. referenceIndexingMaps already has the unreachable where needed
same as above re future evolution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74491