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

[Bug] Conv3Dtranspose default kernel layout should be IODHW #14326

Closed
rebel-jangys opened this issue Mar 17, 2023 · 2 comments
Closed

[Bug] Conv3Dtranspose default kernel layout should be IODHW #14326

rebel-jangys opened this issue Mar 17, 2023 · 2 comments
Labels
needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it status: help wanted type: bug

Comments

@rebel-jangys
Copy link
Contributor

rebel-jangys commented Mar 17, 2023

Hi, I found some bugs in TVM relay type relation of conv3dtranspose.

static const Layout kOIDHW("OIDHW");

I believe the above line should be set to the IODHW layout, since the below conv3dtranspose kernel is made with the IODHW layout.

Array<IndexExpr> wshape({dshape_ncdhw[1], indexdiv(param->channels, param->groups),
param->kernel_size[0], param->kernel_size[1], param->kernel_size[2]});

Additionally, this line should use 'indexdiv(param->channels, param->groups)' like Conv2dTransposeRel instead of dividing the input channels by the number of groups.

ICHECK(reporter->AssertEQ(indexdiv(dshape_ncdhw[1], param->groups), wshape[0]));

The weight of this test is treated as 'OIDHW' layout(the default kernel layout for conv3dtranspose is "OIDHW") despite having 10 input channels and 12 output channels.

x = relay.var("x", relay.TensorType((n, c, d, h, w), "int8"))
w = relay.var("w", relay.TensorType((10, 12, 3, 3, 3), "int8"))
y = relay.nn.conv3d_transpose(x, w, out_dtype="int32")

Correct me if I'm wrong. Once again, thanks for your wonderful framework!

@rebel-jangys rebel-jangys added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it type: bug labels Mar 17, 2023
@masahi
Copy link
Member

masahi commented Mar 20, 2023

I believe the above line should be set to the IODHW

You are right.

Additionally, this line should use 'indexdiv(param->channels, param->groups)'

I think this line tries to compare the input channels. So the correct fix would be to remove the indexdiv.

We should also fix the followings:
https://github.com/apache/tvm/blob/main/include/tvm/relay/attrs/nn.h#L591
https://github.com/apache/tvm/blob/main/include/tvm/relay/attrs/nn.h#L433

kernel_layout="OIDHW",

Can you send a PR to fix them?

@masahi masahi changed the title [Bug] Conv3Dtranspose type [Bug] Conv3Dtranspose default kernel layout should be IODHW Mar 20, 2023
@rebel-jangys
Copy link
Contributor Author

@masahi Sure, I will send you a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it status: help wanted type: bug
Projects
None yet
Development

No branches or pull requests

2 participants