[aarch64] Fix aarch64 build so that quantize_val_arm is defined

[psaab]

Summary: quantize_val_arm is used in the kernels when building under aarch64

Test Plan: CI

Differential Revision: D39272746

[facebook-github-bot]

🔗 Helpful links

• 🧪  See artifacts and rendered test results at hud.pytorch.org/pr/84564
• ✖️ Python docs build was skipped
• ✖️ C++ docs build was skipped
• ❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (1 Pending)

As of commit ca4696f (more details on the Dr. CI page):

Expand to see more

---

Commit ca4696f was recently pushed. Waiting for builds...

---

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D39272746

[kimishpatel]

Why is this qualification needed?

[psaab]

Otherwise in aarch64 builds quantize_val_arm doesn't get defined because it isn't in the fbgemm case. If there's a better way to deal with this I'm all for it.

…

On Tue, Sep 6, 2022 at 8:00 AM Kimish Patel ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In aten/src/ATen/native/quantized/AffineQuantizerBase.cpp <#84564 (comment)>: > @@ -33,7 +33,7 @@ void checkZeroPoint(const std::string& fn_name, int64_t zero_point) { } // anonymous namespace -#ifdef USE_FBGEMM +#if defined(USE_FBGEMM) && !defined(__aarch64__) Why is this qualification needed? — Reply to this email directly, view it on GitHub <#84564 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABQCUVF2GII66J2NS3NU3TV45L7XANCNFSM6AAAAAAQFZR4GI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[kimishpatel]

quantize_val_arm

I dont follow. quantize_val_arm is defined "if you are not USE_FBGEMM". Your changes say define quantize_val_arm "if you are not USE_FBGEMM OR you are aarch64". But if you are building for aarch64 you shouldnt really have USE_FBGEMM=1. SO not sure why the build would fail.

quantize_val_arm is actually used here https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/quantized/cpu/kernels/QuantizedOpKernels.cpp#L3373. So if you want to use appropriate macros to define it, a better thing to do would be to refactor that such that quantize_val_arm uses this, https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/quantized/cpu/kernels/QuantizedOpKernels.cpp#L3274.

I do agree though that this part of the code can be refactored for better readability. It is a bit of a mess.

[psaab]

I took the easy way out to get things working. I'll refactor. Thx

…

On Tue, Sep 6, 2022 at 8:33 AM Kimish Patel ***@***.***> wrote: quantize_val_arm I dont follow. quantize_val_arm is defined "if you are not USE_FBGEMM". Your changes say define quantize_val_arm "if you are not USE_FBGEMM OR you are aarch64". But if you are building for aarch64 you shouldnt really have USE_FBGEMM=1. SO not sure why the build would fail. quantize_val_arm is actually used here https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/quantized/cpu/kernels/QuantizedOpKernels.cpp#L3373. So if you want to use appropriate macros to define it, a better thing to do would be to refactor that such that quantize_val_arm uses this, https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/quantized/cpu/kernels/QuantizedOpKernels.cpp#L3274 . I do agree though that this part of the code can be refactored for better readability. It is a bit of a mess. — Reply to this email directly, view it on GitHub <#84564 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABQCUVJQJTQFBAK2EK3SF3V45P27ANCNFSM6AAAAAAQFZR4GI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[kimishpatel]

I took the easy way out to get things working. I'll refactor. Thx
…
On Tue, Sep 6, 2022 at 8:33 AM Kimish Patel @.> wrote: quantize_val_arm I dont follow. quantize_val_arm is defined "if you are not USE_FBGEMM". Your changes say define quantize_val_arm "if you are not USE_FBGEMM OR you are aarch64". But if you are building for aarch64 you shouldnt really have USE_FBGEMM=1. SO not sure why the build would fail. quantize_val_arm is actually used here https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/quantized/cpu/kernels/QuantizedOpKernels.cpp#L3373. So if you want to use appropriate macros to define it, a better thing to do would be to refactor that such that quantize_val_arm uses this, https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/quantized/cpu/kernels/QuantizedOpKernels.cpp#L3274 . I do agree though that this part of the code can be refactored for better readability. It is a bit of a mess. — Reply to this email directly, view it on GitHub <#84564 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABQCUVJQJTQFBAK2EK3SF3V45P27ANCNFSM6AAAAAAQFZR4GI . You are receiving this because you authored the thread.Message ID: @.>

Sure but were you building for aarch64 with USE_FBGEMM=1? I dont think you want to do that. And if you dont do that, it should likely just work out of the box.

[psaab]

Well, we're trying to actually use simde to wrap a lot of the AVX/SSE code for aarch64 vs do a direct aarch64 port.

[jianyuh]

I guess ideally we would have QNNPACK for arm on mobile side for PyTorch, but @psaab is also exploring the option of using arm for server side (so checking if FBGEMM is working with simde conversion in FBGEMM PR like pytorch/FBGEMM#1271 ).

[kimishpatel]

Looks ok for now. Do leave a comment as to a) why do this and b) maybe a better refactor might be to get _arm variants into arm specific macros.

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D39272746

[psaab]

@pytorchbot merge -g

[pytorchmergebot]

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the green (-g) flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

[pytorchmergebot]

Merge failed

Reason: The following mandatory check(s) failed (Rule superuser):

• pull

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

[psaab]

@pytorchbot merge -g

[pytorchmergebot]

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the green (-g) flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

[pytorchmergebot]

Merge failed

Reason: The following mandatory check(s) failed (Rule superuser):

• pull

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D39272746

[ajtulloch]

@pytorchbot merge -g

[pytorchmergebot]

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the green (-g) flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

[github-actions]

Hey @psaab.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.