Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
May 4, 2021
Merged

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

merged 5 commits into from
May 4, 2021

Conversation

csullivan
Copy link
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
Copy link
Contributor Author

cc @comaniac @ANSHUMAN87 @mbrookhart

Copy link
Contributor

@comaniac comaniac left a comment

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.

src/relay/transforms/type_infer.cc Outdated Show resolved Hide resolved
src/relay/transforms/simplify_expr.cc Outdated Show resolved Hide resolved
src/relay/transforms/simplify_expr.cc Show resolved Hide resolved
src/relay/transforms/simplify_expr.cc Outdated Show resolved Hide resolved
src/relay/transforms/simplify_expr.cc Outdated Show resolved Hide resolved
src/relay/transforms/simplify_expr.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@ANSHUMAN87 ANSHUMAN87 left a comment

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!

src/relay/transforms/simplify_expr.cc Show resolved Hide resolved
src/relay/transforms/simplify_expr.cc Outdated Show resolved Hide resolved
tests/python/relay/test_pass_simplify_expr.py Show resolved Hide resolved
@csullivan
Copy link
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.

Copy link
Contributor

@comaniac comaniac left a comment

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.

tests/python/relay/test_pass_simplify_expr.py Outdated Show resolved Hide resolved
@mbrookhart
Copy link
Contributor

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

Copy link
Contributor

@ANSHUMAN87 ANSHUMAN87 left a comment

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
Copy link
Contributor

comaniac commented May 4, 2021

Thanks @csullivan @ANSHUMAN87

umangyadav pushed a commit to umangyadav/tvm that referenced this pull request May 5, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants