Add option to enable "sve" optimization level on armv9 #830
Conversation
Signed-off-by: Lizhen You <[email protected]>
cpp/cmake/modules/FindSVE.cmake
Outdated
|
|
||
| # Check for SVE support | ||
| message("Checking for SVE support") | ||
| SET(CMAKE_REQUIRED_FLAGS "-march=armv9-a+sve") |
There was a problem hiding this comment.
Please use lowercase for the CMake function names, like set(...), if(...), else(), mark_as_advanced(...).
There was a problem hiding this comment.
I just found the FindAVX.cmake is using the uppercase as well; So it is reasonable to use uppercase for FindSVE.cmake for code consistency?
in FindAVX.cmake:
ACRO(CHECK_SSE lang type flags)
SET(__FLAG_I 1)
SET(CMAKE_REQUIRED_FLAGS_SAVE ${CMAKE_REQUIRED_FLAGS})
FOREACH(__FLAG ${flags})
IF(NOT ${lang}${type}FOUND)
SET(CMAKE_REQUIRED_FLAGS ${__FLAG})
CHECK_CXX_SOURCE_RUNS("${${type}CODE}" ${lang}HAS${type}${__FLAG_I})
IF(${lang}HAS${type}${__FLAG_I})
SET(${lang}${type}FOUND
TRUE
CACHE BOOL "${lang} ${type} support"
)
SET(${lang}${type}_FLAGS
"${__FLAG}"
CACHE STRING "${lang} ${type} flags"
)
ENDIF()
MATH(EXPR __FLAG_I "${__FLAG_I}+1")
ENDIF()
ENDFOREACH()
There was a problem hiding this comment.
Let's prefer "good CMake style" over consistency here, and go lower-case. Maybe as a follow-up we can fix FindAVX.cmake.
There was a problem hiding this comment.
Does the latest code look good to you? The "sve" optimization flag can enhance the performance on AArch64 machine significantly which is around 17% in my run. Hope we can integrate this flag in soon time.
There was a problem hiding this comment.
@LizYou please see my comment below about the style checker (git pre-commit hooks). This PR cannot be merged while the style checker is not passing.
There was a problem hiding this comment.
Sorry for overlooking this comment. I have fixed the style warning with pre-commit run. Pls double-check; @cjnolet
$ pre-commit run --files cpp/CMakeLists.txt cpp/cmake/modules/FindSVE.cmake cpp/cmake/thirdparty/get_faiss.cmake
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
isort................................................(no files to check)Skipped
black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
mypy.................................................(no files to check)Skipped
pydocstyle...........................................(no files to check)Skipped
clang-format.........................................(no files to check)Skipped
no-deprecationwarning................................(no files to check)Skipped
cmake-format.............................................................Passed
- hook id: cmake-format
- duration: 0s
The rapids-cmake cmake-format configuration file was not found at any of the default search locations:
/home/scratch.lyou_gpu/arm/workspaces/cuvs_faiss_sve/cpp/build/_deps/rapids-cmake-src/cmake-format-rapids-cmake.json
Try setting the environment variable RAPIDS_CMAKE_FORMAT_FILE to the path to the config file.
cmake-lint...............................................................Passed
- hook id: cmake-lint
- duration: 0s
The rapids-cmake cmake-format configuration file was not found at any of the default search locations:
/home/scratch.lyou_gpu/arm/workspaces/cuvs_faiss_sve/cpp/build/_deps/rapids-cmake-src/cmake-format-rapids-cmake.json
Try setting the environment variable RAPIDS_CMAKE_FORMAT_FILE to the path to the config file.
include-check............................................................Passed
codespell................................................................Passed
check json...........................................(no files to check)Skipped
verify-copyright.....................................(no files to check)Skipped
verify-alpha-spec....................................(no files to check)Skipped
verify-codeowners....................................(no files to check)Skipped
RAPIDS dependency file generator.....................(no files to check)Skipped
shellcheck...........................................(no files to check)Skipped
cjnolet
added
improvement
|
/ok to test 0d939b1 |
|
@LizYou it looks like we have some style checker errors. We use Git pre-commit hooks to automatically format code changes so they pass the style checker upon commit. Please see the following section of out contributor guide for information on how to configure pre-commit hooks. |
|
/ok to test f4d80ce |
Signed-off-by: Lizhen You <[email protected]>
|
/ok to test ca55b77 |
|
/ok to test 70e7dad |
|
/ok to test 1f3618e |
1 similar comment
|
/ok to test 1f3618e |
cpp/CMakeLists.txt
Outdated
| @@ -18,6 +18,7 @@ include(rapids-export) | |||
| include(rapids-find) | |||
|
|
|||
| option(BUILD_CPU_ONLY "Build CPU only components. Applies to CUVS benchmarks currently" OFF) | |||
| option(CUVS_CPU_ONLY_FAISS_ENABLE_SVE "Enable SVE support for CPU ONLY FAISS" OFF) | |||
There was a problem hiding this comment.
This option is misplaced- we don't build FAISS in the top-level. I think this would be better in get_faiss.cmake
There was a problem hiding this comment.
This option is misplaced- we don't build FAISS in the top-level. I think this would be better in
get_faiss.cmake
Fixed. Pls double check in the PR.
Signed-off-by: Lizhen You <[email protected]>
Signed-off-by: Lizhen You <[email protected]>
|
/ok to test ac53eb1 |
|
/ok to test f52e8e4 |
No description provided.