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] Simplify consecutive transpose/layout_transform #7656

Merged
merged 5 commits into from Mar 16, 2021

Conversation

comaniac
Copy link
Contributor

This PR adds a simplify pattern to the SImplifyExpr pass. The pattern simplifies back-to-back transpose and layout_transform ops, which can be introduced by Relay frontends or ConvertLayout pass.

cc @mbrookhart @icemelon9 @anijain2305

@ANSHUMAN87
Copy link
Contributor

Thanks @comaniac for the PR! I was also working on similar approach. This solves my case too.

@ANSHUMAN87
Copy link
Contributor

Overall pretty good for me.
I just have 2 points which needs clarification:
1:> When the axes has negative values, we should handle it.
2:> Now we are just looking at the start of the chain of transpose, i suggest we look at intermediate residue as well.

Please let me know in case my points are not clear. Thanks!

@comaniac
Copy link
Contributor Author

Thanks for the comments.

1:> When the axes has negative values, we should handle it.

Good point. I'll add the support for it later.

2:> Now we are just looking at the start of the chain of transpose, i suggest we look at intermediate residue as well.

Could you provide an example of this case for better illustration? Thanks.

@ANSHUMAN87
Copy link
Contributor

ANSHUMAN87 commented Mar 13, 2021

2:> Now we are just looking at the start of the chain of transpose, i suggest we look at intermediate residue as well.

Could you provide an example of this case for better illustration? Thanks.

For example we have below case:
Expression:
y = nn.conv(x)
y = transpose(y, axes=0, 3, 1, 2)
y = transpose(y)
y = transpose(y)

Expected transformation:
y = nn.conv(x)
y = transpose(y, axes=0, 3, 1, 2)

Above such cases can be multiple in a sequence as well. So we need to have a solution which can handle multiple simplification in one chain. Please let me know, in case i have not clearly expressed. Thanks!

I understand we can have these points covered in follow up PRs as well. If we are not planning to do it in this PR. Suggest to add TODOs for these points in the source code, which will help in future.

@comaniac
Copy link
Contributor Author

It seems to me that the current pattern already covers this case? You can refer to the test cases I added, which have a ReLU followed by transposes. Is this what you want?

@ANSHUMAN87
Copy link
Contributor

ANSHUMAN87 commented Mar 14, 2021

It seems to me that the current pattern already covers this case? You can refer to the test cases I added, which have a ReLU followed by transposes. Is this what you want?

Thanks @comaniac for clarification. It is indeed supported.
Can we add 1 more test case as below, it will help reviewer to understand the flow is supported:


    def before4():
        x = relay.var("x", shape=(1, 3, 224, 224), dtype="float32")  # NCHW
        y = relay.nn.relu(x)
        y = relay.transpose(y)  # Reverse
        y = relay.transpose(y)  # Reverse
        y = relay.transpose(y, axes=[0, 2, 3, 1])
        y = relay.transpose(y)  # Reverse
        y = relay.transpose(y)  # Reverse
        return relay.Function([x], y)

    def expected4():
        x = relay.var("x", shape=(1, 3, 224, 224), dtype="float32")  # NCHW
        y = relay.nn.relu(x)
        y = relay.transpose(y, axes=[0, 2, 3, 1])
        return relay.Function([x], y)

@comaniac
Copy link
Contributor Author

@ANSHUMAN87 comments are addressed.
Also cc @mbrookhart @icemelon9 @anijain2305

Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry for the delay, I was out the end of last week.

One possible extension would be to try to combine this with the simplify reshape above and target patterns like reshape->transpose->reshape or transpose->reshape->transpose

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 @comaniac!

@comaniac
Copy link
Contributor Author

One possible extension would be to try to combine this with the simplify reshape above and target patterns like reshape->transpose->reshape or transpose->reshape->transpose

That's an interesting point and I just thought about it. However, reshape and transpose cannot be equivalent in any cases, because reshape just changes the definition of accessing the tensor; while transpose reorganizes the element order in memory. Even we combine them to the same patterm, we still need to deal with each case separately and result in no code reuse. Please let me know if I missed something. Thanks.

@mbrookhart
Copy link
Contributor

You're right. I think a set of modular patterns is probably best

@comaniac comaniac merged commit 7f96986 into apache:main Mar 16, 2021
@comaniac
Copy link
Contributor Author

Thanks @mbrookhart @ANSHUMAN87

@comaniac comaniac deleted the simplify_transpsoe branch March 16, 2021 01:16
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
…#7656)

* [Relay][Pass] Simplify consecutive transpose/layout_transform

* lint

* fix

* support negative

* comment
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request May 11, 2021
…#7656)

* [Relay][Pass] Simplify consecutive transpose/layout_transform

* lint

* fix

* support negative

* comment
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.

None yet

3 participants