Page MenuHomePhabricator

[mlir] NFC - Expose an OffsetSizeAndStrideOpInterface
ClosedPublic

Authored by nicolasvasilache on Nov 24 2020, 1:24 AM.

Details

Summary

This revision will make it easier to create new ops base on the strided memref abstraction outside of the std dialect.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Nov 24 2020, 1:24 AM
rriddle requested changes to this revision.Nov 24 2020, 1:34 AM

Can you add proper documentation in the description+commit on what this interface is, how it is intended to be used, and what the invariants are?

mlir/include/mlir/Interfaces/ViewLikeInterface.td
35

Please provide a much more detailed description here. It isn't clear what this is supposed to mean or how it should be used.

218

The formatting on most of these is off, please fix it.

This revision now requires changes to proceed.Nov 24 2020, 1:34 AM
rriddle added inline comments.Nov 24 2020, 1:40 AM
mlir/include/mlir/Interfaces/ViewLikeInterface.h
22

SubViewOp/SubTensorOp don't make sense to mention here.

nicolasvasilache marked an inline comment as done.Nov 24 2020, 2:41 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Interfaces/ViewLikeInterface.td
35

Note: I am hoping to also provide reusable verification/parsing/printing.
Any concern with exposing those through interfaces?

nicolasvasilache marked 2 inline comments as done.

Address comments.

Address comments.

rriddle added inline comments.Nov 24 2020, 2:51 AM
mlir/include/mlir/Interfaces/ViewLikeInterface.td
35

Verification is common, there is a verify field on OpInterface where you can specify that and it will be placed on the generated interface trait class. For parsers/printers those are fine to provide as utilities, just not really on the interface class itself. I'd expose those in a impl namespace in ViewLikeInterfaces.h. I think we already do this with some other parsers/printers somewhere else in the code base. I'd only put things on the interface that are supposed to generalize well across all of the use cases, parsers/printers generally don't.

pifon2a accepted this revision.Nov 24 2020, 5:02 AM

super-nice!

Integrate a little deeper.

nicolasvasilache marked an inline comment as done.Nov 24 2020, 6:42 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Interfaces/ViewLikeInterface.td
35

Thanks! Parser and printer coming as followups.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 24 2020, 6:47 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
nicolasvasilache marked an inline comment as done.