kleidiai: Add runtime feature detection mechanism for aarch64/kleidiai - #26076
Conversation
|
Just said I'd comment here also I opened 3 pr's as a stack to give context to the work here in this pr, a bot has commented on the other two stating that its against the contributing guidelines which is correct as I am a new contributor, however they are draft and the intent is to give context to the intent of this work and subsequent work in this area. Originally all 3 were a single PR however for the sake of review it was decided to split into 3. I can close them if required but I felt it necessary to open and link at least initially. |
|
@ggml-gh-bot review |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Automated code reviewI have enough to write the review. Static review of PR #26076 - runtime feature detection for aarch64/kleidiaiRunning the General, ggml/backend, Security, and Approach and design checklists (the diff adds a new shared runtime-detection header and rewrites SMCU detection, all under Blocking
Will slow the review
Nits
This review was generated automatically by pi coding agent using |
| errno = 0; | ||
| const unsigned long long value = strtoull(name + 3, &end, 10); | ||
| if (errno != 0 || *end != '\0' || | ||
| value > (unsigned long long) std::numeric_limits<size_t>::max()) { |
There was a problem hiding this comment.
| value > (unsigned long long) std::numeric_limits<size_t>::max()) { | |
| value > (size_t) std::numeric_limits<size_t>::max()) { |
There was a problem hiding this comment.
I rewrote the function altogether into more modern c++ which removes the need for the unsigned types also.
|
Btw can you see if the review from the bot makes sense? #26076 (comment) |
| // conservative policy and only treat zero affinity as private. | ||
| const uint32_t sh = (uint32_t)((smidr >> 13) & 0x3); | ||
| const uint32_t nsmc = (uint32_t)((smidr >> 56) & 0xF); | ||
| const size_t shared_count = nsmc == 0xF ? 1 : (size_t)nsmc + 1; |
There was a problem hiding this comment.
SMIDR_EL1.NSMC == 0b1111 is a reserved encoding. Mapping it to one shared SMCU seems unsafe, as it treats an invalid value as valid. Could we return here instead?
There was a problem hiding this comment.
So while its true that if we had an nsmc value of 0xF its a reserved value and something that I wouldn't expect so see (currently) the reason we assume 1 is that the there is no valid encoding for 0 smcu's. So if SH and affinity can reliably identify a shared group then it must indicate that the group contains a minimum of 1.
Returning if we seen this value, might cause us to incorrectly assume there are no SMCU and not leverage the absolute minimum number. I think maybe a log message might be appropriate if we encounter this instead of a return? What do you think @chaxu01
There was a problem hiding this comment.
That sounds reasonable. A one-time warning (or documenting it as "unknown count, assuming a minimum of one shared SMCU") would address that for me.
There was a problem hiding this comment.
Added the log warning in the latest push @chaxu01
This is incorrect, the correct bits are used for SME2 and other features in this code, e.g. https://github.com/torvalds/linux/blob/master/arch/arm64/include/uapi/asm/hwcap.h#L115 |
This is also incorrect, The defines for NSMC as bits [59:56], not [63:56], and as the number of SMCUs minus one associated with a particular {Affinity2, Affinity} group. It is neither a global system count nor a PE count. Our code correctly:
For example, eight PEs reporting affinity A with NSMC=3 produce four SMCUs, not 32 |
So this code was moved from cpu_feats.cpp file (and added to). The existing ggml_cpu_has code, while used in places as a runtime feature detect are all entirely compile time detection, so I in theory if using ggml_cpu_has_sme2 function could compile with a compiler that emits the correct sme define but then take that binary and run it on a non sme device causing a segfault. The intent here was to reuse and centralize something that was already in the code base and which enables us to do true runtime feature detection. I would be open to moving this to a more arm specific place, but it does feel like a useful and meaningful extension to existing code. |
Updated PR description with information about the testing that was done. |
So FEAT_SME/SME2 is defined per core, but it is implemented via a shared co-processor within silicon, the number of units depends on the hardware and this smcu estimate is trying to move away from a maintained list of machines and values to something that is a bit less maintance. I tested this code on M4, M4 Pro, M5, M5 Pro, M5 Max and Macbook Neo. All of which returned the expected number of SMCU's (1,2,1,3,3,1) respectively. |
This change doesn't impact the scoring mechanism it only changes how we construct the data type from which we get the runtime information to generate a score but the behavior and gating are the same. |
|
Hi @taronaeo I left comments yesterday evening addressing the AI reviewer comments. Thanks for taking time to look at the PR. |
Yep I saw it, thanks for responding to the bot comments. I don't know this area too well so I'll be leaving it to @chaxu01 and you. Feel free to ping me for a general review and/or approval once the PR is finalized :) |
|
@ggerganov could you please approve the workflow runs? Thanks. |
|
Merged PR1 and PR2 in my local copy and built it on an ARMv9 Orion O6. It works alright. I don't see any noticeable changes or issues. Complied with |
| // SPDX-FileCopyrightText: Copyright 2026 Arm Limited and/or its affiliates <open-source-office@arm.com> | ||
| // SPDX-License-Identifier: MIT | ||
| // |
| #include <assert.h> | ||
| #include <stdio.h> | ||
| #include <stdlib.h> |
There was a problem hiding this comment.
| #include <assert.h> | |
| #include <stdio.h> | |
| #include <stdlib.h> | |
| #include <cassert> | |
| #include <cstdio> | |
| #include <cstdlib> |
There was a problem hiding this comment.
Applied the changes suggested
|
Looks like there is a conflict. Can you fix that? |
Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com>
Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com>
Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com>
Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com>
Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com>
7d83248 to
0e71b59
Compare
Hi @taronaeo, I've rebased and resolved conflicts (moved some new additions in cpu-feats, to the centralized file) |
ggerganov
left a comment
There was a problem hiding this comment.
A couple of naming nits for consistency.
|
|
||
| #endif | ||
|
|
||
| typedef struct ggml_aarch64_runtime_features { |
There was a problem hiding this comment.
| typedef struct ggml_aarch64_runtime_features { | |
| typedef struct ggml_feats_arch64_runtime { |
There was a problem hiding this comment.
Updated every usage of this as suggested
| int sve_cnt; | ||
| } ggml_aarch64_runtime_features_t; | ||
|
|
||
| static inline ggml_aarch64_runtime_features_t ggml_get_aarch64_runtime_features(void) { |
There was a problem hiding this comment.
| static inline ggml_aarch64_runtime_features_t ggml_get_aarch64_runtime_features(void) { | |
| static inline ggml_feats_arch64_runtime_t ggml_feats_get_arch64_runtime(void) { |
There was a problem hiding this comment.
Agreed, I've renamed as suggested.
Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com>
|
Thanks for all the review comments @taronaeo @ggerganov much appreciated! |
|
Merging in a few hours if there are no further changes. |
Thanks @taronaeo |
Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> * origin/master: (383 commits) cmake : introduce semantic versioning (ggml-org#26839) gguf : harden loader against malformed tensor dims and metadata types (ggml-org#25596) kleidiai: Add runtime feature detection mechanism for aarch64/kleidiai (ggml-org#26076) model : disallow integer dflash sliding_window_pattern (ggml-org#26900) sync : ggml cmake : add config version support (ggml/1582) server : support slot save/restore with media inputs (ggml-org#26640) ui: add read_media tool (ggml-org#25877) opencl: default FA c8 cluster width to 16 on X1E (ggml-org#26433) tests : update speculative params (ggml-org#26925) vulkan: add TQ2_0 (ternary) support (ggml-org#25850) wavtokenizer-dec : bound posnet/convnext block_count against n_layer_all (ggml-org#26892) convert : handle per_layer_config in Gemma4 (transformers 5.15) (ggml-org#26882) opencl: use flat mv q5_k when weight exceeds image1d_buffer_t limit (ggml-org#26880) chat : fix muse-glimmer detection of tool calls after EOM (ggml-org#26879) ci : add missing release check (ggml-org#26923) CUDA: only disable CUDA graphs when mul_mat_id actually needs a stream sync (ggml-org#26802) cuda : add warp-per-row wkv7 kernel for single-token decode (ggml-org#26111) spec : update speculative-simple (ggml-org#26904) chat : tighten bare function parsing for Qwen models (ggml-org#26793) ...
ggml-org#26076) * Add runtime feature detection mechanism for aarch64/kleidiai Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com> * Address Review Comments Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com> * Add log warning for NSMC reserved value Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com> * Address review comments Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com> * Fix Rebase, move code from cpu-feats to ggml-feats Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com> * Address naming of runtime feature struct Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com> --------- Signed-off-by: Jonathan Clohessy <Jonathan.Clohessy@arm.com>
Overview
This PR improves upon a runtime feature detection mechanism located in ggml/src/ggml-cpu/arch/arm/cpu-feats.cpp
This mechanism has been centralized into a new header file which can be used by aarch64 systems to determine which processor features are available at runtime.
ggml/src/ggml-cpu/kleidiai/kleidiai.cpp has been reworked to use this feature detection as part of its kernel selection.
Additionally improvements have been made to SME unit detection.
Additional information
This pr is intended to be 1/3 in a series of pr's to improve the KleidiAI with its cpu feature detection, build system and documentation.
PR 2 - #26077
PR 3 - #26078
Requirements
Testing
I've tested this series of patches combined on several devices to ensure there are no performance regressions tested matched parent/patch builds across the following devices:
For the SME-capable devices:
Apple M4 Pro: the apple_m4 backend was selected with a score of 83.
SME2 KleidiAI kernels were selected for Q4, Q8 and F32, and the runtime SME thread cap was detected as 2.
OPPO Find X9 Pro: the ARMv9.2 SME2 backend was selected with a score of 127.
SME2 KleidiAI kernels were selected for Q4, Q8 and F32, and the runtime SME thread cap was detected as 1.
I also validated the unsupported-feature paths:
Graviton 3: selected ARMv8.6/SVE; SVE2 and SME variants scored zero and were rejected.
Graviton 4: selected ARMv8.6/SVE2; SME variants scored zero and were rejected.
Pixel 9 Pro: selected ARMv9.0/I8MM; both SME variants scored zero and were rejected.
I ran CPU-only performance comparisons using matched Release builds and Qwen3 0.6B Q4_0 and Q8_0 models. Representative patch deltas were:
M4 Pro: approximately +0.8% to +1.6%, with Q8 generation noisy but positive
Graviton 3: approximately -1.17% to +0.02%
Graviton 4: approximately -0.02% to +1.62%
OPPO Find X9 Pro: approximately -0.8% to +6.2%
Pixel 9 Pro: approximately -4.4% to +0.5%
The Android results were particularly sensitive to thermal throttling and run order. After mixing the runs the runs and repeating them after cooldown periods, I could not reproduce a performance change attributable to the new detection or thread-cap logic. Overall, the results were within device/run noise, and no reproducible regression was found.
One limitation is that the Linux systems available for this testing do not expose SME, so they validate the Linux unsupported-feature and rejection paths rather than the positive Linux NSMC path. The positive SME-capacity results currently come from the M4 Pro and OPPO Find X9 Pro.
I tested seperately also, other Devices like M5 Series and Neo Macbooks to for SMCU detection updates, ensuring that the values reported are those we know to be correct based on testing.