Skip to content

[Relay] Conv2D padding representation - #4787

Merged
kevinthesun merged 10 commits into
apache:masterfrom
zxy844288792:padding
Feb 5, 2020
Merged

[Relay] Conv2D padding representation#4787
kevinthesun merged 10 commits into
apache:masterfrom
zxy844288792:padding

Conversation

@zxy844288792

@zxy844288792zxy844288792 commented Jan 29, 2020

Copy link
Copy Markdown

As discussed here: https://discuss.tvm.ai/t/rfc-conv2d-padding-representation/5394. We agree we should enforce topi.nn.conv2d to accept 4-way padding. We also should have relay.nn.conv2d legalizes the padding to 4-way.

get_pad_tuple is from topi util.py. I deleted some unuseful code and reuse it for relay.op.nn.conv2d.

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

comaniac
comaniac requested changes Jan 29, 2020
View reviewed changes

@comaniaccomaniac 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.

Leave some comments.

Also, I found that now we have get_pad_tuple in both Relay and TOPI. I feel that we should remove the TOPI one to enforce the TOPI conv2d input type. @icemelon9 @yzhliu do you guys have any suggestions?

Comment threadpython/tvm/relay/op/nn/nn.py Outdated
Comment threadpython/tvm/relay/op/nn/util.py Outdated
Comment threadpython/tvm/relay/op/nn/nn.py Outdated
Comment threadtopi/python/topi/nn/conv2d.py Outdated
comaniac
comaniac reviewed Jan 30, 2020
View reviewed changes
Comment threadtopi/python/topi/nn/conv2d.py Outdated
4-D with shape [batch, out_channel, out_height, out_width]
"""
#only accepts 4-way padding
assert len(padding) == 4, "only accepts 4-way padding"

@comaniaccomaniacJan 30, 2020

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.

I'm afraid that you have to add this assertion to all conv2d compute functions. Specifically, all conv2d functions with @autotvm.register_topi_compute(nn.conv2d, ...) decorator should have this assertion. @icemelon9 could you help confirm?

@icemelonicemelonJan 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, @comaniac is correct. Probably directly contribute to #4644? 😄

@comaniaccomaniacJan 30, 2020

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.

@zxy844288792 how about we revert this file first and add a note in python/tvm/relay/op/nn/nn.py to remind us to add it back when #4644 is merged?

@zxy844288792zxy844288792Jan 30, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@comaniac Agree

@comaniac

comaniac commented Jan 31, 2020

Copy link
Copy Markdown
Contributor

LGTM.
@icemelon9 @yzhliu could you help review and merge? Thanks.

icemelon
icemelon reviewed Jan 31, 2020
View reviewed changes
Comment threadpython/tvm/relay/op/nn/util.py Outdated
@icemelon

icemelon commented Jan 31, 2020

Copy link
Copy Markdown
Member

You should also add get_pad_tuple2d to these contrib conv2d ops, e.g., contrib_conv2d_nchwc

@zxy844288792

zxy844288792 commented Feb 1, 2020

Copy link
Copy Markdown
Author

You should also add get_pad_tuple2d to these contrib conv2d ops, e.g., contrib_conv2d_nchwc

I have added get_pad_tuple2d for contrib_conv2d_nchwc and contrib_conv2d_nchwc_int8, contrib_conv2d_winograd_without_weight_transform and contrib_conv2d_winograd_nnpack_without_weight_transform

@FrozenGeneFrozenGene mentioned this pull request Feb 3, 2020
Merged
kevinthesun
kevinthesun approved these changes Feb 3, 2020
View reviewed changes

@kevinthesunkevinthesun 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

@icemelon

icemelon commented Feb 4, 2020

Copy link
Copy Markdown
Member

@zxy844288792 I see that you didn't modify the contrib_depthwise_conv2d_nchwc. Should we also use get_pad_tuple2d for it?

@kevinthesun
kevinthesun merged commit 5ea4f0d into apache:masterFeb 5, 2020
@kevinthesun

kevinthesun commented Feb 5, 2020

Copy link
Copy Markdown
Contributor

Thanks @zxy844288792 @icemelon9 @comaniac

anijain2305 pushed a commit to anijain2305/tvm that referenced this pull request Feb 10, 2020
Xingyu ZhouUbuntu
[Relay] Conv2D padding representation (apache#4787)
a77d77d
* enforce 4-way padding
* add util with get_pad_tuple
* delete unnecessary arguments
* fix lint
* add container.Array case
* fix cudnn conv2d asymmetric padding logic
* rename get_pad_tuple to get_pad_tuple2d
* revert change for topi/python/topi/nn/conv2d.py
* add get_pad_tuple2d for several contrib conv2d ops
* add get_pad_tuple2d for all conv2d ops
@masahimasahi mentioned this pull request Feb 12, 2020
Merged
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 26, 2020
Xingyu Zhou@alexwong
[Relay] Conv2D padding representation (apache#4787)
aeaae65
* enforce 4-way padding
* add util with get_pad_tuple
* delete unnecessary arguments
* fix lint
* add container.Array case
* fix cudnn conv2d asymmetric padding logic
* rename get_pad_tuple to get_pad_tuple2d
* revert change for topi/python/topi/nn/conv2d.py
* add get_pad_tuple2d for several contrib conv2d ops
* add get_pad_tuple2d for all conv2d ops
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 28, 2020
Xingyu Zhou@alexwong
[Relay] Conv2D padding representation (apache#4787)
a17742d
* enforce 4-way padding
* add util with get_pad_tuple
* delete unnecessary arguments
* fix lint
* add container.Array case
* fix cudnn conv2d asymmetric padding logic
* rename get_pad_tuple to get_pad_tuple2d
* revert change for topi/python/topi/nn/conv2d.py
* add get_pad_tuple2d for several contrib conv2d ops
* add get_pad_tuple2d for all conv2d ops
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2020
Xingyu Zhou@zhiics
[Relay] Conv2D padding representation (apache#4787)
f7c2fd6
* enforce 4-way padding
* add util with get_pad_tuple
* delete unnecessary arguments
* fix lint
* add container.Array case
* fix cudnn conv2d asymmetric padding logic
* rename get_pad_tuple to get_pad_tuple2d
* revert change for topi/python/topi/nn/conv2d.py
* add get_pad_tuple2d for several contrib conv2d ops
* add get_pad_tuple2d for all conv2d ops
@ZihengJiangZihengJiang mentioned this pull request Sep 17, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@icemelonicemelonicemelon approved these changes
@comaniaccomaniaccomaniac approved these changes
@kevinthesunkevinthesunkevinthesun approved these changes

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

@zxy844288792@comaniac@icemelon@kevinthesun