[oneDNN] Fix to #33282 , added support of X input broadcasting to oneDNN elementwise ops #33549

jczaja · 2021-06-14T15:18:43Z

PR types

Bug fixes

PR changes

OPs

Describe

This is fix to #33282 . It does implement broadcasting for elementwise X input as previosuly only input Y was having broadcasting support. Inplace support for oneDNN elementwise kernels was disabled.

paddle-bot-old · 2021-06-14T15:18:46Z

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

lidanqing-intel · 2021-06-15T06:32:27Z

@jczaja Hi next time could you please name the PR with more specific description? For example this PR could be extend broadcast elementwise_add op with X(1 dims) Y(2 dims).
It will be easier for release note

lidanqing-intel · 2021-06-15T09:50:22Z

paddle/fluid/operators/elementwise/mkldnn/elementwise_mkldnn_op.h

-    // For Inplace src and and dst are the same memory object
-    const auto dst_memory =
-        is_inplaced ? src_x_memory : handler.AcquireDstMemory(z);
+    const auto dst_memory = handler.AcquireDstMemory(z);



So we remove the inplace support for elementwise_add and elementwise_mul ? But you plan to remove it permanently or you will still add inplace later ?

Inplace working with oneDNN caching is very hard , and there is little perf gain and some performance improvement. Among all of this elementwise ops for inplace are most difficult so I removed inplace support for elementwise oneDNN kernels.

lidanqing-intel · 2021-06-15T10:52:50Z

paddle/fluid/platform/mkldnn_reuse.h

@@ -674,15 +666,21 @@ class BinaryMKLDNNHandler : public platform::MKLDNNHandlerT<T, dnnl::binary> {
      const auto dst_tz =
          (z == nullptr) ? src_x_tz : framework::vectorize(z->dims());


if z is null, we point set dst_tz to src_x_tz, but what if src_x_tz is one dimension ? And why z could be nullptr, in which case z could be nullptr ?

Good observation.

z is null in situation when we want Binary primitive to write into allocation managed by oneDNN e.g. we do not want output be send to Tensor. This is only used in elementwise_mul_grad:77 as this operation consists of Binary + Reorder . Binary output is a temporary data that is an input to Reorder . Output of reorder goes into output tensor.

Currently there in elementwise_mul_grad (src_x_tz) is always gradient of of Out buffer so I know it is never to be broadcasted. But I agree that code would be better if it works for z == nullptr and x being bradcasted. So I improve that. thanks

lidanqing-intel · 2021-06-15T11:09:21Z

paddle/fluid/platform/mkldnn_reuse.h

          src_x_tz, platform::MKLDNNGetDataType<T>(), x->format());
      auto src1_md = dnnl::memory::desc(
          src_y_tz, platform::MKLDNNGetDataType<T>(), y->format());
