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.

Sign up for GitHub

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

Jump to bottom

[AlterLayout] NCHWc upsampling, fix depthwise conv #2806

Merged
merged 2 commits into from
Mar 20, 2019
Merged

[AlterLayout] NCHWc upsampling, fix depthwise conv #2806

merged 2 commits into from
Mar 20, 2019

Conversation

antinucleon
Copy link
Contributor

Support NCHWc upsamping.

@antinucleon antinucleon changed the title [AlterLayout] NCHW upsampling [AlterLayout] NCHWc upsampling Mar 13, 2019
@antinucleon antinucleon requested a review from yzhliu March 14, 2019 03:20
ajtulloch
ajtulloch reviewed Mar 14, 2019
View reviewed changes
src/relay/op/nn/upsampling.cc
const Array<Layout>& old_in_layouts,
const Array<Array<IndexExpr>> &old_in_shapes) {
// NOTE: Discard "const" qualifier here.
T *params = const_cast<T*>(attrs.as<T>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why this is necessary? It's quite counterintuitive why this kind of thing is needed, since the desired behaviour is essentially "if input layout is NCHWc, then expect input in NCHWc, perform compute, and output in NCHWc" right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is same to: https://github.com/dmlc/tvm/blob/master/src/relay/op/nn/pooling.cc#L22

In order to invoke AlterLayout Pass, we need FInferCorrectLayout attr, which is this function. This is part of logic of current AlterLayoutPass.

yzhliu
yzhliu reviewed Mar 14, 2019
View reviewed changes
Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

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

Can we add some test cases?

src/relay/op/nn/upsampling.cc Outdated Show resolved Hide resolved
topi/include/topi/image/resize.h Outdated Show resolved Hide resolved
src/relay/op/nn/upsampling.cc Show resolved Hide resolved
@antinucleon antinucleon force-pushed the hack_layout branch 2 times, most recently from 6fb25d8 to f569e03 Compare March 17, 2019 06:35
@antinucleon
Copy link
Contributor Author

Can we add some test cases?

Added

@antinucleon antinucleon changed the title [AlterLayout] NCHWc upsampling [AlterLayout] NCHWc upsampling, fix depthwise conv Mar 17, 2019
@antinucleon antinucleon mentioned this pull request Mar 17, 2019
Closed
@antinucleon antinucleon changed the title [AlterLayout] NCHWc upsampling, fix depthwise conv [WIP][AlterLayout] NCHWc upsampling, fix depthwise conv Mar 17, 2019
@antinucleon antinucleon force-pushed the hack_layout branch 4 times, most recently from bd259cc to 5bcd6a0 Compare March 18, 2019 02:41
@antinucleon antinucleon changed the title [WIP][AlterLayout] NCHWc upsampling, fix depthwise conv [AlterLayout] NCHWc upsampling, fix depthwise conv Mar 18, 2019
@antinucleon antinucleon force-pushed the hack_layout branch 2 times, most recently from a0b4515 to 03c8f5e Compare March 18, 2019 06:21
@antinucleon
Copy link
Contributor Author

@yzhliu For the failed test, I am able to pass on local machine. Could you help take a look?

@yzhliu
Copy link
Member

yzhliu commented Mar 18, 2019

I guess the failed test is not for cpu?

@antinucleon
Copy link
Contributor Author

I guess the failed test is not for cpu?

Not really: https://github.com/dmlc/tvm/blob/master/topi/tests/python/test_topi_depthwise_conv2d.py#L311

yzhliu
yzhliu approved these changes Mar 19, 2019
View reviewed changes
Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

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

We can merge as long as CI is happy.

topi/python/topi/x86/conv2d.py Outdated Show resolved Hide resolved
@antinucleon
Copy link
Contributor Author

@tqchen Docker test issue appears again. Could you clean up cache?

@tqchen
Copy link
Member

tqchen commented Mar 19, 2019

I agree with @ajtulloch that the AlterLayout function can cause some confusion, perhaps we could do an RFC discussion about the mechanism and clarify more in the comments. I am also not sure why const discard is necessarily here @yzhliu @merrymercy can you explain? Of course, the discussion does not have to block this PR since it fixes some problem

@tqchen
Copy link
Member

tqchen commented Mar 19, 2019

https://discuss.tvm.ai/t/discuss-and-documentation-of-alterlayout-pass/1944

@antinucleon antinucleon force-pushed the hack_layout branch 2 times, most recently from 7d3e4ee to ee141a2 Compare March 19, 2019 22:42
@Laurawly Laurawly merged commit f81e287 into apache:master Mar 20, 2019
@Laurawly
Copy link
Contributor

Thanks @antinucleon @yzhliu @ajtulloch @tqchen , the CI has passed, so this is merged.

Laurawly pushed a commit to Laurawly/tvm-1 that referenced this pull request Mar 22, 2019
@antinucleon @Laurawly
[AlterLayout] NCHWc upsampling, fix depthwise conv (apache#2806)
b34849c 
* [AlterLayout] NCHW upsampling

* [Relay][Pass] Fix Depthwise AlterLayout
wweic pushed a commit to wweic/tvm that referenced this pull request Mar 24, 2019
@antinucleon @wweic
[AlterLayout] NCHWc upsampling, fix depthwise conv (apache#2806)
6cdf90c 
* [AlterLayout] NCHW upsampling

* [Relay][Pass] Fix Depthwise AlterLayout
wweic pushed a commit to neo-ai/tvm that referenced this pull request Mar 24, 2019
@antinucleon @wweic
[AlterLayout] NCHWc upsampling, fix depthwise conv (apache#2806)
4d32dc8 
* [AlterLayout] NCHW upsampling

* [Relay][Pass] Fix Depthwise AlterLayout
@tqchen tqchen mentioned this pull request Nov 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajtulloch ajtulloch ajtulloch left review comments

@yzhliu yzhliu yzhliu approved these changes

Assignees

@yzhliu yzhliu

Labels
status: need review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@antinucleon @yzhliu @tqchen @Laurawly @ajtulloch