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

[CPU] [ARM] SVE FP16 functions for MHASingleToken kernel #28182

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

NishantPrabhuFujitsu
Copy link
Contributor

Details

In continuation with #27273, adds SVE FP16 implementations for functions called during execution of MHASingleToken for SVE-128, SVE-256 and SVE-512 platforms. SVE implementations are compiled only if runtime support for SVE is detected on the hardware, otherwise it falls back to Neon.

Benchmarking results

Below are the benchmarking results of execution time of each ported function. Measurements were performed by running each function individually on dummy inputs (128 fp16 elements) for 1,000,000 iterations and computing average time (in micro-seconds).

image

@NishantPrabhuFujitsu NishantPrabhuFujitsu requested review from a team as code owners December 23, 2024 10:30
@github-actions github-actions bot added category: CPU OpenVINO CPU plugin category: build OpenVINO cmake script / infra labels Dec 23, 2024
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label Dec 23, 2024
@NishantPrabhuFujitsu
Copy link
Contributor Author

@dmitry-gorokhov you might remember a conversation we had about the inference output not being correct when the new exp_ps_sve implementation I added in #27273 is used in exp_reduce_sum. This issue is no longer happening, i.e. inference is working fine with our new exp(x) implementation.

I am not 100% sure what change caused this to get fixed but I will try to find out more soon.

@dmitry-gorokhov dmitry-gorokhov added this to the 2025.0 milestone Dec 23, 2024
@dmitry-gorokhov
Copy link
Contributor

build_jenkins

@NishantPrabhuFujitsu
Copy link
Contributor Author

I'm not very sure if the fix I just pushed for https://github.com/openvinotoolkit/openvino/actions/runs/12465345215/job/34792490178?pr=28182#step:15:3723 is correct. Could you please check if it would work?

@dmitry-gorokhov
Copy link
Contributor

build_jenkins

@dmitry-gorokhov
Copy link
Contributor

So we have Android ARM build failures. It happens because Android host uses compiler that actually doesn't support SVE, but we are still trying to cross-compile MHA for SVE isa.
To fix this we need to adjust ENABLE_SVE option based on ov_check_compiler_supports_sve check. I think we should add such check here https://github.com/openvinotoolkit/openvino/blob/master/cmake/developer_package/features.cmake#L108 (as it done for avx512).

@NishantPrabhuFujitsu NishantPrabhuFujitsu force-pushed the mha-single-token-arm-sve-f16 branch from e7574bd to 41a4a26 Compare December 25, 2024 08:46
@NishantPrabhuFujitsu
Copy link
Contributor Author

NishantPrabhuFujitsu commented Dec 25, 2024

I've added compiler version checks for clang (>13, source) and gcc (>10, source) in features.cmake. I couldn't find information about MSVC, and SVE is not supported on Apple platforms anyway. Hopefully this will be sufficient.

For the other failing pipelines, I remember those being CI issues encountered in #27273 as well. I hope that's still the case.

@dmitry-gorokhov
Copy link
Contributor

dmitry-gorokhov commented Dec 25, 2024

I've added compiler version checks for clang (>13, source) and gcc (>10, source) in features.cmake. I couldn't find information about MSVC, and SVE is not supported on Apple platforms anyway. Hopefully this will be sufficient.

@NishantPrabhuFujitsu As I mentioned instead of compiler versions check it is more sufficient to implement condition based on ov_check_compiler_supports_sve call. ov_check_compiler_supports_sve tries to compile SVE code with given compiler - that seems to be most reliable way to check if SVE is supported.

@NishantPrabhuFujitsu
Copy link
Contributor Author

NishantPrabhuFujitsu commented Dec 26, 2024

@dmitry-gorokhov In that case, I'm not sure I understand what I need to change.

For Android platforms, ov_check_compiler_supports_sve check already seems to work as expected: CXX_HAS_SVE check fails (see CI logs, I've taken this from #27841 but I remember seeing the same for this PR) and the next two lines state that SVE is not supported on the platform. Isn't that enough to prevent SVE from being cross-compiled on Android?

If my understanding is wrong, please help understand how exactly this works and what adjustment to the check would fix it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: build OpenVINO cmake script / infra category: CPU OpenVINO CPU plugin ExternalPR External contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants