-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
[Quant] Add fused ConvAdd2d module for onednn backend #91152
[Quant] Add fused ConvAdd2d module for onednn backend #91152
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/91152
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 2366a9f: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
ghstack-source-id: 83915ff79444b1e26eb6afda3112ef599342e944 Pull Request resolved: pytorch#91152
cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
ghstack-source-id: 73cd56e761d35b1dab38ebf2429ac54d51f552ef Pull Request resolved: pytorch#91152
cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
ghstack-source-id: 82712796ce3eb709f9225d52eb0d174291392659 Pull Request resolved: pytorch#91152
cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
ghstack-source-id: 426bded8b3473fe7277b6fd8e99f9752e0b49e16 Pull Request resolved: pytorch#91152
ghstack-source-id: 2dfaa66d06a073235e5d077d4274ae34c20dad8e Pull Request resolved: pytorch#91152
ghstack-source-id: 2dfaa66d06a073235e5d077d4274ae34c20dad8e Pull Request resolved: pytorch#91152
**Summary** Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused `ConvAdd2d` module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown. **Test plan** ``` python -m pytest test_quantization.py -k test_conv2d_add ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
**Summary** Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused `ConvAdd2d` module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown. **Test plan** ``` python -m pytest test_quantization.py -k test_conv2d_add ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
**Summary** Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused `ConvAdd2d` module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown. **Test plan** ``` python -m pytest test_quantization.py -k test_conv2d_add ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
@@ -274,14 +274,26 @@ def _test_conv_api_impl( | |||
batch_size, in_channels_per_group, input_feature_map_size, | |||
out_channels_per_group, groups, kernel_size, X_scale, X_zero_point, | |||
W_scale, W_zero_point, use_bias, use_channelwise) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are a lot of if/else here in this function, I'm wondering if we can pass them as arguments,
e.g. can we pass
1). inputs to the module as arguments ([[X_q, X2_q]] if (post_op == "add") else [[X_q]])
2). separate the fused_conv_module and the individual conv_module as two arguments, to avoid branches in L296-303
3). any other refactors to remove branches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions. I have removed most of the if/else branches in this function. Please help to take a look again @jerryzh168.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove the branches as much as we can to make the code clearer
**Summary** Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused `ConvAdd2d` module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown. **Test plan** ``` python -m pytest test_quantization.py -k test_conv2d_add ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
**Summary** Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused `ConvAdd2d` module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown. **Test plan** ``` python -m pytest test_quantization.py -k test_conv2d_add ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
**Summary** Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused `ConvAdd2d` module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown. **Test plan** ``` python -m pytest test_quantization.py -k test_conv2d_add ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
**Summary** Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused `ConvAdd2d` module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown. **Test plan** ``` python -m pytest test_quantization.py -k test_conv2d_add ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
Most of the branches has been removed. |
**Summary** Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused `ConvAdd2d` module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown. **Test plan** ``` python -m pytest test_quantization.py -k test_conv2d_add ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
**Summary** Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused `ConvAdd2d` module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown. **Test plan** ``` python -m pytest test_quantization.py -k test_conv2d_add ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
Hi @jerryzh168, thanks for your review. the previous comments has been fixed. Could you kindly take a look of this PR again? |
batch_size = 2 | ||
in_channels_per_group = 2 | ||
H = 8 | ||
W = 8 | ||
out_channels_per_group = 2 | ||
groups = 3 | ||
kernel_h = 3 | ||
kernel_w = 3 | ||
stride_h = 2 | ||
stride_w = 2 | ||
pad_h = 1 | ||
pad_w = 1 | ||
dilation = 1 | ||
# Tests the correctness of the conv2d module. | ||
in_channels = in_channels_per_group * groups | ||
out_channels = out_channels_per_group * groups | ||
input_feature_map_size = (H, W) | ||
kernel_size = (kernel_h, kernel_w) | ||
stride = (stride_h, stride_w) | ||
padding = (pad_h, pad_w) | ||
dilation = (dilation, dilation) | ||
X_scale = 1.3 | ||
X_zero_point = 2 | ||
W_scale = [0.5] | ||
W_zero_point = [0] if qengine_is_onednn() else [3] | ||
Y_scale = 5.0 | ||
Y_zero_point = 4 | ||
qconv_cls = nniq.ConvReLU2d | ||
module_name = "QuantizedConvReLU2d" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you move these outside of the loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments. Done.
for use_bias, use_channelwise in options: | ||
if torch.backends.quantized.engine == "qnnpack": | ||
use_channelwise = False | ||
batch_size = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished.
[True, False], # use_channelwise | ||
) | ||
for pad_mode, use_bias, use_channelwise in options: | ||
batch_size = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also finished here.
class ConvAdd2d(_FusedModule): | ||
r"""This is a sequential container which calls the Conv2d modules with extra Add. | ||
During quantization this will be replaced with the corresponding fused module.""" | ||
def __init__(self, add, conv): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we swap the order of conv and add, to be more consistent with the other ops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, changed the order to make it more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, had a few comments inline
def forward(self, x1, x2): | ||
return self.add(self[0](x1), x2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also this feels a bit hacky, i'm wondering how we can extend _FusedModule to support this use case..
**Summary** Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused `ConvAdd2d` module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown. **Test plan** ``` python -m pytest test_quantization.py -k test_conv2d_add ``` cc jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 [ghstack-poisoned]
@pytorchbot merge |
Merge startedYour 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 |
Stack from ghstack (oldest at bottom):
Summary
Post op fusion can reduce data movement overhead and improve inference performance. This PR adds fused
ConvAdd2d
module for onednn backend, which will be used for int8 inference with onednn backend. Cannot call this module with other quantization backends otherwise an error is thrown.Test plan
cc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10