Move KleidiAI support detection to CMake logic.#26178
Conversation
snnn
left a comment
There was a problem hiding this comment.
Hello! I've reviewed the changes, and this is a great refactoring that simplifies the build logic. I have a few technical suggestions for improvement.
1. Potential bug in KleidiAI platform detection for Apple
In cmake/CMakeLists.txt, the platform check for Apple is:
if(NOT (... OR (APPLE AND CMAKE_OSX_ARCHITECTURES STREQUAL "arm64")))This check depends on CMAKE_OSX_ARCHITECTURES being defined. According to the logic in the new cmake/detect_onnxruntime_target_platform.cmake file, if CMAKE_OSX_ARCHITECTURES is not defined during a build on an Apple machine, onnxruntime_target_platform is set from CMAKE_SYSTEM_PROCESSOR. For a native build on Apple Silicon, this means onnxruntime_target_platform would be arm64, but CMAKE_OSX_ARCHITECTURES would be undefined.
In this case, the check above would evaluate to false, and KleidiAI would be incorrectly disabled.
A more robust condition would be to check onnxruntime_target_platform directly:
if(NOT (... OR (APPLE AND onnxruntime_target_platform STREQUAL "arm64")))This would correctly enable KleidiAI for native Apple Silicon builds, regardless of whether CMAKE_OSX_ARCHITECTURES is explicitly set.
2. Potential issues in cmake/adjust_global_compile_flags.cmake
I noticed two small things in this file:
-
The
if/elseifchain appears to have a syntax error based on the indentation in the diff. It looks like anifstatement was added after anelseif, but before the finalelse, which is not valid CMake syntax. The logic for enablingSAFESEHfor x86 builds should likely be nested inside theelseifblock that checks forx64orx86, or placed after the entireif-elseif-elseblock. -
The
FATAL_ERRORmessage at the end of that block seems to have a copy-paste error:message(FATAL_ERROR "Unknown CMAKE_SYSTEM_PROCESSOR: ${CMAKE_SYSTEM_PROCESSOR}")
Since the variable being checked throughout this block is
onnxruntime_target_platform, the error message should probably refer to that variable for clearer debugging:message(FATAL_ERROR "Unknown onnxruntime_target_platform: ${onnxruntime_target_platform}")
This review comment was generated by an AI assistant.
### Description <!-- Describe your changes. --> Use of the KleidiAI library is not supported on all platforms. Previously, this support detection was done in both `tools/ci_build/build.py` and `cmake/CMakeLists.txt`. This change consolidates and simplifies the checks. They are now done in one place in the CMake logic. Move setting of onnxruntime_target_platform into a separate file. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> Clean up and simplify.
### Description <!-- Describe your changes. --> Use of the KleidiAI library is not supported on all platforms. Previously, this support detection was done in both `tools/ci_build/build.py` and `cmake/CMakeLists.txt`. This change consolidates and simplifies the checks. They are now done in one place in the CMake logic. Move setting of onnxruntime_target_platform into a separate file. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> Clean up and simplify.
Description
Use of the KleidiAI library is not supported on all platforms. Previously, this support detection was done in both
tools/ci_build/build.pyandcmake/CMakeLists.txt. This change consolidates and simplifies the checks. They are now done in one place in the CMake logic.Motivation and Context
Clean up and simplify.