This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Conv {1,2,3}D ops defined with TC syntax
ClosedPublic

Authored by limo1996 on Jul 27 2020, 3:12 AM.

Details

Summary

Replaced definition of named ND ConvOps with tensor comprehension
syntax which reduces boilerplate code significantly. Furthermore,
new ops to support TF convolutions added (without strides and dilations).

Diff Detail

Event Timeline

limo1996 created this revision.Jul 27 2020, 3:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2020, 3:12 AM
ftynse added inline comments.Jul 27 2020, 3:47 AM
mlir/include/mlir/Dialect/Linalg/IR/LinalgNamedStructuredOpsSpec.tc
22

Can we name these as conv_2d_<layout> ? I understand it can be seen as redundant we the layout, but it's much easier to grok with dimensionality included.

limo1996 marked an inline comment as done.Jul 27 2020, 4:59 AM
limo1996 added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgNamedStructuredOpsSpec.tc
22

Yeah sure..

I wonder how to support padding, strides and dilatation.. I was thinking of passing additional arguments (1D arrays) for dilatations and strides. However we want to probably have them as attributes. But if there is some wrapper generic ConvOp such that they are not exposed we might tolerate it right? With padding in TF there are 2 "algorithms": SAME or VALID so we can have op types for those as well even though it would double their count and we would have less flexibility..

limo1996 updated this revision to Diff 280881.Jul 27 2020, 5:53 AM

Comment of Alex addressed + pure Conv{1,2,3}D ops added

limo1996 updated this revision to Diff 281127.Jul 28 2020, 12:09 AM

Removed unnecessary Conv 1,2,3D named ops

limo1996 marked an inline comment as done.Jul 28 2020, 12:09 AM
limo1996 retitled this revision from [WIP][mlir] Conv ops defined with TC syntax to [mlir][Linalg] Conv {1,2,3}D ops defined with TC syntax.Jul 28 2020, 12:12 AM
limo1996 edited the summary of this revision. (Show Details)
nicolasvasilache accepted this revision.Jul 29 2020, 6:09 AM

Nice use of the language :) !

mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
1321

Note: bonus points to auto-generate all these when we extend the parser.

mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
686

Same here re auto-generation and parser.

This revision is now accepted and ready to land.Jul 29 2020, 6:09 AM
limo1996 marked 2 inline comments as done.Jul 29 2020, 6:20 AM
limo1996 added inline comments.
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
1321

Yeah :) Do you keep track of those TODO items somewhere?

limo1996 marked an inline comment as done.Jul 29 2020, 7:27 AM

Could you please rebase?

Could you please rebase?

But it depends on the previous commit.. Can you be please more concrete? Thank you.

Could you please rebase?

But it depends on the previous commit.. Can you be please more concrete? Thank you.

If you want your diff landed, rebase _always_ means rebase on the current head of the main repository. LLVM maintains linear revision history, which means any new diff is applied on top of the current head. Currently, your diff does not apply to the head.

If by "previous commit" you mean D83879, it has landed some time ago so the current head includes those changes.

Could you please rebase?

But it depends on the previous commit.. Can you be please more concrete? Thank you.

If you want your diff landed, rebase _always_ means rebase on the current head of the main repository. LLVM maintains linear revision history, which means any new diff is applied on top of the current head. Currently, your diff does not apply to the head.

If by "previous commit" you mean D83879, it has landed some time ago so the current head includes those changes.

Ahh yeah.. Makes perfect sense to me. Thank you!

This revision was automatically updated to reflect the committed changes.