This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Add ConvolutionOpInterface.
ClosedPublic

Authored by mravishankar on Sep 14 2021, 4:10 PM.

Details

Summary

Add an interface for convolution ops. Currently does some basic
verification and returns a charecterization of the loop dimensions.

Diff Detail

Event Timeline

mravishankar created this revision.Sep 14 2021, 4:10 PM
mravishankar requested review of this revision.Sep 14 2021, 4:10 PM

Some of the tests fail due to issue with verifier. I dont plan to submit before that is addressed in some way, but otherwise this is ready for review

Can you expand in the description on the motivation for all of this?

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
92

(Please fix before pushing this)

Can you expand in the description on the motivation for all of this?

I am waiting for other people to comment before I finish this up (making sure this is along the right lines). There are a whole bunch of convolution named ops defined. Having an interface will allow reasoning about the op during transformation without having to switch between all the different ops. Also if done right, any new ops that implement the interface would also be handled seemlessly. So far this is just doing verification of the op, and characterization of the loops in terms of convolutions.

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
92

Thanks. I forgot about this one.

Fix issue with verification order and address missing documentation.

mravishankar marked an inline comment as done.Sep 16 2021, 10:56 PM

This is now ready for review.

Generally this looks good to me.

I'd drop the parts of the interface named after convnet terminology inthis CL and only focus on the verifier, which looks very nice to me.
The reason is that the interface should be geared towards enabling/simplifying transformations, in particular vectorization (like vector.contraction does) and I suspect those names will not pay for themselves.

We also need negative tests for the validation errors.

Accepting conditioned on the 2 points above.

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h
30

I am unclear why these are needed immediately.
Can we split in a followup CL that also uses and tests them ?
This will also make this CL simpler to review.

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
116

One of the advantages of having an interface is to be able to increase the flexibility of the op definitions and be able to detect them from a linalg.generic (in particular to rewrite a generic as a named op for which we have a library call or a fast instruction, in the future).

How about adding a TODO that we want to detect the image operand in the future?

131

We don't need to assume there is only one?
We can return whatever the inference tells us from analysing the affine maps?

nicolasvasilache accepted this revision.Sep 17 2021, 4:16 AM
This revision is now accepted and ready to land.Sep 17 2021, 4:16 AM
antiagainst accepted this revision.Sep 17 2021, 8:57 AM

Thanks Mahesh! Overall LGTM, just a few nits/typos.

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h
58

Depthwise dimension reads a bit strange to me. We talk about depthwise convolution op as a whole. But for the dimension, you mean Depth Multiplier dimension?

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
95

s/satisy/satisfy/

98

Nit: Could use code blocks to wrap these grammar definitions.

103

s/change/channel/

mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
291

Would be nice to put the comment here in bullet points for better readability.

mlir/python/mlir/dialects/linalg/opdsl/lang/comprehension.py
487

This is duplicated?

Can you expand in the description on the motivation for all of this?

I am waiting for other people to comment before I finish this up (making sure this is along the right lines).

Still not done?

Also I think this is fairly important to have when sending for review initially!

Can you expand in the description on the motivation for all of this?

I am waiting for other people to comment before I finish this up (making sure this is along the right lines).

Still not done?

Also I think this is fairly important to have when sending for review initially!

Its done now. Wanted comments on it before I go finish it up since wasnt sure that it met what everyone was looking for. Anyway, added it now

mravishankar marked 6 inline comments as done.
  • Add negative tests
  • Split methods that return loop characterization into separate patch
  • Add better description

Make tests less complex.

Generally this looks good to me.

I'd drop the parts of the interface named after convnet terminology inthis CL and only focus on the verifier, which looks very nice to me.

Done.

The reason is that the interface should be geared towards enabling/simplifying transformations, in particular vectorization (like vector.contraction does) and I suspect those names will not pay for themselves.

We also need negative tests for the validation errors.

Where/how do I add these? I was looking for what is done for ContractionOpInterface and I didnt find any negative tests.

Accepting conditioned on the 2 points above.

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h
58

Thanks I didnt know what it was called in general.

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
116

Ill make this the default implementation. Ops can override if they want to.

131

True, but all batches could be grouped into one "batch". I think we can expand that if we really need to, but I cant think of a case for it. Just starting with the minimum flexibility needed to support current ops. We can generalize later on.

mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
291

It was. It got collapsed with Esc-q on emacs. Sorry.

mlir/python/mlir/dialects/linalg/opdsl/lang/comprehension.py
487

From what? The one above is the ContractionOpInterface. Is it duplicated somewhere else?

nicolasvasilache accepted this revision.Sep 19 2021, 3:05 PM

you're right that the error messages in the contraction op interface are not tested .. what you have here looks great to me.
I'd just rename the test file to make invalid stand out.

mlir/test/Dialect/Linalg/conv_interface.mlir
1 ↗(On Diff #373444)

s/conv_interface.mlir/invalid-conv-interface.mlir ?

Rename test file and rebase.

This revision was automatically updated to reflect the committed changes.