Skip to content

Conversation

jondea
Copy link
Contributor

@jondea jondea commented May 20, 2024

This will be used in PyTorch for accelerating dynamic quantization by using s8s8 matmul accelerated by ACL (see
uxlfoundation/oneDNN#1885). This can go in without oneDNN 3.5, but will only offer performance benefits after it does.

Also, do you have a policy on copyright headers? I noticed that on this branch at least, most of the copyright headers were removed a few years ago. However, Arm's usual policy when contributing to open source projects is to include a copyright header on any file which is modified. Would this be acceptable? If not, is there somewhere else suitable to note copyright?

This will be used in PyTorch for accelerating dynamic quantization by
using s8s8 matmul accelerated by ACL (see
uxlfoundation/oneDNN#1885). This can go in without
oneDNN 3.5, but will only offer performance benefits after it does.
Copy link

@jgong5 jgong5 left a comment

Choose a reason for hiding this comment

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

The change LGTM. I'm not sure about the copyright though. cc @jingxu10 and @Guobing-Chen

@jondea
Copy link
Contributor Author

jondea commented Jun 4, 2024

Thanks @jgong5. Given that this file doesn't have an existing copyright notice, would it be acceptable to add an Arm copyright line in the top level header file? This seems to be one of the only references to copyright on the pytorch_ideep branch

*Copyright (c) 2018 Intel Corporation.

@jondea
Copy link
Contributor Author

jondea commented Aug 7, 2024

I'm happy to get this in without the copyright notices, and work it out later. What's the process for getting this merged?

@yanbing-j
Copy link
Contributor

Do you include this change in PyTorch CI? If so, I can merge directly.
cc @Xia-Weiwen to review more.

@Xia-Weiwen
Copy link
Contributor

Do you include this change in PyTorch CI? If so, I can merge directly. cc @Xia-Weiwen to review more.

LGTM

@yanbing-j yanbing-j merged commit 8e21a72 into intel:ideep_pytorch Aug 13, 2024
pytorch-bot bot pushed a commit to pytorch/pytorch that referenced this pull request Aug 23, 2024
oneDNN+ACL has optimized kernels for s8s8 matmul, so input is signed.
This change leaves behaviour on all other platforms the same. This
change requires intel/ideep#313 to go in, and
oneDNN 3.5 for the optimized kernels. This change speeds up dynamic
quantized linear by ~10x.

Signed-off-by: Jonathan Deakin <jonathan.deakin@arm.com>
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Aug 24, 2024
oneDNN+ACL has optimized kernels for s8s8 matmul, so input is signed. This change leaves behaviour on all other platforms the same. This change requires intel/ideep#313 to go in, and oneDNN 3.5 for the optimized kernels. This change speeds up dynamic quantized linear by ~10x.

Also, do you have a policy on copyright headers? Arm's usual policy when contributing to open source projects is to include a copyright header on any file which is modified. Would this be acceptable? If not, is there somewhere else suitable to note copyright?

Pull Request resolved: #126687
Approved by: https://github.com/jgong5, https://github.com/malfet, https://github.com/snadampal

Co-authored-by: Nikita Shulga <2453524+malfet@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants