This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Add conv ops with TF definition.
ClosedPublic

Authored by hanchung on Feb 4 2021, 7:34 AM.

Details

Summary

The dimension order of a filter in tensorflow is
[filter_height, filter_width, in_channels, out_channels], which is different
from current definition. The current definition follows TOSA spec. Add TF
version conv ops to .tc, so we do not have to insert a transpose op around a
conv op.

Diff Detail

Event Timeline

hanchung created this revision.Feb 4 2021, 7:34 AM
hanchung requested review of this revision.Feb 4 2021, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2021, 7:34 AM

Is it possible to mark mlir files were copied from other files?
I actually copied it from another file and only modified the dimension order of filters.

Is it possible to mark mlir files were copied from other files?
I actually copied it from another file and only modified the dimension order of filters.

I did some research and look like not possible. The information is obtained from git, and there is no git cp command. If the old file exists, it will treat it as a new file, not renamed or branched file.

antiagainst requested changes to this revision.Feb 8 2021, 6:00 AM

Thanks Hanhan for working on this!!

mlir/include/mlir/Dialect/Linalg/IR/LinalgNamedStructuredOpsSpec.tc
111

Instead of using tf as a prefix to indicate the semantics, what about just being explicit in the name? For example, we can have conv_2d_input_nhwc_filter_hwcf? This does make the op name a bit lengthy but I thin it's worth it given the the clarity. One does not need to open TensorFlow documentation to understand the semantics here.

113

Can you also add documentation for these ops? You can see https://reviews.llvm.org/D94966 as an example.

This revision now requires changes to proceed.Feb 8 2021, 6:00 AM
hanchung updated this revision to Diff 322178.Feb 8 2021, 11:22 AM

rename ops and add docs.

hanchung marked 2 inline comments as done.Feb 8 2021, 11:23 AM
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
563

How much are you relying on these vectorization patterns?
I am asking because they essentially need to go away and be redone with a better vectorization strategy and more general calling mechanism (i.e. a linalg op interface).

hanchung added inline comments.Feb 9 2021, 4:20 AM
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
563

I think we don't really rely on these patterns in IREE. I added these patterns because I looked into other conv ops usage and have a similar change with them. We can remove it if you think it's better.

antiagainst accepted this revision.Feb 9 2021, 6:34 AM

LGTM. Just additional requests regarding documentation for indexing maps and tests. It would be nice if you can add some additional tests in generalize-named-ops.mlir and named-ops.mlir to verify the semantics and IR format.

mlir/include/mlir/Dialect/Linalg/IR/LinalgNamedStructuredOpsSpec.tc
113

Also the indexing maps' dimension order? That is typically very important to know but very obscure and hidden entirely.

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
563

Yeah. We aren't relying on the vectorization in core at the moment. :)

This revision is now accepted and ready to land.Feb 9 2021, 6:34 AM
hanchung updated this revision to Diff 322758.Feb 10 2021, 10:59 AM

rebase and address comments

antiagainst accepted this revision.Feb 10 2021, 1:15 PM
antiagainst added inline comments.
mlir/test/Dialect/Linalg/generalize-named-ops.mlir
185

It would be nice to turn one of the tests to have non-1 dilations and non-1 strides to make sure the affine_map is good for them.

hanchung marked an inline comment as done.Feb 10 2021, 10:00 PM
hanchung added inline comments.
mlir/test/Dialect/Linalg/generalize-named-ops.mlir
185

good point, done!

hanchung updated this revision to Diff 322906.Feb 10 2021, 10:00 PM
hanchung marked an inline comment as done.

modify tests to cover strides and dilation

This revision was landed with ongoing or failed builds.Feb 10 2021, 11:00 PM
This revision was automatically updated to reflect the committed changes.