-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
[CI/Build] fix cpu_extension for apple silicon #21195
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
[CI/Build] fix cpu_extension for apple silicon #21195
Conversation
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
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.
Code Review
This pull request fixes a build issue for Apple Silicon CPUs by enabling ASIMD support and introducing a check_sysctl
function. To improve robustness, I've identified a critical issue regarding argument quoting in the new check_sysctl
function that should be addressed.
537a376
to
34d30da
Compare
addressed commit signing issue |
@@ -70,7 +86,10 @@ endfunction() | |||
is_avx512_disabled(AVX512_DISABLED) | |||
|
|||
if (MACOSX_FOUND AND CMAKE_SYSTEM_PROCESSOR STREQUAL "arm64") | |||
set(APPLE_SILICON_FOUND TRUE) |
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.
removed APPLE_SILICON_FOUND as it wasn't used downstream anymore but maybe it's still useful to keep it for future reference/use?
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.
Yeah this would be useful to follow the pattern and use in downstream code
Signed-off-by: ignaciosica <[email protected]>
Signed-off-by: ignaciosica <[email protected]>
Signed-off-by: ignaciosica <[email protected]>
34d30da
to
e25e799
Compare
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.
LGTM I think this is reasonable and shouldn't be disruptive to other backends
Signed-off-by: ignaciosica <[email protected]>
Signed-off-by: ignaciosica <[email protected]> Signed-off-by: shuw <[email protected]>
Signed-off-by: ignaciosica <[email protected]> Signed-off-by: x22x22 <[email protected]>
Signed-off-by: ignaciosica <[email protected]>
Signed-off-by: ignaciosica <[email protected]>
Signed-off-by: ignaciosica <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
Signed-off-by: ignaciosica <[email protected]> Signed-off-by: Paul Pak <[email protected]>
Signed-off-by: ignaciosica <[email protected]>
Signed-off-by: ignaciosica <[email protected]> Signed-off-by: Boyuan Feng <[email protected]>
Signed-off-by: ignaciosica <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
Signed-off-by: ignaciosica <[email protected]>
Signed-off-by: ignaciosica <[email protected]>
Purpose
The purpose for this pr is to fix the cpu installation from source for apple silicon CPUs. Recently, basic serving capabilities and examples stopped working in my local setup (M3 pro). I bisected my way to this pr #14129 which introduced int8 quantization for ARM CPU. The problem lied on the fact that although apple silicon shared some codepath with the rest of the arm cpu in
cpu_extension.cmake
, it also had some specific configurations that ended up breaking build after the int8 support was introduced; more specifically, apple silicon pathway was not enabling ASIMD_FOUND thus not includingquant.cpp
(cpu_extension.cmake:284
) source.After a fresh install from source
Basic example failed with
In order to fix, this pr enabled ASIMD_FOUND for apple silicon as well. For safety, it checked for support with the following command:
sysctl -n hw.optional.neon
. As far as I know, all apple silicon, starting from M1 up to M4 generation support this feature, but still decided to gate ASIMD_FOUND on this check in the case the support is dropped for future generations.After this fix, cpu installation from source started working again.
This pr also enables bf16 support for apple silicon. This feature is gated via the following check hw.optional.arm.FEAT_BF16. Based on this LLVM's commit, bf16 support was introduced for cpu in m2 generation.
Test Plan
Unfortunately I'm not familiar enough with build runs in CI, I would appreciate some guidance for this point.