Add an interface for convolution ops. Currently does some basic
verification and returns a charecterization of the loop dimensions.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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) |
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. |
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. | |
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? |
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? |
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
- Add negative tests
- Split methods that return loop characterization into separate patch
- Add better description
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? |
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 | s/conv_interface.mlir/invalid-conv-interface.mlir ? |
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.