Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qnnpack quantized activations: fix memory format issues #46077

Closed
wants to merge 2 commits into from

Conversation

vkuzo
Copy link
Contributor

@vkuzo vkuzo commented Oct 9, 2020

Stack from ghstack:

Summary:

Some of QNNPACK quantized kernels were not handling NHWC correctly,
the data written respected the input format but the memory flag
was always set to contiguous. This led to incorrect outputs. This PR

  1. adds testing for NHWC for qnnpack activations
  2. fixes those activations which did not set the memory format on the output so that the new tests pass

Test Plan:

python test/test_quantization.py TestQuantizedOps.test_qhardsigmoid
python test/test_quantization.py TestQuantizedOps.test_leaky_relu
python test/test_quantization.py TestQuantizedOps.test_hardswish
python test/test_quantization.py TestQNNPackOps.test_qnnpack_tanh
python test/test_quantization.py TestQNNPackOps.test_qnnpack_sigmoid

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D24213257

Summary:

Some of QNNPACK quantized kernels were not handling NHWC correctly,
the data written respected the input format but the memory flag
was always set to contiguous.  This PR
1. adds testing for NHWC for qnnpack activations
2. fixes those activations which did not set the memory format on the output

Test Plan:

```
python test/test_quantization.py TestQuantizedOps.test_qhardsigmoid
python test/test_quantization.py TestQuantizedOps.test_leaky_relu
python test/test_quantization.py TestQuantizedOps.test_hardswish
python test/test_quantization.py TestQNNPackOps.test_qnnpack_tanh
python test/test_quantization.py TestQNNPackOps.test_qnnpack_sigmoid
```

Reviewers:

Subscribers:

Tasks:

Tags:

[ghstack-poisoned]
vkuzo added a commit that referenced this pull request Oct 9, 2020
Summary:

Some of QNNPACK quantized kernels were not handling NHWC correctly,
the data written respected the input format but the memory flag
was always set to contiguous.  This PR
1. adds testing for NHWC for qnnpack activations
2. fixes those activations which did not set the memory format on the output

Test Plan:

```
python test/test_quantization.py TestQuantizedOps.test_qhardsigmoid
python test/test_quantization.py TestQuantizedOps.test_leaky_relu
python test/test_quantization.py TestQuantizedOps.test_hardswish
python test/test_quantization.py TestQNNPackOps.test_qnnpack_tanh
python test/test_quantization.py TestQNNPackOps.test_qnnpack_sigmoid
```

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: b9422fa0fd8dce1f5c1b94e7edf1fccc4b635091
Pull Request resolved: #46077
@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #46077 into gh/vkuzo/153/base will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@                  Coverage Diff                  @@
##           gh/vkuzo/153/base   #46077      +/-   ##
=====================================================
- Coverage              68.19%   68.19%   -0.01%     
=====================================================
  Files                    410      410              
  Lines                  53439    53439              
=====================================================
- Hits                   36445    36444       -1     
- Misses                 16994    16995       +1     
Impacted Files Coverage Δ
torch/testing/_internal/expecttest.py 77.55% <0.00%> (-1.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ffd2af...349e579. Read the comment docs.

dtype=torch_type)
qY_hat = op(qX, negative_slope=alpha)
self.assertEqual(qY.dequantize(), qY_hat.dequantize(),
msg="F.leaky_relu failed ({} vs {})".format(qY, qY_hat))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For leaky_relu and the ones below, should we be using _test_activation_function to be consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be valuable for the future. We are hoping to land this urgently (FB teams blocked by this, and also to be able to pick into OSS v1.7), so maybe we can improve the test infra as a follow-up PR. I would also recommend reducing our exposure to hypothesis in these kinds of tests, it seems to be a net negative in my experience so far, as the tensor values it gives are not representative of real inputs, which could mask issues.

Copy link
Contributor

@supriyar supriyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! This is a sneaky bug