-      if (rankdiff > 0) {
+      auto rankdiff = x->dims().size() - y->dims().size();
+      if (rankdiff > 0) {  // Second input is lesser than first


Sorry but why it is implemented this way.
I thought broadcast is copy values line by line until Y is same dimension as X dimension.
If X is 5*12, and Y is 5, rankdiff is 1?
or it is not like that ?

If X is [5,12] and Y is [5] then Y dims for oneDNN need to be [5,1] . And then oneDNN having inputs [5,12] and [5,1] will recognize that second input need broadcasting to have applied. So code you are referencing is just to change shape from [5] to [5,1]

lidanqing-intel

LGTM

jczaja · 2021-06-15T15:48:19Z

@juncaipeng PR-CI-APPROVAL is complaining on wrong PADDLE_ENFORCE_EQ macro I touched in this PR and I do not understand what is wrong with it. couple you please advice?

juncaipeng · 2021-06-16T03:00:39Z

@juncaipeng PR-CI-APPROVAL is complaining on wrong PADDLE_ENFORCE_EQ macro I touched in this PR and I do not understand what is wrong with it. couple you please advice?

The error infomation is Possible errors include 1. the error message is empty / 2. the error message is too short / 3. the error type is not specified.

The specification in https://github.com/PaddlePaddle/Paddle/wiki/Paddle-Error-Message-Writing-Specification-(English-Verison)

Maybe you can add more error messages ~

jakpiase · 2021-06-16T09:15:44Z

paddle/fluid/platform/mkldnn_reuse.h

          src_x_tz, platform::MKLDNNGetDataType<T>(), x->format());
      auto src1_md = dnnl::memory::desc(
          src_y_tz, platform::MKLDNNGetDataType<T>(), y->format());
-      if (rankdiff > 0) {
+      if (rankdiff > 0) {  // Second input is lesser than first


Suggested change

if (rankdiff > 0) { // Second input is lesser than first

if (rankdiff > 0) { // Second input is smaller than first

jakpiase · 2021-06-16T09:19:37Z

paddle/fluid/platform/mkldnn_reuse.h

@@ -644,14 +644,6 @@ class BinaryMKLDNNHandler : public platform::MKLDNNHandlerT<T, dnnl::binary> {
            platform::CreateKey(
                dev_ctx, framework::vectorize(x->dims()), uniq_name,
                (algo == dnnl::algorithm::binary_mul ? "M" : ""))) {


Why aren't you just passing algo to CreateKey function? It can convert dnnl:algorithm datatype to a string

lidanqing-intel · 2021-06-18T02:36:48Z

@jczaja Please cherry-pick this PR into release/2.1 after this PR pass all CIs. Thanks.

lidanqing-intel · 2021-06-18T03:37:06Z

@jczaja I saw you remove the whole test of inplace tests. Since 22 is tag day and Baidu expect today to merge this PR. Is it possible to keep minor change and make it pass CIs. If CI still fail not because of this PR, let me know.

jczaja · 2021-06-18T14:29:28Z

@juncaipeng I would like You to advice me on failing test_pass_builder . Reason of its failure is that
"/tmp/test_viz_pass" file cannot be created , as this file is to be created/opened via std::ofstream , and in this
situation tmp directory does not exist so ofstream fials due to missing tmp directory. When this tmp directory is to be created?

Error message:

2021-06-18 19:51:35 Error Message Summary:
2021-06-18 19:51:35 ----------------------
2021-06-18 19:51:35 UnavailableError: Can not open file /workspace/Paddle/build/python/paddle/fluid/tests/unittests/tmp/test_viz_pass for printing the graph.
2021-06-18 19:51:35 [Hint: Expected fout->good() == true, but received fout->good():0 != true:1.] (at /workspace/Paddle/paddle/fluid/framework/ir/graph_viz_pass.cc:44)

arlesniak

LGTM

lidanqing-intel

LGTM

jakpiase · 2021-06-23T17:57:32Z

LGTM

…sting to oneDNN elementwise ops (PaddlePaddle#33549) * - fix to PaddlePaddle#33282 * - Increased threshold for elementwise_mul_bf16 grad * -disabled faulty UT * - fix to approval

…DNN elementwise ops (#33549) (#33845) * - fix to #33282 * - Increased threshold for elementwise_mul_bf16 grad * -disabled faulty UT * - fix to approval

jczaja added the Intel label Jun 14, 2021

lidanqing-intel self-requested a review June 15, 2021 07:45

jczaja requested review from jakpiase and arlesniak June 15, 2021 08:22

jczaja changed the title ~~[oneDNN] Fix to #33282~~ [oneDNN] Fix to #33282 , added support of X input broadcasting to oneDNN elementwise_add/elementwise_mul Jun 15, 2021

lidanqing-intel mentioned this pull request Jun 15, 2021

MKLDNN error for blazeface model #33282

Closed

lidanqing-intel reviewed Jun 15, 2021

View reviewed changes

paddle-bot-old bot referenced this pull request Jun 15, 2021

- Fix after review

f7cf10b

jczaja closed this Jun 15, 2021

jczaja reopened this Jun 15, 2021

paddle-bot-old bot referenced this pull request Jun 15, 2021

- Lint fixes

4c82acf

lidanqing-intel previously approved these changes Jun 15, 2021

View reviewed changes

jczaja dismissed lidanqing-intel’s stale review via 45ae9bd June 15, 2021 15:26

jczaja assigned juncaipeng Jun 15, 2021

jakpiase reviewed Jun 16, 2021

View reviewed changes

jczaja changed the title ~~[oneDNN] Fix to #33282 , added support of X input broadcasting to oneDNN elementwise_add/elementwise_mul~~ [oneDNN] Fix to #33282 , added support of X input broadcasting to oneDNN elementwise ops Jun 16, 2021

- fix to PaddlePaddle#33282

7a13edd

jczaja force-pushed the prv-33282-fix branch from a5a65bb to 7a13edd Compare June 18, 2021 11:15

jczaja added 3 commits June 22, 2021 12:07

- Increased threshold for elementwise_mul_bf16 grad

f2f2410

-disabled faulty UT

404656b

- fix to approval

8ac7a1f

arlesniak approved these changes Jun 23, 2021

View reviewed changes

lidanqing-intel approved these changes Jun 23, 2021

View reviewed changes

jakpiase approved these changes Jun 23, 2021

View reviewed changes

jczaja merged commit 049dd85 into PaddlePaddle:develop Jun 24, 2021

jczaja deleted the prv-33282-fix branch June 24, 2021 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[oneDNN] Fix to #33282 , added support of X input broadcasting to oneDNN elementwise ops #33549

[oneDNN] Fix to #33282 , added support of X input broadcasting to oneDNN elementwise ops #33549

jczaja commented Jun 14, 2021

paddle-bot-old bot commented Jun 14, 2021

lidanqing-intel commented Jun 15, 2021

lidanqing-intel Jun 15, 2021

jczaja Jun 15, 2021

lidanqing-intel Jun 15, 2021 •

edited

Loading

jczaja Jun 15, 2021

lidanqing-intel Jun 15, 2021

jczaja Jun 15, 2021

lidanqing-intel left a comment

jczaja commented Jun 15, 2021

juncaipeng commented Jun 16, 2021

jakpiase Jun 16, 2021

jakpiase Jun 16, 2021

lidanqing-intel commented Jun 18, 2021

lidanqing-intel commented Jun 18, 2021 •

edited

Loading

jczaja commented Jun 18, 2021

arlesniak left a comment

lidanqing-intel left a comment

jakpiase commented Jun 23, 2021

		@@ -674,15 +666,21 @@ class BinaryMKLDNNHandler : public platform::MKLDNNHandlerT<T, dnnl::binary> {
		const auto dst_tz =
		(z == nullptr) ? src_x_tz : framework::vectorize(z->dims());

	if (rankdiff > 0) { // Second input is lesser than first
	if (rankdiff > 0) { // Second input is smaller than first

[oneDNN] Fix to #33282 , added support of X input broadcasting to oneDNN elementwise ops #33549

[oneDNN] Fix to #33282 , added support of X input broadcasting to oneDNN elementwise ops #33549

Conversation

jczaja commented Jun 14, 2021

PR types

PR changes

Describe

paddle-bot-old bot commented Jun 14, 2021

lidanqing-intel commented Jun 15, 2021

lidanqing-intel Jun 15, 2021

Choose a reason for hiding this comment

jczaja Jun 15, 2021

Choose a reason for hiding this comment

lidanqing-intel Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

jczaja Jun 15, 2021

Choose a reason for hiding this comment

lidanqing-intel Jun 15, 2021

Choose a reason for hiding this comment

jczaja Jun 15, 2021

Choose a reason for hiding this comment

lidanqing-intel left a comment

Choose a reason for hiding this comment

jczaja commented Jun 15, 2021

juncaipeng commented Jun 16, 2021

jakpiase Jun 16, 2021

Choose a reason for hiding this comment

jakpiase Jun 16, 2021

Choose a reason for hiding this comment

lidanqing-intel commented Jun 18, 2021

lidanqing-intel commented Jun 18, 2021 • edited Loading

jczaja commented Jun 18, 2021

arlesniak left a comment

Choose a reason for hiding this comment

lidanqing-intel left a comment

Choose a reason for hiding this comment

jakpiase commented Jun 23, 2021

lidanqing-intel Jun 15, 2021 •

edited

Loading

lidanqing-intel commented Jun 18, 2021 •

edited

Loading