Unifying post-quantization properties#20724

DominikaJedynak · 2021-11-03T13:53:36Z

Description

Unifying requantize/dequantize post-quantization fuses for different operators into single property.

mxnet-bot · 2021-11-03T13:53:39Z

Hey @DominikaJedynak , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

To trigger all jobs: @mxnet-bot run ci [all]
To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [windows-gpu, unix-cpu, sanity, centos-cpu, clang, unix-gpu, miscellaneous, website, windows-cpu, edge, centos-gpu]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

DominikaJedynak · 2021-11-03T18:48:41Z

@mxnet-bot run ci [centos-gpu, unix-cpu]

mxnet-bot · 2021-11-03T18:48:48Z

Jenkins CI successfully triggered : [unix-cpu, centos-gpu]

DominikaJedynak · 2021-11-04T07:38:48Z

@mxnet-bot run ci [centos-gpu]

mxnet-bot · 2021-11-04T07:38:55Z

Jenkins CI successfully triggered : [centos-gpu]

DominikaJedynak · 2021-11-09T21:02:23Z

@mxnet-bot run ci [unix-cpu]

mxnet-bot · 2021-11-09T21:02:30Z

Jenkins CI successfully triggered : [unix-cpu]

bgawrych

Overall LGTM - one comment below

bgawrych · 2021-11-10T07:48:25Z

src/operator/subgraph/dnnl/dnnl_post_quantize_property.h

-    support_requantize_fusion_op_name.insert("_sg_onednn_conv");
-    support_requantize_fusion_op_name.insert("_contrib_quantized_elemwise_add");
-    support_requantize_fusion_op_name.insert("_contrib_quantized_npi_add");
+    disable_fuse_all                  = dmlc::GetEnv("MXNET_DISABLE_ONEDNN_FUSE_ALL", false);


What about changing this flag to something less general? I mean this flag doesn't disable all fuses (only the ones in this file). So better would be something like "MXNET_DISABLE_ONEDNN_FUSE_REQUANTIZE" and "MXNET_DISABLE_ONEDNN_FUSE_DEQUANTIZE". We also can negate default value to call it "MXNET_ONEDNN_FUSE_REQUANTIZE" and "MXNET_ONEDNN_FUSE_DEQUANTIZE".
+@anko-intel

Fully agree. "MXNET_ONEDNN_FUSE_REQUANTIZE" and "MXNET_ONEDNN_FUSE_DEQUANTIZE" are good proposals

mozga-intel

LGTM! Thanks!

mozga-intel · 2021-11-08T14:51:05Z

src/operator/subgraph/dnnl/dnnl_post_quantize_property.h

