[Quant] Use the true src zero point to query and create conv pd #90818

leslie-fang-intel · 2022-12-14T03:56:55Z

Stack from ghstack (oldest at bottom):

Summary
Previously, we use DNNL_RUNTIME_S32_VAL as the zero point for src in both weight prepack and convolution forward to ensure the same block format of weight is used. The problem is DNNL_RUNTIME_S32_VAL may query out a different block format weight comparing with the true zero point for src. It makes oneDNN convolution into jit path instead of brgconv path. Here we will use the true zero point for src to create pd and make reorder if it's a different block format weight as weight prepack generated.

Test Plan

python -m pytest quantization/core/test_quantized_op.py::TestQuantizedConv::test_conv_transpose_reorder_issue_onednn

cc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10

[ghstack-poisoned]

pytorch-bot · 2022-12-14T03:56:59Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/90818

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 0656a6e:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

…nv pd" **Summary** Previously, we use `DNNL_RUNTIME_S32_VAL` as the `zero point` for `src` in both weight prepack and convolution forward to ensure the same block format of weight is used. The problem is `DNNL_RUNTIME_S32_VAL` may query out a different block format weight comparing with the true `zero point` for `src`. It makes oneDNN convolution into `jit` path instead of `brgconv` path. Here we will use the true `zero point` for `src` to create pd and make reorder if it's a different block format weight as weight prepack generated. **Test Plan** ``` python -m pytest quantization/core/test_quantized_op.py::TestQuantizedConv::test_conv_reorder_issue_onednn ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]

leslie-fang-intel · 2022-12-14T04:10:54Z

@Xia-Weiwen Please help to take a look.

ghstack-source-id: f470c5e Pull Request resolved: #90818

…nv pd" **Summary** Previously, we use `DNNL_RUNTIME_S32_VAL` as the `zero point` for `src` in both weight prepack and convolution forward to ensure the same block format of weight is used. The problem is `DNNL_RUNTIME_S32_VAL` may query out a different block format weight comparing with the true `zero point` for `src`. It makes oneDNN convolution into `jit` path instead of `brgconv` path. Here we will use the true `zero point` for `src` to create pd and make reorder if it's a different block format weight as weight prepack generated. **Test Plan** ``` python -m pytest quantization/core/test_quantized_op.py::TestQuantizedConv::test_conv_reorder_issue_onednn ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]

ghstack-source-id: 1af2429 Pull Request resolved: #90818

jgong5 · 2022-12-14T07:43:46Z

aten/src/ATen/native/quantized/cpu/qconv.cpp

  }
  // Since src zero point is unknown, set runtime value here
-  op_attr.set_zero_points(DNNL_ARG_SRC, ideep::utils::tensor_zp_mask(1), {DNNL_RUNTIME_S32_VAL});
+  op_attr.set_zero_points(DNNL_ARG_SRC, ideep::utils::tensor_zp_mask(1), src_zero_points);


Are we use src zero point to initialize the primitive in the cache or use DNNL_RUNTIME_S32_VAL instead? If it is the latter, the primitive cache won't take effect any longer?

Are we use src zero point to initialize the primitive in the cache or use DNNL_RUNTIME_S32_VAL instead? If it is the latter, the primitive cache won't take effect any longer?

We used to use runtime value to create primitive and put it in cache. This PR uses true values of src zero point to create a primitive.
Src zero point is part of the cache key. If src zero point changes, cache misses. It has always been this behavior, not introduced by this PR.

OK. If we were using DNNL_RUNTIME_S32_VAL previously, the src zero point shouldn't be part of the key? Anyway, the change looks good then.

Xia-Weiwen

LGTM
BTW, try_reorder is no longer needed after this PR #90354 is landed which uses new ideep API. The try_reorder behavior is implemented inside ideep.

…nv pd" **Summary** Previously, we use `DNNL_RUNTIME_S32_VAL` as the `zero point` for `src` in both weight prepack and convolution forward to ensure the same block format of weight is used. The problem is `DNNL_RUNTIME_S32_VAL` may query out a different block format weight comparing with the true `zero point` for `src`. It makes oneDNN convolution into `jit` path instead of `brgconv` path. Here we will use the true `zero point` for `src` to create pd and make reorder if it's a different block format weight as weight prepack generated. **Test Plan** ``` python -m pytest quantization/core/test_quantized_op.py::TestQuantizedConv::test_conv_reorder_issue_onednn ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]

