Add mkldnn OP for slice by huangzhiyuan · Pull Request #13730 · apache/mxnet

huangzhiyuan · 2018-12-26T14:25:17Z

Description

Add mkldnn implement for slice OP

Checklist

Essentials

Please feel free to remove inapplicable items for your 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 new C++ functions in header files, their functionalities and arguments are documented.
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

test_slice has passed

@pengzhao-intel , @ZhennanQin , @TaoLv

pengzhao-intel · 2018-12-26T14:32:43Z

@huangzhiyuan please verify the performance in #12303 and other tests from your local.

src/operator/nn/mkldnn/mkldnn_slice-inl.h

src/operator/nn/mkldnn/mkldnn_slice.cc

src/operator/nn/mkldnn/mkldnn_slice-inl.h

src/operator/tensor/matrix_op-inl.h

pengzhao-intel

Any specific test case for this change is added?

pengzhao-intel · 2019-01-04T13:34:16Z

src/operator/nn/mkldnn/mkldnn_slice.cc

+    offsets[i] = s;
+  }
+  auto in_mem = in.GetMKLDNNData();
+  auto in_mem_pd = in_mem->get_primitive_desc();


Please merge in_mem and in_mem_pd into one line as out_mem_pd in case the in_mem doesn't use in here.

This is just for the overall code style, I can also modify it if you insist.

what's the "overall code style"?

Have merge im_mem and im_mem_pd into one line in the new commit.

src/operator/nn/mkldnn/mkldnn_slice.cc

src/operator/tensor/matrix_op-inl.h

src/operator/tensor/matrix_op.cc

huangzhiyuan · 2019-01-06T06:22:16Z

@pengzhao-intel No specific test case for this change should be added, the current test case is enough to cover. Thanks for your review :)

pengzhao-intel

Thanks for the improvements!

LGTM

TaoLv · 2019-01-11T14:13:14Z

src/operator/nn/mkldnn/mkldnn_slice.cc

+    offsets[i] = s;
+  }
+  auto in_mem = in.GetMKLDNNData();
+  auto in_mem_pd = in_mem->get_primitive_desc();


what's the "overall code style"?

TaoLv · 2019-01-11T14:15:34Z

src/operator/nn/mkldnn/mkldnn_slice.cc

+  static MX_THREAD_LOCAL std::unordered_map<MKLDNNSliceSignature, MKLDNNSliceFwd, OpHash> fwds;
+#endif
+  MKLDNNSliceSignature key(param);
+  key.AddSign(in_data);


Do we need to have is_train and out_data into the key?

In_data is enough for the key to cache. And I have put is_train and out_data into the key in new commit.

TaoLv · 2019-01-11T14:46:26Z

src/operator/tensor/matrix_op-inl.h

+
+  if (in_stype == kDefaultStorage) {
+#if MXNET_USE_MKLDNN == 1
+  if (dev_mask == Context::kCPU && MKLDNNEnvSet() && SupportMKLDNNSlice(param)) {


huangzhiyuan · 2019-01-14T06:33:29Z

The CI failure seem to same with #13833, and fix patch [#13841] is WIP. @TaoLv please help to review again :)

TaoLv

Thank you for the fix. Let's wait for CI passing.

TaoLv · 2019-01-16T15:10:04Z

Closes #12303 .

* add mkldnn slice * fix lint * fix lint * mv SliceEx to matrix_op.cc * fix lint * optimize dispatch_mode * retrigger ci * fix indent

add mkldnn slice

c8e867c

pengzhao-intel mentioned this pull request Dec 26, 2018

support slice on MKLDNN arrays better #12303

Closed

TaoLv added Operator MKLDNN pr-awaiting-review PR is waiting for code review labels Dec 26, 2018

ZhennanQin reviewed Dec 27, 2018

View reviewed changes

src/operator/nn/mkldnn/mkldnn_slice-inl.h Show resolved Hide resolved

src/operator/nn/mkldnn/mkldnn_slice.cc Outdated Show resolved Hide resolved

huangzhiyuan added 2 commits December 27, 2018 21:00

fix lint

3123e0c

fix lint

c2e6d26

TaoLv reviewed Dec 28, 2018

View reviewed changes

src/operator/nn/mkldnn/mkldnn_slice-inl.h Outdated Show resolved Hide resolved

src/operator/tensor/matrix_op-inl.h Outdated Show resolved Hide resolved

src/operator/tensor/matrix_op-inl.h Outdated Show resolved Hide resolved

src/operator/tensor/matrix_op-inl.h Outdated Show resolved Hide resolved

huangzhiyuan added 2 commits January 2, 2019 09:45

mv SliceEx to matrix_op.cc

609843f

fix lint

ddc3b9d

pengzhao-intel reviewed Jan 4, 2019

View reviewed changes

optimize dispatch_mode

a66ac0a

pengzhao-intel approved these changes Jan 11, 2019

View reviewed changes

TaoLv reviewed Jan 11, 2019

View reviewed changes

huangzhiyuan added 4 commits January 12, 2019 19:34

Merge branch 'master' into mkldnn-slice

520d72d

retrigger ci

9a4ab21

fix indent

cc7a13a

Merge branch 'master' into mkldnn-slice

91fe193

TaoLv approved these changes Jan 15, 2019

View reviewed changes

Merge branch 'master' into mkldnn-slice

ed1b5e5

TaoLv merged commit 2616275 into apache:master Jan 16, 2019

huangzhiyuan deleted the mkldnn-slice branch January 17, 2019 01:32

stephenrawls pushed a commit to stephenrawls/incubator-mxnet that referenced this pull request Feb 16, 2019

Add mkldnn OP for slice (apache#13730)

90e6791

* add mkldnn slice * fix lint * fix lint * mv SliceEx to matrix_op.cc * fix lint * optimize dispatch_mode * retrigger ci * fix indent

haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019

Add mkldnn OP for slice (apache#13730)

8c14c36

* add mkldnn slice * fix lint * fix lint * mv SliceEx to matrix_op.cc * fix lint * optimize dispatch_mode * retrigger ci * fix indent

Conversation

huangzhiyuan commented Dec 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Changes

Uh oh!

pengzhao-intel commented Dec 26, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pengzhao-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huangzhiyuan commented Jan 6, 2019

Uh oh!

pengzhao-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huangzhiyuan commented Jan 14, 2019

Uh oh!

TaoLv left a comment

Choose a reason for hiding this comment

Uh oh!

TaoLv commented Jan 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

huangzhiyuan commented Dec 26, 2018 •

edited

Loading