Summary:

Some of QNNPACK quantized kernels were not handling NHWC correctly,
the data written respected the input format but the memory flag
was always set to contiguous.  This led to incorrect outputs.  This PR
1. adds testing for NHWC for qnnpack activations
2. fixes those activations which did not set the memory format on the output so that the new tests pass

Test Plan:

```
python test/test_quantization.py TestQuantizedOps.test_qhardsigmoid
python test/test_quantization.py TestQuantizedOps.test_leaky_relu
python test/test_quantization.py TestQuantizedOps.test_hardswish
python test/test_quantization.py TestQNNPackOps.test_qnnpack_tanh
python test/test_quantization.py TestQNNPackOps.test_qnnpack_sigmoid
```

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D24213257](https://our.internmc.facebook.com/intern/diff/D24213257)

[ghstack-poisoned]
vkuzo added a commit that referenced this pull request Oct 9, 2020
Summary:

Some of QNNPACK quantized kernels were not handling NHWC correctly,
the data written respected the input format but the memory flag
was always set to contiguous.  This PR
1. adds testing for NHWC for qnnpack activations
2. fixes those activations which did not set the memory format on the output

Test Plan:

```
python test/test_quantization.py TestQuantizedOps.test_qhardsigmoid
python test/test_quantization.py TestQuantizedOps.test_leaky_relu
python test/test_quantization.py TestQuantizedOps.test_hardswish
python test/test_quantization.py TestQNNPackOps.test_qnnpack_tanh
python test/test_quantization.py TestQNNPackOps.test_qnnpack_sigmoid
```

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: a3e079fa48f68e0a258129a439af5cac80a43848
Pull Request resolved: #46077
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 138c22f.

malfet pushed a commit to malfet/pytorch that referenced this pull request Oct 12, 2020
Summary:
Pull Request resolved: pytorch#46077

Some of QNNPACK quantized kernels were not handling NHWC correctly,
the data written respected the input format but the memory flag
was always set to contiguous.  This PR
1. adds testing for NHWC for qnnpack activations
2. fixes those activations which did not set the memory format on the output

Test Plan:
```
python test/test_quantization.py TestQuantizedOps.test_qhardsigmoid
python test/test_quantization.py TestQuantizedOps.test_leaky_relu
python test/test_quantization.py TestQuantizedOps.test_hardswish
python test/test_quantization.py TestQNNPackOps.test_qnnpack_tanh
python test/test_quantization.py TestQNNPackOps.test_qnnpack_sigmoid
```

Imported from OSS

Reviewed By: supriyar

Differential Revision: D24213257

fbshipit-source-id: 764fb588a8d8a0a6e6e4d86285904cdbab26d487
@vkuzo vkuzo mentioned this pull request Oct 12, 2020
malfet added a commit that referenced this pull request Oct 13, 2020
)

Summary:
Pull Request resolved: #46077

Some of QNNPACK quantized kernels were not handling NHWC correctly,
the data written respected the input format but the memory flag
was always set to contiguous.  This PR
1. adds testing for NHWC for qnnpack activations
2. fixes those activations which did not set the memory format on the output

Test Plan:
```
python test/test_quantization.py TestQuantizedOps.test_qhardsigmoid
python test/test_quantization.py TestQuantizedOps.test_leaky_relu
python test/test_quantization.py TestQuantizedOps.test_hardswish
python test/test_quantization.py TestQNNPackOps.test_qnnpack_tanh
python test/test_quantization.py TestQNNPackOps.test_qnnpack_sigmoid
```

Imported from OSS

Reviewed By: supriyar

Differential Revision: D24213257

fbshipit-source-id: 764fb588a8d8a0a6e6e4d86285904cdbab26d487

Co-authored-by: Vasiliy Kuznetsov <vasiliy@fb.com>
@facebook-github-bot facebook-github-bot deleted the gh/vkuzo/153/head branch October 13, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants