Skip to content

[Relay][Pass] Update SimplifyTranspose to correctly simplify rank changing layout transforms#7807

Merged
comaniac merged 5 commits into
apache:mainfrom
csullivan:simplify_transpose_with_rank_change
May 4, 2021
Merged

[Relay][Pass] Update SimplifyTranspose to correctly simplify rank changing layout transforms#7807
comaniac merged 5 commits into
apache:mainfrom
csullivan:simplify_transpose_with_rank_change

Conversation

@csullivan

@csullivan csullivan commented Apr 7, 2021

Copy link
Copy Markdown
Contributor

This PR adds support to SimplifyTranspose to simplify consecutive transpose<->layout_transform operations when the layout transform involves a rank expansion or contraction. The added unit test fails on main because the current SimplifyTranspose only doesn't consider the case when the layout transform and transpose may act on tensors of different rank. As an example,

NCHW4c -> LayoutTransform -> NCHW -> Transpose -> NHWC

now simplifies to

NCHW4c -> LayoutTransform -> NHWC.

The reverse, rank expansion, is also supported.

@csullivan

csullivan commented Apr 7, 2021

Copy link
Copy Markdown
Contributor Author

cc @comaniac @ANSHUMAN87 @mbrookhart

comaniac
comaniac reviewed Apr 7, 2021
View reviewed changes

@comaniac comaniac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! Overall LGTM. Please add more comments to help future maintaners catch up the logic easily.

Comment thread src/relay/transforms/type_infer.cc
Comment thread src/relay/transforms/simplify_expr.cc Outdated
Comment thread src/relay/transforms/simplify_expr.cc
Comment thread src/relay/transforms/simplify_expr.cc Outdated
Comment thread src/relay/transforms/simplify_expr.cc Outdated
Comment thread src/relay/transforms/simplify_expr.cc Outdated
ANSHUMAN87
ANSHUMAN87 reviewed Apr 8, 2021
View reviewed changes

@ANSHUMAN87 ANSHUMAN87 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @csullivan for the PR!
Please find some comments.
Overall LGTM!

Comment thread src/relay/transforms/simplify_expr.cc
Comment thread src/relay/transforms/simplify_expr.cc Outdated
Comment thread tests/python/relay/test_pass_simplify_expr.py
@csullivan

csullivan commented Apr 21, 2021

Copy link
Copy Markdown
Contributor Author

Thanks @comaniac @ANSHUMAN87! I've extended the support for various rank changing layout transform + transpose patterns. Please see the additional tests for an overview and let me know other change requests you may have.

ANSHUMAN87
ANSHUMAN87 reviewed Apr 21, 2021
View reviewed changes
Comment thread tests/python/relay/test_pass_simplify_expr.py Outdated
ANSHUMAN87
ANSHUMAN87 reviewed Apr 21, 2021
View reviewed changes
Comment thread tests/python/relay/test_pass_simplify_expr.py Outdated
comaniac
comaniac approved these changes Apr 21, 2021
View reviewed changes

@comaniac comaniac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One bonus suggestion is it would be great to have one comment on the top of each new added test cases to brief explain what's testing.

Comment thread tests/python/relay/test_pass_simplify_expr.py Outdated
@mbrookhart

mbrookhart commented May 3, 2021

Copy link
Copy Markdown
Contributor

@csullivan Thanks for the contribution. What's the status on this? I looks like it has a lint error.

ANSHUMAN87
ANSHUMAN87 approved these changes May 4, 2021
View reviewed changes

@ANSHUMAN87 ANSHUMAN87 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great now! 👍

@comaniac comaniac merged commit 396a09e into apache:main May 4, 2021
@comaniac

comaniac commented May 4, 2021

Copy link
Copy Markdown
Contributor

Thanks @csullivan @ANSHUMAN87

umangyadav pushed a commit to umangyadav/tvm that referenced this pull request May 5, 2021
@csullivan @umangyadav
[Relay][Pass] Update SimplifyTranspose to correctly simplify rank cha…
1560f7f 
…nging layout transforms (apache#7807)
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
@csullivan
[Relay][Pass] Update SimplifyTranspose to correctly simplify rank cha…
2b75485 
…nging layout transforms (apache#7807)
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
@csullivan
[Relay][Pass] Update SimplifyTranspose to correctly simplify rank cha…
86b74ff 
…nging layout transforms (apache#7807)
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
@csullivan
[Relay][Pass] Update SimplifyTranspose to correctly simplify rank cha…
ad220a8 
…nging layout transforms (apache#7807)
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request May 11, 2021
@csullivan @trevor-m
[Relay][Pass] Update SimplifyTranspose to correctly simplify rank cha…
76494dd 
…nging layout transforms (apache#7807)
@junrushao junrushao mentioned this pull request Nov 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@comaniac comaniac comaniac approved these changes
+1 more reviewer
@ANSHUMAN87 ANSHUMAN87 ANSHUMAN87 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@csullivan @mbrookhart @comaniac @ANSHUMAN87