Fix to #34554 #37079

jczaja · 2021-11-09T14:10:35Z

PR types

Bug fixes

PR changes

OPs

Describe

This disabled caching of oneDNN primitives so that oneDNN cache its own elements. This change fixes the problems reported in #34554

paddle-bot-old · 2021-11-09T14:10:38Z

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

jczaja · 2021-11-16T10:20:21Z

@pawelpiotrowicz , @Silv3S, @tsocha , @zuzg, @piotrekobiIntel Please review

ghost · 2021-11-16T11:04:33Z

paddle/fluid/operators/mkldnn/conv_mkldnn_op.cc

+template <typename T>
+std::shared_ptr<std::tuple<float, std::vector<float>>> get_bias_scales(
+    const framework::ExecutionContext& ctx,
+    const platform::MKLDNNDeviceContext& dev_ctx, const std::string& key) {
+  return std::make_shared<std::tuple<float, std::vector<float>>>(
+      std::make_tuple(0.0f, std::vector<float>(1, 1.0f)));
+}
+
+template <>
+std::shared_ptr<std::tuple<float, std::vector<float>>> get_bias_scales<int8_t>(
+    const framework::ExecutionContext& ctx,
+    const platform::MKLDNNDeviceContext& dev_ctx, const std::string& key) {
+  // Get scales int8 bias key
+  const std::string key_bs = key + "@bs";
+
+  // Scales for int8 bias are to be cached to avoid
+  // computing them each iteration
+  auto bias_scale_tuple =
+      std::static_pointer_cast<std::tuple<float, std::vector<float>>>(
+          dev_ctx.GetBlob(key_bs));
+  if (bias_scale_tuple) return bias_scale_tuple;
+
+  const auto* filter = ctx.Input<Tensor>("Filter");
+  const auto& weights_tz = framework::vectorize(filter->dims());
+  const int groups = std::max(ctx.Attr<int>("groups"), 1);
+
+  const auto& scale_weights_data =
+      ctx.Attr<std::vector<float>>("Scale_weights");
+  const auto& scale_in_data = ctx.Attr<float>("Scale_in");
+
+  bool is_multi_channel = scale_weights_data.size() > 1;
+  int mask_reorder = is_multi_channel ? 1 << 0 : 1;
+  const int count =
+      is_multi_channel
+          ? (groups > 1 ? (weights_tz)[1] * (weights_tz)[0] : (weights_tz)[0])
+          : 1;
+
+  bias_scale_tuple =
+      std::make_shared<std::tuple<float, std::vector<float>>>(std::make_tuple(
+          static_cast<float>(mask_reorder), std::vector<float>(count)));
+  for (int i = 0; i < count; i++) {
+    std::get<1>(*bias_scale_tuple)[i] = scale_in_data * scale_weights_data[i];
+  }
+
+  dev_ctx.SetBlob(key_bs, bias_scale_tuple);
+
+  return bias_scale_tuple;
+}


Maybe merging these 2 functions into one and doing something different depending on the result of:
if(std::is_same<T, int8_t>::value || std::is_same<T, uint8_t>::value
would be a better solution?
It would allow to do get rid of the remap structs that seemed quite confusing to me at first glance.

I have diffrent observation e.g. I think remapping is way easier for me to understand:) And also "if.." is a runtime check as c++14 does not have if contrexpr (c++17) , and remapping mechanism makes a check evaluated in compile time

ghost

LGTM

Silv3S

LGTM

jakpiase · 2021-11-16T15:34:46Z

paddle/fluid/operators/mkldnn/conv_mkldnn_op.cc

