add mkldnn softmax_output#13699

rongzha1 · 2018-12-20T08:48:11Z

Description

Add mkldnn implement for softmax_output OP

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

[done ] Changes are complete (i.e. I finished coding on this PR)
[done ] All changes have test coverage:
[ done] 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_softmax has passed

@pengzhao-intel

pengzhao-intel · 2018-12-22T04:00:34Z

@rongzha1 please help fix the lint issue

Warning, treated as error:

/work/mxnet/python/mxnet/ndarray/init.py:docstring of mxnet.ndarray.SoftmaxOutput:30: WARNING: Bullet list ends without a blank line; unexpected unindent.

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

TaoLv · 2018-12-25T04:57:38Z

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

+  }
+
+  auto input_mem = idata.GetMKLDNNData();
+  auto output_mem = odata.GetMKLDNNData();


This function will create memory with default layout.

Is it possible for softmax_output to have input with internal layout? Then how does the output look like?

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

src/operator/softmax_output-inl.h

pengzhao-intel · 2018-12-25T05:22:57Z

@rongzha1 could you post some performance changes by this PR?

rongzha1 · 2018-12-26T02:31:52Z

@rongzha1 could you post some performance changes by this PR?

for 1024*256 input, performance speedup 2.75

env: skx-8180, 1socket 28 core,
model : wide_and_deep model

old (ms)	opt(ms)	speedup
0.1890553	0.0687512	2.7498492

sandeep-krishnamurthy

Thank you for your contributions.
@azai91 - You may be interested in this PR.

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

pengzhao-intel · 2019-01-02T01:18:21Z

@rongzha1 please rebase the code and make the CI pass :)

rongzha1 · 2019-01-03T01:34:24Z

please help to review and merge to the master branch. @eric-haibin-lin @zheng-da @azai91

pengzhao-intel

WIP to review :)

pengzhao-intel · 2019-01-07T07:57:12Z

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

+#include "./mkldnn_ops-inl.h"
+#include "./mkldnn_base-inl.h"
+
+#if MXNET_USE_MKLDNN == 1


Move to the before of include

pengzhao-intel · 2019-01-07T07:58:36Z

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

+  //  softmax_output has no axis parameter, so use it as it original implement.
+  int axis = data.shape().ndim() - 1;
+  mkldnn::softmax_forward::desc desc = is_train
+      ? mkldnn::softmax_forward::desc(mkldnn::prop_kind::forward_training,


does the training mode support now?

following mkldnn_softmax did

TaoLv · 2019-01-10T13:09:42Z

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

+      ? mkldnn::softmax_forward::desc(mkldnn::prop_kind::forward_training,
+                                      data_md, axis)
+      : mkldnn::softmax_forward::desc(mkldnn::prop_kind::forward_scoring,
+                                      data_md, axis);


auto prop = is_train ? mkldnn::prop_kind::forward_training : mkldnn::prop_kind::forward_scoring; auto desc = mkldnn::softmax_forward::desc(prop, data_md, axis);

TaoLv · 2019-01-10T13:11:48Z

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

+
+static mkldnn::softmax_forward::primitive_desc GetSoftmaxOutputFwdDescImpl(
+               const SoftmaxOutputParam& param, bool is_train,
+               const NDArray &data, const mkldnn::memory &input_mem) {


why we need have data and input_mem at the same time?

OK, remove data

TaoLv · 2019-01-10T13:17:48Z

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

+  }
+
+  auto input_mem = idata.GetMKLDNNData();
+  auto output_mem = odata.GetMKLDNNData();


Is it possible for softmax_output to have input with internal layout? Then how does the output look like?

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

TaoLv · 2019-01-10T13:23:10Z

src/operator/softmax_output.cc

-  return op;
+
+DMLC_REGISTER_PARAMETER(SoftmaxOutputParam);
+struct SoftmaxOutputGrad {


@eric-haibin-lin Please help to review this. Do we have any existing gradient structure to do this?

TaoLv · 2019-01-10T14:08:50Z

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

+  auto input_mem = idata.GetMKLDNNData();
+  auto output_mem = odata.GetMKLDNNData();
+
+  MKLDNNSoftmaxOutputFwd &fwd = GetSoftmaxOutputForward(param, ctx, idata, *input_mem);


label is not used?

label is used for backward

TaoLv · 2019-01-10T14:10:29Z

src/operator/softmax_output.cc

+  gnode->attrs.op = nnvm::Op::Get("_backward_SoftmaxOutput");
+  gnode->attrs.name = n->attrs.name + "_backward";
+  std::vector<nnvm::NodeEntry> in_grad(2);
+  for (uint32_t i = 0; i < 2; ++i) {


if there are only two elements, no need to have for-loop.

src/operator/softmax_output.cc

TaoLv · 2019-01-10T14:19:51Z

src/operator/softmax_output.cc

+                               const std::vector<NDArray> &outputs) {
+  CHECK_EQ(inputs.size(), 2U);
+  const SoftmaxOutputParam &param = nnvm::get<SoftmaxOutputParam>(attrs.parsed);
+  // MKLDNN softmaxOutput only works well on the special MKLDNN layout.


What does "special MKLDNN layout" mean here?

means support ndim 1,2,4; remove this ambiguous comments,

pengzhao-intel

Thanks for the contribution.

LGTM

TaoLv · 2019-01-26T15:32:57Z

src/operator/softmax_output.cc

+// Softmax symbol is renamed to SoftmaxOutput and deprecated since Dec, 2015
+NNVM_REGISTER_OP(SoftmaxOutput).add_alias("Softmax");

-MXNET_REGISTER_OP_PROPERTY(Softmax, DeprecatedSoftmaxProp)


@szha could you help to take a look at this change? SoftmaxOutput is re-writed with NNVM flavor in this PR and the deprecated Softmax is moved to be an alias of SoftmaxOutput. Need your confirm that it doesn't break any API.

Seems that they differ by only the label field, which means the note isn't accurate and if it should be considered breakage then it already happened in the past. Looks like the Softmax op as it stands isn't really usable so I think making it an alias of SoftmaxOutput is actually improvement.

TaoLv · 2019-01-30T13:51:27Z

Ping @szha @eric-haibin-lin for review. Thank you.

pengzhao-intel · 2019-02-02T06:56:17Z

@szha could you help take a look for the API change?

rongzha1 · 2019-02-11T08:06:43Z

Hi, @szha @eric-haibin-lin Can you help to review this PR and merge it to master branch? Thanks

TaoLv · 2019-02-12T12:51:27Z

@rongzha1 Please re-base code and re-trigger CI. I will merge this PR tomorrow if there is no other comment.

TaoLv

Nit comments.

TaoLv · 2019-02-12T12:53:17Z

src/operator/nn/mkldnn/mkldnn_softmax_output.cc

+
+  auto input_mem = idata.GetMKLDNNData();
+  auto out_mem = CreateMKLDNNMem(out_data[softmaxout_enum::kOut],
+                            input_mem->get_primitive_desc(), req[softmaxout_enum::kOut]);


TaoLv · 2019-02-12T12:54:41Z

src/operator/softmax_output.cc

+  }
+  FallBackCompute(SoftmaxOutputCompute<cpu>, attrs, ctx, inputs, req, outputs);
+}
+


remove blank line.

TaoLv · 2019-02-12T12:56:25Z

src/operator/softmax_output.cc

+  return {"data", "label"};
+}
+
+


