-
Notifications
You must be signed in to change notification settings - Fork 227
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
aarch64: apply the cherrypicked onednn PR-1768 #1717
Conversation
This is to improve the torch.compile() perf by 5.8x on AWS Graviton3 instances. This patching is required till PyTorch oneDNN is upgraded to v3.4.
1e01d5d
to
32d9e8e
Compare
This is to improve the torch.compile() perf by 5.8x on AWS Graviton3 instances. This patching is required till PyTorch oneDNN is upgraded to v3.4.
Why have we landed this change? If it missed the deadline for oneDNN branch cut, then it should wait until the next release or needs to be cherry-picked into one |
@@ -110,6 +110,9 @@ def parse_arguments(): | |||
print("Applying mkl-dnn patch to fix crash due to /sys not accesible") | |||
os.system("cd /pytorch/third_party/ideep/mkl-dnn && patch -p1 < /builder/mkldnn_fix/fix-xbyak-failure.patch") | |||
|
|||
print("Applying mkl-dnn patch to improve torch.compile() perf") | |||
os.system("cd /pytorch/third_party/ideep/mkl-dnn && patch -p1 < /builder/mkldnn_fix/onednn-pr1768-aarch64-add-acl-sbgemm-inner-product-primitive.patch") # noqa: E501 |
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.
Not to author, reviewer: I don't know why those were accepted initially, but one should avoid using os.system
as much as possible, as it gives no signal back on whether the underlying command succeeded or failed, this way making regression detection a much harder process. (i.e. one can add os.system("fokjhdsfkjhsef");
anywhere in this script and it will be just a no-op)
subprocess.check_call
is a better way of doing that, for example like that:
builder/aarch64_linux/aarch64_wheel_ci_build.py
Lines 111 to 112 in ab5fc90
with open("/builder/mkldnn_fix/fix-xbyak-failure.patch") as f: | |
check_call(["patch", "-p1"], stdin=f, cwd="/pytorch/third_party/ideep/mkl-dnn") |
This way typo in patch name or failure to apply the patch will result in the failure
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.
Sounds good. Sorry about missing this one. I see this commit adresses exactly that: 0926610
This is to improve the torch.compile() perf by 5.8x on AWS Graviton3 instances. This patching is required till PyTorch oneDNN is upgraded to v3.4.