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

[nicolasvasilache]

Summary

This revision prepares the ground for declaratively defining Linalg "named" ops. Such named ops form the backbone of operations that are ubiquitous in the ML application domain.

This revision closely related to the definition of a "Tensor Computation Primitives Dialect" and demonstrates that ops can be expressed as declarative configurations of the linalg.generic op.

[mravishankar]

Overall looks fine to me.

mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td

373

Curious why. Isnt it
affine_map<(d0, d1) -> (d0, d1)>, affine_map<(d0, d1) -> (d0)>, affine_map<(d0, d1) -> (d1)>?

397

Same as above. Is there some information missing that blocks this?

mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h

234

I think you dont need this. You already have an unreachable in referenceIteratorTypes

266

Same here. You probably dont need this. referenceIndexingMaps already has the unreachable where needed

[jpienaar]

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

mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td

137

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.

138

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.

141

What does a return of none signify?

162

Same here and below. "Query" makes me think of a layer of indirection.

mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h

256

Could we move this to a constant on the trait? Or would the trait generate an accessor for it?

[nicolasvasilache]

ddress review comments.

[nicolasvasilache]

ld breaking typo.

[nicolasvasilache]

Fix build breadking typo.

[nicolasvasilache]

mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td

137

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.

138

I rephrased and renamed, thanks!

141

updated, thanks!

373

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.

397

same as above too :)

mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h

234

This is setting the ground for the next revision, I agree it is redundant right now.

256

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.

266

same as above re future evolution.