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.

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

Add explicit linking to atomic library when required #1

Merged
merged 8 commits into from
Nov 7, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
6 changes: 6 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ if(PTL_USE_TBB)
find_package(TBB 2017 REQUIRED)
endif()

# Check if compiler provides std::atomic as a library
include(CheckAtomic)
if(NOT HAVE_CXX_ATOMICS_WITHOUT_LIB OR NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB)
set(PTL_LINK_ATOMIC TRUE)
endif()

# -------------------------------------------------------------------------------------- #
# PTL Primary Build
add_subdirectory(source)
Expand Down
95 changes: 95 additions & 0 deletions cmake/Modules/CheckAtomic.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# SPDX-FileCopyrightText: 2003-2018 University of Illinois at Urbana-Champaign.
#
# SPDX-License-Identifier: BSD-3-Clause

#[=======================================================================[.rst:
CheckAtomic
-----------

Check if the compiler supports std:atomic out of the box or if libatomic is
needed for atomic support. If it is needed libatomicis added to
``CMAKE_REQUIRED_LIBRARIES``. So after running CheckAtomic you can use
std:atomic.

Since 5.75.0.
#]=======================================================================]

include(CheckCXXSourceCompiles)
include(CheckLibraryExists)
csparker247 marked this conversation as resolved.
Show resolved Hide resolved

# Sometimes linking against libatomic is required for atomic ops, if
# the platform doesn't support lock-free atomics.

function(check_working_cxx_atomics varname)
set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -std=c++11")
check_cxx_source_compiles("
#include <atomic>
std::atomic<int> x;
std::atomic<short> y;
std::atomic<char> z;
int main() {
++z;
++y;
return ++x;
}
" ${varname})
set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
endfunction()

function(check_working_cxx_atomics64 varname)
set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
set(CMAKE_REQUIRED_FLAGS "-std=c++11 ${CMAKE_REQUIRED_FLAGS}")
check_cxx_source_compiles("
#include <atomic>
#include <cstdint>
std::atomic<uint64_t> x (0);
int main() {
uint64_t i = x.load(std::memory_order_relaxed);
return 0;
}
" ${varname})
set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
endfunction()

# Check for (non-64-bit) atomic operations.
if(MSVC OR APPLE) # apple do not need latomic link
set(HAVE_CXX_ATOMICS_WITHOUT_LIB True)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we just skip directly to setting this variable for MSVC and APPLE? Isn't the point of this test to actually check if the code snips above compile?

Copy link
Author

@csparker247 csparker247 Nov 3, 2023

Choose a reason for hiding this comment

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

That's a good question. It was part of the original commits, and I can at least confirm that removing APPLE there breaks my build. But really the compiler check should handle that, I would think. I'll look closer at the rest of the module logic.

Copy link
Author

@csparker247 csparker247 Nov 6, 2023

Choose a reason for hiding this comment

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

I need to lint my latest changes, but I've refactored CheckAtomic in a way that should be more generic across platforms.

elseif(LLVM_COMPILER_IS_GCC_COMPATIBLE)
# First check if atomics work without the library.
check_working_cxx_atomics(HAVE_CXX_ATOMICS_WITHOUT_LIB)
# If not, check if the library exists, and atomics work with it.
if(NOT HAVE_CXX_ATOMICS_WITHOUT_LIB)
check_library_exists(atomic __atomic_fetch_add_4 "" HAVE_LIBATOMIC)
if(HAVE_LIBATOMIC)
list(APPEND CMAKE_REQUIRED_LIBRARIES "atomic")
check_working_cxx_atomics(HAVE_CXX_ATOMICS_WITH_LIB)
if (NOT HAVE_CXX_ATOMICS_WITH_LIB)
message(FATAL_ERROR "Host compiler must support std::atomic!")
endif()
else()
message(FATAL_ERROR "Host compiler appears to require libatomic, but cannot find it.")
endif()
endif()
endif()

# Check for 64 bit atomic operations.
if(MSVC OR APPLE) # apple do not need latomic link
set(HAVE_CXX_ATOMICS64_WITHOUT_LIB True)
elseif(LLVM_COMPILER_IS_GCC_COMPATIBLE)
# First check if atomics work without the library.
check_working_cxx_atomics64(HAVE_CXX_ATOMICS64_WITHOUT_LIB)
# If not, check if the library exists, and atomics work with it.
if(NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB)
check_library_exists(atomic __atomic_load_8 "" HAVE_CXX_LIBATOMICS64)
if(HAVE_CXX_LIBATOMICS64)
list(APPEND CMAKE_REQUIRED_LIBRARIES "atomic")
check_working_cxx_atomics64(HAVE_CXX_ATOMICS64_WITH_LIB)
if (NOT HAVE_CXX_ATOMICS64_WITH_LIB)
message(FATAL_ERROR "Host compiler must support 64-bit std::atomic!")
endif()
else()
message(FATAL_ERROR "Host compiler appears to require libatomic for 64-bit operations, but cannot find it.")
endif()
endif()
endif()
9 changes: 9 additions & 0 deletions source/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ if(BUILD_OBJECT_LIBS)
if(PTL_USE_TBB)
target_link_libraries(ptl-object PUBLIC TBB::tbb)
endif()
if(PTL_LINK_ATOMIC)
target_link_libraries(ptl-object PUBLIC atomic)
endif()

target_include_directories(
ptl-object PUBLIC $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/source>
Expand All @@ -55,6 +58,9 @@ if(BUILD_SHARED_LIBS)
if(PTL_USE_TBB)
target_link_libraries(ptl-shared PUBLIC TBB::tbb)
endif()
if(PTL_LINK_ATOMIC)
target_link_libraries(ptl-shared PUBLIC atomic)
endif()

target_compile_definitions(ptl-shared PUBLIC PTL_BUILD_DLL)

Expand Down Expand Up @@ -88,6 +94,9 @@ if(BUILD_STATIC_LIBS)
if(PTL_USE_TBB)
target_link_libraries(ptl-static PUBLIC TBB::tbb)
endif()
if(PTL_LINK_ATOMIC)
target_link_libraries(ptl-static PUBLIC atomic)
endif()

target_include_directories(
ptl-static
Expand Down
Loading