jerryzh168 · 2023-01-09T18:18:17Z

test/quantization/core/test_quantized_op.py

            qx = torch.quantize_per_tensor(x, scale=1.0, zero_point=0, dtype=torch.quint8)
            # The following should pass when input shape is changed
            torch.ops.quantized.conv2d(qx, w_packed, output_scale=1.0, output_zero_point=0)
+        # conv_transposed part


maybe split this into a separate test?

Thanks for the comment. Put it into a separate test.

…nv pd" **Summary** Previously, we use `DNNL_RUNTIME_S32_VAL` as the `zero point` for `src` in both weight prepack and convolution forward to ensure the same block format of weight is used. The problem is `DNNL_RUNTIME_S32_VAL` may query out a different block format weight comparing with the true `zero point` for `src`. It makes oneDNN convolution into `jit` path instead of `brgconv` path. Here we will use the true `zero point` for `src` to create pd and make reorder if it's a different block format weight as weight prepack generated. **Test Plan** ``` python -m pytest quantization/core/test_quantized_op.py::TestQuantizedConv::test_conv_reorder_issue_onednn ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]

leslie-fang-intel · 2023-01-11T06:12:26Z

@jerryzh168 I have changed according to your comments. Could you help to take a look of this PR again?

ghstack-source-id: ffa41ac Pull Request resolved: pytorch#90818

…nv pd" **Summary** Previously, we use `DNNL_RUNTIME_S32_VAL` as the `zero point` for `src` in both weight prepack and convolution forward to ensure the same block format of weight is used. The problem is `DNNL_RUNTIME_S32_VAL` may query out a different block format weight comparing with the true `zero point` for `src`. It makes oneDNN convolution into `jit` path instead of `brgconv` path. Here we will use the true `zero point` for `src` to create pd and make reorder if it's a different block format weight as weight prepack generated. **Test Plan** ``` python -m pytest quantization/core/test_quantized_op.py::TestQuantizedConv::test_conv_reorder_issue_onednn ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]

leslie-fang-intel · 2023-01-26T02:23:31Z

Hi @jerryzh168 Is there any other comments to this PR? Could you help to take a look again?

…nv pd" **Summary** Previously, we use `DNNL_RUNTIME_S32_VAL` as the `zero point` for `src` in both weight prepack and convolution forward to ensure the same block format of weight is used. The problem is `DNNL_RUNTIME_S32_VAL` may query out a different block format weight comparing with the true `zero point` for `src`. It makes oneDNN convolution into `jit` path instead of `brgconv` path. Here we will use the true `zero point` for `src` to create pd and make reorder if it's a different block format weight as weight prepack generated. **Test Plan** ``` python -m pytest quantization/core/test_quantized_op.py::TestQuantizedConv::test_conv_reorder_issue_onednn ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]

jerryzh168 · 2023-01-27T07:14:21Z

test/quantization/core/test_quantized_op.py

+        if 'onednn' not in supported_qengines:
+            return


I think you can use: https://github.com/pytorch/pytorch/blob/master/test/quantization/core/test_quantized_op.py#L3992

Good suggestions. Changed it.

…nv pd" **Summary** Previously, we use `DNNL_RUNTIME_S32_VAL` as the `zero point` for `src` in both weight prepack and convolution forward to ensure the same block format of weight is used. The problem is `DNNL_RUNTIME_S32_VAL` may query out a different block format weight comparing with the true `zero point` for `src`. It makes oneDNN convolution into `jit` path instead of `brgconv` path. Here we will use the true `zero point` for `src` to create pd and make reorder if it's a different block format weight as weight prepack generated. **Test Plan** ``` python -m pytest quantization/core/test_quantized_op.py::TestQuantizedConv::test_conv_reorder_issue_onednn ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]

leslie-fang-intel · 2023-01-31T01:21:50Z

@pytorchbot merge

pytorchmergebot · 2023-01-31T01:23:36Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

[Quant] Use the true src zero point to query and create pd

ff2ff41

[ghstack-poisoned]

leslie-fang-intel requested review from digantdesai, jerryzh168, jianyuh, kimishpatel, salilsdesai and z-a-f as code owners December 14, 2022 03:56

