Skip to content

[Relay][Frontend][BugFix] Fixed relay grouped convolution selection #3020

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

Closed
Wheest wants to merge 1 commit into from
Closed

[Relay][Frontend][BugFix] Fixed relay grouped convolution selection #3020

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions python/tvm/relay/op/nn/_nn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,20 +125,22 @@ def schedule_conv2d(attrs, outs, target):
groups = attrs.groups
layout = attrs.data_layout
kernel_layout = attrs.kernel_layout
channels = attrs.channels #TODO should be input channels
Copy link
Member

@vinx13 vinx13 Apr 15, 2019

Choose a reason for hiding this comment

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

it is not safe to use attrs.channels since this attr is optinoal

Copy link
Contributor Author

@Wheest Wheest Apr 15, 2019

Choose a reason for hiding this comment

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

In that case, the in_channels needs to be exposed to the function. Either by being added to the attrs (defined at nn.h:51), or passing it as an extra argument (such as the tinfo suggestion) by every caller of it in the codebase.

Would there ever be any cases when the in_channels would be indeterminate when this function is called?

Copy link
Member

@vinx13 vinx13 Apr 15, 2019

Choose a reason for hiding this comment

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

in_channels should be available from the shape of input / weight after type infer

Copy link
Member

@vinx13 vinx13 Apr 23, 2019

Choose a reason for hiding this comment

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

@Wheest As suggested in #3070 (comment), you can get input channels via outs[0].op.attrs["workload"]. Can you check if this work for you?

Copy link
Contributor Author

@Wheest Wheest Apr 23, 2019

Choose a reason for hiding this comment

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

Thanks for the insight @vinx13, I would never have groked all of the info that outs stores. I can confirm that setting in_channels = outs[0].op.attrs['workload'][1][1] works for groups==1 and groups==in_channels. However, interestingly when 1<groups<in_channels, it seems that outs[0].op.attrs.items is empty. This suggests that whatever calls schedule_conv2d does not set outs[0].op.attrs.items correctly when using grouped convolutions. Am investigating why.

Copy link
Contributor

@kumasento kumasento Apr 23, 2019

Choose a reason for hiding this comment

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

Hi @Wheest, I did notice such circumstances in my PR. Please let me know if you have any new discovery :)

Copy link
Member

@vinx13 vinx13 Apr 23, 2019

Choose a reason for hiding this comment

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

@Wheest FYI workload is set here
This is probably because group_conv2d_nchw is not registered as autotvm template

Copy link
Contributor

@kumasento kumasento Apr 23, 2019

Choose a reason for hiding this comment

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

@vinx13 You might be correct - depthwise_conv2d_nchw has been properly registered while group_conv2d_nchw has not. I'm working on fixing that

Copy link
Contributor Author

@Wheest Wheest Apr 23, 2019

Choose a reason for hiding this comment

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

Thanks, I'll hold until then. Why would autotvm templates be used in this workflow? I thought that the process is:

  1. Define model in using high level Relay API (with the bug in this PR happening here).
  2. Generate model description in Relay IR
  3. Compile model to chosen backend.
    As I understand it, you might use autotvm at step 3. to tune and optimise the program. But I thought of it a "separate tool" in the tvm suite, not used in step 1 or 2.

Copy link
Member

@vinx13 vinx13 Apr 23, 2019

Choose a reason for hiding this comment

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

Actually we only need extra attributes when calling schedule. One way is to add attributes needed in topi compute. Autotvm already did this, but we can also register them manually. For the group conv case, we can add an attribute when calling tvm.compute

with target:
if groups == 1 and layout == "NCHW":
return topi.generic.schedule_conv2d_nchw(outs)
if groups == 1 and layout == "NCHW4c":
return topi.generic.schedule_conv2d_nchw(outs)
if groups == 1 and layout == "NHWC":
return topi.generic.schedule_conv2d_nhwc(outs)
if groups != 1:
if groups == channels:
if layout == "NCHW":
# TODO(leyuan, merrymercy, Huyuwei): fold depthwise topi into conv2d.
return topi.generic.schedule_depthwise_conv2d_nchw(outs)
if layout == "NHWC" and kernel_layout == "HWOI":
return topi.generic.schedule_depthwise_conv2d_nhwc(outs)
if layout == "NCHW4c":
if groups > 1:
if layout == "NCHW":
return topi.generic.schedule_group_conv2d_nchw(outs)
raise ValueError("No compatible schedule")

Expand Down