+    } else {
+      src_md = platform::MKLDNNMemDesc(src_tz, data_type, chosen_memory_format);
+      weights_md = platform::MKLDNNMemDesc(weights_tz, data_type,
+                                           MKLDNNMemoryFormat::any);


You are using "chosen memory format" everywhere except that place, please unity that

good catch. Fixed

jakpiase · 2021-11-16T15:37:29Z

paddle/fluid/operators/mkldnn/conv_mkldnn_op.cc

+     * the memory format preferred for best performance
+     */
+    const auto chosen_memory_format = MKLDNNMemoryFormat::any;
+    const auto weights_format = MKLDNNMemoryFormat::any;


What is the point of having two identical variables here? Every format im conv should use any, do I think that this redundancy is not necessary

some legacy remaining. I removed those

lidanqing-intel · 2021-11-18T08:06:39Z

paddle/fluid/operators/mkldnn/conv_mkldnn_op.cc

+  scale_tuple =
+      std::make_shared<std::tuple<float, std::vector<float>>>(std::make_tuple(
+          static_cast<float>(sum_scale), std::vector<float>(count)));
+  for (int i = 0; i < count; i++) {


for loop could use #pragma omp parallel for?

This generation of scales is performed only once as it is cached now. So there is no need adding openmp as it would just create multiple threads . each thread with some resources that would just be not used later on. So that is why I do not add omp parallel here

lidanqing-intel · 2021-11-18T08:10:07Z

paddle/fluid/operators/mkldnn/conv_mkldnn_op.cc

-      const int groups = ctx.Attr<int>("groups");
-      const std::string padding_algorithm =
-          ctx.Attr<std::string>("padding_algorithm");
+    const std::string fuse_activation =


Will const auto& avoid copy here and following assignment ?

This code I just touched due to indentation changes so I do not know. It sounds like a good idea to check that but I will not do that in this PR. Please add to our bugtracker.

jakpiase

LGTM

lidanqing-intel

LGTM

jczaja · 2021-11-19T09:26:36Z

@chenwhql Please review and approve PR-CI-APPROVAL. Problem is again with PADDLE_ENFORCE checker that works only on 2 out of 3 lines of PADDLE_ENFORCE.

lidanqing-intel · 2021-11-22T08:30:41Z

@baoachun Please review this

lidanqing-intel

LGTM

baoachun · 2021-11-29T08:03:45Z

@jczaja @lidanqing-intel, This pr needs to be blocked temporarily because it involves incompatible upgrades.

jczaja · 2021-11-29T09:56:34Z

@baoachun There is no API change in this PR. let me know if I can merge it

jczaja · 2021-11-29T13:59:16Z

@baoachun I want to clarify. There is no API change in this PR. SetMkldnnCacheCapacity() is still changing the capacity of cache as it used to be. With time we will update this function : SetMkldnnCacheCapacity to also change capacity of oneDNN internal cache. Even in a future it will not impact API e.g. SetMkldnnCacheCapacity will remain.

baoachun · 2021-12-01T08:38:51Z

@jczaja , then can I understand it like this: What you are modifying is the internal cache of oneDNN, and SetMkldnnCacheCapacity() is the cache of the modified paddle oneDNN, they are two types of cache, so the function of the SetMkldnnCacheCapacity() api will not change?

jczaja · 2021-12-01T10:59:06Z

@baoachun Yes. this is correct

baoachun · 2021-12-01T11:37:45Z

Hi @jczaja @lidanqing-intel, This pr will cause the performance of the quantitative model to decrease, we need to evaluate the risks brought by the code merge.

paddle-bot-old · 2021-12-04T02:37:52Z

Sorry to inform you that 326b7fc's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

paddle-bot-old · 2022-05-09T08:45:42Z

很抱歉，经过我们的反复讨论，你的PR暂未达到合入标准，请阅读飞桨原生算子开发规范，你可以重新提交新的PR，我们先将此PR关闭，感谢你的贡献。
Sorry to inform you that through our discussion, your PR fails to meet the merging standard (Reference: Paddle Custom Operator Design Doc). You can also submit an new one. Thank you.

- disabling caching

a4e2538

jczaja added the Intel label Nov 9, 2021

jczaja added 8 commits November 9, 2021 16:31

- draft of fix

567b1f4

- fix

0fb4bbf

- fix

8c7a5ba

- fix

40c529e

- fix

a637ca4

- fix

1d3d0d9

- fix

cf2503e

Fix to UT

a1fe2e5

jczaja requested review from jakpiase, wozna, arlesniak and lidanqing-intel November 16, 2021 10:01

ghost reviewed Nov 16, 2021

View reviewed changes

ghost previously approved these changes Nov 16, 2021

View reviewed changes

Silv3S previously approved these changes Nov 16, 2021

View reviewed changes

jakpiase reviewed Nov 16, 2021

View reviewed changes

- fixes after review

27ee49e

jczaja dismissed stale reviews from Silv3S and ghost via 27ee49e November 17, 2021 15:41

lidanqing-intel reviewed Nov 18, 2021

View reviewed changes

jakpiase previously approved these changes Nov 18, 2021

View reviewed changes

lidanqing-intel previously approved these changes Nov 19, 2021

View reviewed changes

- Rebase to develop

3731f2f

jczaja dismissed stale reviews from lidanqing-intel and jakpiase via 3731f2f November 24, 2021 16:08

jczaja added 4 commits November 24, 2021 18:16

Merge branch 'develop' into prv-disabling-conv-caching

2ee2c95

- workarounds for PADDLE_ENFORCE

25d0f70

- fixes

70f4aaa

- UT dummy ENFORCE workaround

326b7fc

lidanqing-intel approved these changes Nov 29, 2021

View reviewed changes

jczaja closed this May 9, 2022

paddle-bot-old bot added the status: not progressed label May 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to #34554 #37079

Fix to #34554 #37079

jczaja commented Nov 9, 2021

paddle-bot-old bot commented Nov 9, 2021

jczaja commented Nov 16, 2021

ghost Nov 16, 2021

jczaja Nov 16, 2021

ghost left a comment

Silv3S left a comment

jakpiase Nov 16, 2021

jczaja Nov 17, 2021

jakpiase Nov 16, 2021

jczaja Nov 17, 2021 •

edited

Loading

lidanqing-intel Nov 18, 2021 •

edited

Loading

jczaja Nov 18, 2021

lidanqing-intel Nov 19, 2021

lidanqing-intel Nov 18, 2021

jczaja Nov 18, 2021

jakpiase left a comment

lidanqing-intel left a comment

jczaja commented Nov 19, 2021

lidanqing-intel commented Nov 22, 2021

lidanqing-intel left a comment

baoachun commented Nov 29, 2021

jczaja commented Nov 29, 2021

jczaja commented Nov 29, 2021

baoachun commented Dec 1, 2021

jczaja commented Dec 1, 2021

baoachun commented Dec 1, 2021

paddle-bot-old bot commented Dec 4, 2021

paddle-bot-old bot commented May 9, 2022

Fix to #34554 #37079

Fix to #34554 #37079

Conversation

jczaja commented Nov 9, 2021

PR types

PR changes

Describe

paddle-bot-old bot commented Nov 9, 2021

jczaja commented Nov 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Silv3S left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jczaja Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

lidanqing-intel Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakpiase left a comment

Choose a reason for hiding this comment

lidanqing-intel left a comment

Choose a reason for hiding this comment

jczaja commented Nov 19, 2021

lidanqing-intel commented Nov 22, 2021

lidanqing-intel left a comment

Choose a reason for hiding this comment

baoachun commented Nov 29, 2021

jczaja commented Nov 29, 2021

jczaja commented Nov 29, 2021

baoachun commented Dec 1, 2021

jczaja commented Dec 1, 2021

baoachun commented Dec 1, 2021

paddle-bot-old bot commented Dec 4, 2021

paddle-bot-old bot commented May 9, 2022

jczaja Nov 17, 2021 •

edited

Loading

lidanqing-intel Nov 18, 2021 •

edited

Loading