This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by nicolasvasilache on Feb 12 2020, 8:48 AM.

Details

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.

Diff Detail

Unit TestsFailed

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2020, 8:48 AM
nicolasvasilache retitled this revision from [mlir][Linalg] Refactor in preparation for automatic Linalg "named" ops. to [mlir][Linalg] NFC - Refactor in preparation for automatic Linalg "named" ops..Feb 12 2020, 8:50 AM
mravishankar accepted this revision.Feb 12 2020, 10:13 AM

Overall looks fine to me.

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

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

392

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

This revision is now accepted and ready to land.Feb 12 2020, 10:13 AM

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?

157

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 marked 17 inline comments as done.

ddress review comments.

ld breaking typo.

Fix build breadking typo.

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!

368

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.

392

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.

This revision was automatically updated to reflect the committed changes.