+class SgDNNLPostQuantizeSelector : public SubgraphSelectorV2 {
 public:
  /*! \brief pattern match status */
  enum SelectStatus {


[Future consideration] It could be fine to replace plain enum with class enum - to avoid implicitly conversion to other types (like another enum or int)

mozga-intel · 2021-11-10T09:57:49Z

src/operator/subgraph/dnnl/dnnl_post_quantize_property.h

 #include <string>
 #include <vector>

 #include "../../nn/dnnl/dnnl_convolution-inl.h"


[Future consideration] + @anko-intel

Suggested change

#include "../../nn/dnnl/dnnl_convolution-inl.h"

#include "./dnnl_convolution-inl.h"

This particular file is in nn/dnnl, not subgraph/dnnl, but I can change all paths in a way that avoids dots if you consider it more transparent

mozga-intel · 2021-11-10T09:59:49Z

src/operator/subgraph/dnnl/dnnl_post_quantize_property.h

+class SgDNNLPostQuantizeSelector : public SubgraphSelectorV2 {
 public:
  /*! \brief pattern match status */
  enum SelectStatus {


[Future consideration] Why the enum (enum SelectStatus) is public?

It was not part of my changes but you are right, it is better to make it private :)

mozga-intel · 2021-11-10T10:33:07Z

src/operator/subgraph/dnnl/dnnl_post_quantize_property.h

-    support_requantize_fusion_op_name.insert("_contrib_quantized_npi_add");
+  explicit SgDNNLPostQuantizeSelector(const bool dis_fuse_all, const bool dis_float_output)
+      : disable_fuse_all(dis_fuse_all), disable_float_output(dis_float_output) {
+    support_requantize_fusion_op_name = support_req_fusion_op;


What are the pros and cons of using this assignment? Roughly, why the set is global and then it's inexplicably assigned?

It is used in two different classes and is private in both, and I wanted to avoid duplicating. I will place this set inside anonymous namespace, as we discussed.

mozga-intel · 2021-11-10T11:43:44Z

src/operator/subgraph/dnnl/dnnl_post_quantize_property.h

+          if (n.outputs.size() > 1) {
+            // check if requantize have other outputs than dequantize
+            // if it has we can't fuse dequantize
+            for (auto kv : n.outputs) {


How about? If an object is const-> then kv is const, otherwise, the kv might be non-const.

Suggested change

for (auto kv : n.outputs) {

for (const auto kv : n.outputs) {

kv.first, which is the only place where kv is used, is marked const, but I can make kv const as well for clarity

mozga-intel · 2021-11-10T12:06:59Z

src/operator/subgraph/dnnl/dnnl_post_quantize_property.h

+        support_requantize_fusion_op_name.count(raw_node->op()->name)) {
+      status = kStart;
+      matched_list.clear();
+      matched_list.push_back(&n);


If we clear the vector here, the capacity is the same as before; Then it might be better to use emplace_back(&n) instead of push_back(n) ~ I hope to avoid creating temp object before.
How about using?

Suggested change

matched_list.push_back(&n);

matched_list.emplace_back(&n);

mozga-intel · 2021-11-10T12:08:54Z

src/operator/subgraph/dnnl/dnnl_post_quantize_property.h

 namespace op {

-class SgDNNLPostQuantizeSelector : public SubgraphSelector {
+const std::set<std::string> support_req_fusion_op = {"_contrib_quantized_elemwise_add",


This global set is likely enabled in all TU. What is an advantage of using it in this place?

Commented above

DominikaJedynak · 2021-11-23T23:07:31Z

@mxnet-bot run ci [unix-cpu, miscellaneous ]

mxnet-bot · 2021-11-23T23:07:36Z

Jenkins CI successfully triggered : [unix-cpu, miscellaneous]

DominikaJedynak · 2021-11-24T08:29:08Z

@mxnet-bot run ci [unix-cpu]

mxnet-bot · 2021-11-24T08:29:12Z

Jenkins CI successfully triggered : [unix-cpu]

DominikaJedynak · 2021-11-24T21:57:55Z

@mxnet-bot run ci [unix-cpu]

mxnet-bot · 2021-11-24T21:58:02Z

Jenkins CI successfully triggered : [unix-cpu]

mozga-intel

LGTM! Thanks!

mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 3, 2021

mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 3, 2021

mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 9, 2021

bgawrych approved these changes Nov 10, 2021

View reviewed changes

mozga-intel suggested changes Nov 10, 2021

View reviewed changes

mseth10 removed the pr-awaiting-review PR is waiting for code review label Nov 16, 2021

mseth10 added the pr-awaiting-testing PR is reviewed and waiting CI build and test label Nov 23, 2021

Small fix

1041fed

mseth10 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 23, 2021

DominikaJedynak force-pushed the unify_requantize_dequantize branch from 09a17c1 to 1041fed Compare November 23, 2021 22:08

mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 23, 2021

mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 24, 2021

mseth10 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 24, 2021

mozga-intel approved these changes Nov 25, 2021

View reviewed changes

bgawrych merged commit 26f9fa6 into apache:master Nov 25, 2021

DominikaJedynak deleted the unify_requantize_dequantize branch November 25, 2021 11:23

	#include "../../nn/dnnl/dnnl_convolution-inl.h"
	#include "./dnnl_convolution-inl.h"

	for (auto kv : n.outputs) {
	for (const auto kv : n.outputs) {

Conversation

DominikaJedynak commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

mxnet-bot commented Nov 3, 2021

Uh oh!

DominikaJedynak commented Nov 3, 2021

Uh oh!

mxnet-bot commented Nov 3, 2021

Uh oh!

DominikaJedynak commented Nov 4, 2021

Uh oh!

mxnet-bot commented Nov 4, 2021

Uh oh!

DominikaJedynak commented Nov 9, 2021

Uh oh!

mxnet-bot commented Nov 9, 2021

Uh oh!

bgawrych left a comment

Choose a reason for hiding this comment

Uh oh!

bgawrych Nov 10, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

mozga-intel left a comment

Choose a reason for hiding this comment

Uh oh!

mozga-intel Nov 8, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mozga-intel Nov 10, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

mozga-intel Nov 10, 2021 • 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!

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!

mozga-intel Nov 10, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominikaJedynak commented Nov 23, 2021

Uh oh!

mxnet-bot commented Nov 23, 2021

Uh oh!

DominikaJedynak commented Nov 24, 2021

Uh oh!

mxnet-bot commented Nov 24, 2021

Uh oh!

DominikaJedynak commented Nov 24, 2021

Uh oh!

mxnet-bot commented Nov 24, 2021

Uh oh!

mozga-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

DominikaJedynak commented Nov 3, 2021 •

edited

Loading

bgawrych Nov 10, 2021 •

edited

Loading

mozga-intel Nov 8, 2021 •

edited

Loading

mozga-intel Nov 10, 2021 •

edited

Loading

mozga-intel Nov 10, 2021 •

edited

Loading

mozga-intel Nov 10, 2021 •

edited

Loading