reduce to 1 blank line.

…tput

TaoLv · 2019-02-13T10:15:45Z

Merging now.

* add mkldnn softmax_output * fix gpu OP unittest error * fix ci/jenkins/mxnet-validation/unix-gpu compiler error * fix coding style * fix Tao comments * remove blank line, fix indentx * modify according to sandeep's comments * change get CPU engine method, and pravate variable * move macro MXNET_USE_MKLDNN to the head * modify according to Tao's comments * make output layout as input * change API of GetSoftmaxOutputForward * add CommitOutput for mkldnn_softmax_output * trigger Jenkins re-test * add alias Softmax symbol for SoftmaxOutput OP * indent and remove blank line

TaoLv added Operator MKLDNN labels Dec 20, 2018

TaoLv reviewed Dec 25, 2018

View reviewed changes

sandeep-krishnamurthy reviewed Dec 26, 2018

View reviewed changes

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

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

TaoLv reviewed Dec 27, 2018

View reviewed changes

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

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

pengzhao-intel reviewed Jan 7, 2019

View reviewed changes

TaoLv reviewed Jan 10, 2019

View reviewed changes

pengzhao-intel approved these changes Jan 16, 2019

View reviewed changes

TaoLv reviewed Jan 26, 2019

View reviewed changes

TaoLv reviewed Feb 12, 2019

View reviewed changes

rongzha1 added 11 commits February 12, 2019 21:09

add mkldnn softmax_output

0c81bf2

fix gpu OP unittest error

8cc2eec

fix ci/jenkins/mxnet-validation/unix-gpu compiler error

ea81bbd

fix coding style

403dc37

fix Tao comments

dbfff62

remove blank line, fix indentx

6e4e946

modify according to sandeep's comments

38ec708

change get CPU engine method, and pravate variable

37e9c11

move macro MXNET_USE_MKLDNN to the head

d7af111

modify according to Tao's comments

58a23bd

make output layout as input

bbf11d7

rongzha1 added 6 commits February 12, 2019 21:09

change API of GetSoftmaxOutputForward

3420b61

add CommitOutput for mkldnn_softmax_output

25162cb

trigger Jenkins re-test

171da54

add alias Softmax symbol for SoftmaxOutput OP

c031f6b

indent and remove blank line

ee845f7

Merge remote-tracking branch 'official/master' into mkldnn_softmax_ou…

0757669

…tput

TaoLv approved these changes Feb 13, 2019

View reviewed changes

TaoLv merged commit 45978a9 into apache:master Feb 13, 2019

DickJC123 mentioned this pull request Mar 4, 2019

SoftmaxOutput crashes with normalization "valid" #14301

Closed

bartekkuncer mentioned this pull request Jul 20, 2021

[BUGFIX] Fix backport of SoftmaxOutput implementation using onednn kernels #20459

Merged

2 tasks

Conversation

rongzha1 commented Dec 20, 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 22, 2018

Uh oh!

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!

Uh oh!

pengzhao-intel commented Dec 25, 2018

Uh oh!

rongzha1 commented Dec 26, 2018

Uh oh!

sandeep-krishnamurthy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pengzhao-intel commented Jan 2, 2019

Uh oh!

rongzha1 commented Jan 3, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

TaoLv commented Jan 30, 2019

rongzha1 commented Dec 20, 2018 •

edited

Loading

sandeep-krishnamurthy left a comment •

edited

Loading