pytorch-bot bot added the release notes: quantization release notes category label Dec 14, 2022

leslie-fang-intel mentioned this pull request Dec 14, 2022

[Quant] Remove all the dequant nodes when the ref module has multi input args #90157

Closed

This was referenced Dec 14, 2022

[Quant] Update IDeep to support oneDNN conv add fusion #90605

Closed

[Quant] Add fused conv2d_add op for onednn backend #90262

Closed

[Quant] Add fused conv2d_add_relu op for onednn backend #90364

Closed

github-actions bot added the module: cpu CPU specific problem (e.g., perf, algorithm) label Dec 14, 2022

leslie-fang-intel marked this pull request as draft December 14, 2022 03:58

leslie-fang-intel added intel This tag is for PR from Intel ciflow/trunk Trigger trunk jobs on your pull request labels Dec 14, 2022

leslie-fang-intel requested review from Xia-Weiwen and jgong5 December 14, 2022 03:58

leslie-fang-intel changed the title ~~[Quant] Use the true src zero point to query and create pd~~ [Quant] Use the true src zero point to query and create conv pd Dec 14, 2022

pytorchbot added the open source label Dec 14, 2022

leslie-fang-intel added a commit that referenced this pull request Dec 14, 2022

[Quant] Use the true src zero point to query and create pd

590e108

ghstack-source-id: f470c5e Pull Request resolved: #90818

leslie-fang-intel added a commit that referenced this pull request Dec 14, 2022

[Quant] Use the true src zero point to query and create pd

5893426

ghstack-source-id: 1af2429 Pull Request resolved: #90818

jgong5 reviewed Dec 14, 2022

View reviewed changes

Xia-Weiwen approved these changes Dec 14, 2022

View reviewed changes

jgong5 approved these changes Dec 15, 2022

View reviewed changes

leslie-fang-intel added 4 commits January 4, 2023 10:22

jerryzh168 reviewed Jan 9, 2023

View reviewed changes

leslie-fang-intel added 3 commits January 10, 2023 10:16

leslie-fang-intel requested review from jerryzh168 and removed request for z-a-f January 10, 2023 02:52

leslie-fang-intel assigned leslie-fang-intel and unassigned leslie-fang-intel Jan 10, 2023

leslie-fang-intel added a commit to leslie-fang-intel/pytorch that referenced this pull request Jan 13, 2023

[Quant] Use the true src zero point to query and create pd

1d0e552

ghstack-source-id: ffa41ac Pull Request resolved: pytorch#90818

leslie-fang-intel added a commit to leslie-fang-intel/pytorch that referenced this pull request Jan 26, 2023

[Quant] Use the true src zero point to query and create pd

e15637f

ghstack-source-id: ffa41ac Pull Request resolved: pytorch#90818

leslie-fang-intel added 2 commits January 26, 2023 10:35

jerryzh168 reviewed Jan 27, 2023

View reviewed changes

jerryzh168 approved these changes Jan 27, 2023

View reviewed changes

leslie-fang-intel added 3 commits January 28, 2023 11:02

pytorchmergebot added the Merged label Jan 31, 2023

pytorchmergebot closed this in 21c7c7c Jan 31, 2023

facebook-github-bot deleted the gh/leslie-fang-intel/12/head branch June 8, 2023 17:51

[Quant] Use the true src zero point to query and create conv pd #90818

[Quant] Use the true src zero point to query and create conv pd #90818

Uh oh!

Conversation

leslie-fang-intel commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/90818

✅ No Failures

Uh oh!

leslie-fang-intel commented Dec 14, 2022

Uh oh!

jgong5 Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

Xia-Weiwen Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

jgong5 Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

Xia-Weiwen left a comment

Choose a reason for hiding this comment

Uh oh!

jerryzh168 Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

leslie-fang-intel Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

leslie-fang-intel commented Jan 11, 2023

Uh oh!

leslie-fang-intel commented Jan 26, 2023

Uh oh!

jerryzh168 Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

leslie-fang-intel Jan 28, 2023

Choose a reason for hiding this comment

Uh oh!

leslie-fang-intel commented Jan 31, 2023

Uh oh!

pytorchmergebot commented Jan 31, 2023

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

leslie-fang-intel commented Dec 14, 2022 •

edited

Loading

pytorch-bot bot commented Dec 14, 2022 •

edited

Loading