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.

Sign up for GitHub

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

ARROW-17511: [C++] Add support for xsimd 9.0.0 #13958

Merged
merged 4 commits into from
Aug 25, 2022
Merged

ARROW-17511: [C++] Add support for xsimd 9.0.0 #13958

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3cb212
ARROW-17511: [C++] Add support for xsimd 9.0.0
kou Aug 24, 2022
805cfae
Add FORCE_ANY_NEWER_VERSION
kou Aug 25, 2022
63e745b
Simplify
kou Aug 25, 2022
a81feaa
Add missing ARG_
kou Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 36 additions & 10 deletions cpp/cmake_modules/ThirdpartyToolchain.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ endmacro()
macro(resolve_dependency DEPENDENCY_NAME)
set(options)
set(one_value_args HAVE_ALT IS_RUNTIME_DEPENDENCY REQUIRED_VERSION USE_CONFIG)
set(multi_value_args COMPONENTS PC_PACKAGE_NAMES)
set(multi_value_args COMPONENTS PC_PACKAGE_NAMES REQUIRED_VERSIONS)
cmake_parse_arguments(ARG
"${options}"
"${one_value_args}"
Expand All @@ -246,18 +246,30 @@ macro(resolve_dependency DEPENDENCY_NAME)
else()
set(PACKAGE_NAME ${DEPENDENCY_NAME})
endif()
set(FIND_PACKAGE_ARGUMENTS ${PACKAGE_NAME})
if(ARG_REQUIRED_VERSION)
list(APPEND FIND_PACKAGE_ARGUMENTS ${ARG_REQUIRED_VERSION})
endif()
set(FIND_PACKAGE_ARGUMENTS_BEFORE_VERSION ${PACKAGE_NAME})
set(FIND_PACKAGE_ARGUMENTS_AFTER_VERSION)
if(ARG_USE_CONFIG)
list(APPEND FIND_PACKAGE_ARGUMENTS CONFIG)
list(APPEND FIND_PACKAGE_ARGUMENTS_AFTER_VERSION CONFIG)
endif()
if(ARG_COMPONENTS)
list(APPEND FIND_PACKAGE_ARGUMENTS COMPONENTS ${ARG_COMPONENTS})
list(APPEND FIND_PACKAGE_ARGUMENTS_AFTER_VERSION COMPONENTS ${ARG_COMPONENTS})
endif()
if(ARG_REQUIRED_VERSION)
list(APPEND ARG_REQUIRED_VERSIONS ${ARG_REQUIRED_VERSION})
endif()
if(${DEPENDENCY_NAME}_SOURCE STREQUAL "AUTO")
find_package(${FIND_PACKAGE_ARGUMENTS})
if(ARG_REQUIRED_VERSIONS)
foreach(VERSION ${ARG_REQUIRED_VERSIONS})
find_package(${FIND_PACKAGE_ARGUMENTS_BEFORE_VERSION} ${VERSION}
${FIND_PACKAGE_ARGUMENTS_AFTER_VERSION})
if(${${PACKAGE_NAME}_FOUND})
break()
endif()
endforeach()
else()
find_package(${FIND_PACKAGE_ARGUMENTS_BEFORE_VERSION}
${FIND_PACKAGE_ARGUMENTS_AFTER_VERSION})
endif()
if(${${PACKAGE_NAME}_FOUND})
set(${DEPENDENCY_NAME}_SOURCE "SYSTEM")
else()
Expand All @@ -267,7 +279,21 @@ macro(resolve_dependency DEPENDENCY_NAME)
elseif(${DEPENDENCY_NAME}_SOURCE STREQUAL "BUNDLED")
build_dependency(${DEPENDENCY_NAME})
elseif(${DEPENDENCY_NAME}_SOURCE STREQUAL "SYSTEM")
find_package(${FIND_PACKAGE_ARGUMENTS} REQUIRED)
if(ARG_REQUIRED_VERSIONS)
foreach(VERSION ${ARG_REQUIRED_VERSIONS})
find_package(${FIND_PACKAGE_ARGUMENTS_BEFORE_VERSION} ${VERSION}
${FIND_PACKAGE_ARGUMENTS_AFTER_VERSION})
if(${${PACKAGE_NAME}_FOUND})
break()
endif()
endforeach()
if(NOT ${${PACKAGE_NAME}_FOUND})
message(FATAL_ERROR "Couldn't find ${DEPENDENCY_NAME}")
endif()
else()
find_package(${FIND_PACKAGE_ARGUMENTS_BEFORE_VERSION}
${FIND_PACKAGE_ARGUMENTS_AFTER_VERSION} REQUIRED)
endif()
endif()
if(${DEPENDENCY_NAME}_SOURCE STREQUAL "SYSTEM" AND ARG_IS_RUNTIME_DEPENDENCY)
provide_find_module(${PACKAGE_NAME})
Expand Down Expand Up @@ -2253,7 +2279,7 @@ else()
endif()

if(ARROW_USE_XSIMD)
resolve_dependency(xsimd REQUIRED_VERSION "8.1.0")
resolve_dependency(xsimd REQUIRED_VERSIONS "9.0.0" "8.1.0")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to maintain this list manually in the future? What do you think @cyb70289 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure of the problem but looks inconvenient to maintain the list.
Can we only support the latest version?
Is it necessary to also update xsimd version in thirdparty/version.txt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or can we support by default all versions above 8.1.0? If xsimd breaks compatibility, we can perhaps revisit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll implement 8.1.0 or later condition.

FYI: I explain this case more:


if(xsimd_SOURCE STREQUAL "BUNDLED")
add_library(xsimd INTERFACE IMPORTED)
Expand Down