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

[MXNET-326] fix filter layout in DepthwiseConv2dBackwardFilterKernel #10578

Merged
merged 8 commits into from Apr 17, 2018

Conversation

Projects
None yet
6 participants
@nihui
Contributor

nihui commented Apr 17, 2018

Description

the filter layout should be NCHW, while the exsiting code write as NHWC, which is wrong.
this change fix it.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here
fix filter layout in DepthwiseConv2dBackwardFilterKernel
the filter layout should be NCHW, while the exsiting code write as NHWC, which is wrong.
this change fix it.

@nihui nihui referenced this pull request Apr 17, 2018

Closed

[MXNET-261]Update MKLDNN & Add CPP Test #10365

7 of 7 tasks complete

@nihui nihui changed the title from fix filter layout in DepthwiseConv2dBackwardFilterKernel to [MXNET-326] fix filter layout in DepthwiseConv2dBackwardFilterKernel Apr 17, 2018

@nihui

This comment has been minimized.

Contributor

nihui commented Apr 17, 2018

this change is needed to pass depthwise conv unittest, which was skipped
reference #10365

@xinyu-intel

This comment has been minimized.

Contributor

xinyu-intel commented Apr 17, 2018

Thank you for this pr. You can also open the depth_wise_conv unit test in your pr to check if this change can be passed:)

@@ -450,14 +453,17 @@ DepthwiseConv2dBackwardFilterKernel(const DepthwiseArgs args,
const int input_offset_temp =
(out_b * in_channel * in_height * in_width) +
(in_c * in_height * in_width) + (in_row * in_width);
const int filter_backprop_temp =
(in_channel * filter_width * filter_height) +
(filter_width * f_h);
CUDA_UNROLL for (int f_w = 0; f_w < filter_width; ++f_w) {
const int in_col = in_col_start + f_w;
const int addr_temp = filter_width * f_h;

This comment has been minimized.

@xinyu-intel

xinyu-intel Apr 17, 2018

Contributor

It seems that variable addr_temp was declared but never referenced.

nihui added some commits Apr 17, 2018

@xinyu-intel

This comment has been minimized.

Contributor

xinyu-intel commented Apr 17, 2018

Thank you:) It seems that this change can pass the unit test except MKLDNN version which will be fixed in #10365 .

@xinyu-intel

This comment has been minimized.

Contributor

xinyu-intel commented Apr 17, 2018

@nihui I will apply my changes to your repo to check if this pr can solve both two problems:)

@pengzhao-intel

This comment has been minimized.

pengzhao-intel commented Apr 17, 2018

Good jobs @nihui @xinyu-intel

@zheng-da @marcoabreu please help take a review :)
And we can close #8712 after this PR is merged.

CHECK_EQ(mkldnn_format_last, 56);
CHECK_EQ(mkldnn_nchw, 5);
CHECK_EQ(mkldnn_oihw, 12);
}

This comment has been minimized.

@zheng-da

zheng-da Apr 17, 2018

Contributor

This is MKLDNN specific. Maybe it's better to keep it in the fix for the MKLDNN implementation.

@@ -239,6 +239,7 @@ mkldnn_memory_format_t GetDefaultFormat(const mkldnn::memory::desc &desc) {
case mkldnn_gOihw8o:
case mkldnn_Goihw8g:
case mkldnn_gOihw16o:
case mkldnn_Goihw16g:

This comment has been minimized.

@zheng-da

zheng-da Apr 17, 2018

Contributor

is this related to this PR?

@zheng-da

This comment has been minimized.

Contributor

zheng-da commented Apr 17, 2018

I'm a little confused. Does this PR fix everything (including MKLDNN) or just fix the bug in the original implementation?

If this is to fix everything, I suppose we can close #10365?

@TaoLv

This comment has been minimized.

Contributor

TaoLv commented Apr 17, 2018

@zheng-da Yes. #10365 can be closed. We merged these two PRs together, otherwise neither of them can pass ci.

@marcoabreu

It's fine with me two do both changes together

@marcoabreu

This comment has been minimized.

Contributor

marcoabreu commented Apr 17, 2018

Thanks a lot for catching this and re-enabling the test, @nihui! Also thanks to the people from Intel to have these joint efforts!

@marcoabreu marcoabreu merged commit 62a0ab6 into apache:master Apr 17, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@nihui

This comment has been minimized.

Contributor

nihui commented Apr 18, 2018

I think it is needed to cherry-pick this commit to v1.2.0 branch, as depthwise convolution is widely used, otherwise a regression bug would be in the coming stable release. thanks! @marcoabreu @eric-haibin-lin

rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018

[MXNET-326] fix filter layout in DepthwiseConv2dBackwardFilterKernel (a…
…pache#10578)

* fix filter layout in DepthwiseConv2dBackwardFilterKernel

the filter layout should be NCHW, while the exsiting code write as NHWC, which is wrong.
this change fix it.

* remove old addr_temp code

* do not skip test_depthwise_convolution

* fix channel index

* update mkldnn to fix bugs

* add mkldnn type

* add mkldnn memory format cpp tests

* test_depthwise_convolution

zheng-da added a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 8, 2018

[MXNET-326] fix filter layout in DepthwiseConv2dBackwardFilterKernel (a…
…pache#10578)

* fix filter layout in DepthwiseConv2dBackwardFilterKernel

the filter layout should be NCHW, while the exsiting code write as NHWC, which is wrong.
this change fix it.

* remove old addr_temp code

* do not skip test_depthwise_convolution

* fix channel index

* update mkldnn to fix bugs

* add mkldnn type

* add mkldnn memory format cpp tests

* test_depthwise_convolution

@anirudh2290 anirudh2290 referenced this pull request Jun 11, 2018

Merged

cherry-pick bug fixes in MKLDNN for v1.2.0 #11212

0 of 7 tasks complete

zheng-da added a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018

[MXNET-326] fix filter layout in DepthwiseConv2dBackwardFilterKernel (a…
…pache#10578)

* fix filter layout in DepthwiseConv2dBackwardFilterKernel

the filter layout should be NCHW, while the exsiting code write as NHWC, which is wrong.
this change fix it.

* remove old addr_temp code

* do not skip test_depthwise_convolution

* fix channel index

* update mkldnn to fix bugs

* add mkldnn type

* add mkldnn memory format cpp tests

* test_depthwise_convolution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment