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][Legalize] Legalize conv2d_transpose for NHWC #4399

Merged
merged 1 commit into from
Nov 23, 2019

Conversation

apivovarov
Copy link
Contributor

@apivovarov apivovarov commented Nov 22, 2019

This PR Legalizes conv2d_transpose for NHWC.
I need to compile palm_detection.tflite model from mediapipe.
This model uses TRANSPOSE_CONV op. TFLite models uses NHWC data layout only.
In order to add TRANSPOSE_CONV to TFLite frontend I need to Legalize conv2d_transpose for NHWC first.

The fact that weights layout and kernel_layout should have flipped IO is very confusing.
I added comments about it in code.

@apivovarov apivovarov force-pushed the conv2d_transpose_legalize branch 4 times, most recently from cf65f02 to 717ac15 Compare November 22, 2019 09:12
@kevinthesun
Copy link
Contributor

@anijain2305 Can you take a look?

@apivovarov
Copy link
Contributor Author

@merrymercy Can you have a look as well?
All checks have passed

Comment on lines +143 to +146
elif kernel_layout == 'OIHW':
# input kernel layout is swapped to IOHW
# output kernel layout will be IOHW
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

How about just removing this code?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, this is needed, otherwise legalization wont happen

# output kernel layout will be IOHW
pass
else:
raise ValueError("Invalid kernel_layout {}".format(kernel_layout))
Copy link
Contributor

Choose a reason for hiding this comment

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

We can do return None here. That is if some topi schedule supports this particular combination of layouts, it will fallback to that.

Return None basically means legalize does not transform anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@anijain2305 anijain2305 left a comment

Choose a reason for hiding this comment

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

LGTM overall. Minor changes.

@apivovarov apivovarov force-pushed the conv2d_transpose_legalize branch from 717ac15 to 4efed1a Compare November 22, 2019 23:34
@apivovarov apivovarov force-pushed the conv2d_transpose_legalize branch from 4efed1a to 2d1bccf Compare November 22, 2019 23:36
Copy link
Contributor

@kevinthesun kevinthesun left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinthesun kevinthesun merged commit 9049d66 into apache:master Nov 23, 2019
@kevinthesun
Copy link
Contributor

Thanks @apivovarov

zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Nov 26, 2019
yongwww pushed a commit to neo-ai/tvm that referenced this pull request Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants