Skip to content

[libc] Fix standalone cross compiling build for the GPU #84042

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

Merged
merged 3 commits into from
Mar 6, 2024
Merged

[libc] Fix standalone cross compiling build for the GPU #84042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ed8760
[libc] Fix standalone cross compiling build for the GPU
jhuber6 Mar 5, 2024
0354794
Update libc/CMakeLists.txt
jhuber6 Mar 5, 2024
c4cefc2
Update libc/CMakeLists.txt
jhuber6 Mar 5, 2024
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
14 changes: 12 additions & 2 deletions libc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,11 @@ set(LIBC_BUILD_DIR ${CMAKE_CURRENT_BINARY_DIR})
set(LIBC_ENABLE_USE_BY_CLANG OFF CACHE BOOL "Whether or not to place libc in a build directory findable by a just built clang")

# Defining a global namespace to enclose all libc functions.
set(LIBC_NAMESPACE "__llvm_libc_${LLVM_VERSION_MAJOR}_${LLVM_VERSION_MINOR}_${LLVM_VERSION_PATCH}_${LLVM_VERSION_SUFFIX}"
set(default_namespace "__llvm_libc")
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change related to the rest of this PR? It seems like an odd inclusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because when you build it standalone the namespace is __llvm_libc_____ which is silly, though that's not specifically a GPU problem.

Copy link
Member

Choose a reason for hiding this comment

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

Why would LLVM_VERSION_MAJOR not be set? Does this happen for the runtimes build? (if so, cc @petrhosek )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happens if you build it directly out of the runtimes/ directory. We specifically build everything else independent from LLVM if possible so I don't think this is a surprising change to have a fallback if the user is building out-of-tree or manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

Copy link
Member

@nickdesaulniers nickdesaulniers Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if building via the runtimes directory SHOULD set LLVM_VERSION_MAJOR? Otherwise libcxx, compiler-rt, and now llvmlibc wont be able to key off this value.

The non LIBC_NAMESPACE changes LGTM, if you want to split this PR in two, I'd accept the second half. For the first, I suspect we may want to fix the runtimes CMake itself. (And maybe error if LLVM_VERSION_MAJOR is unset, rather than use an emptier namespace).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already want to support out-of-tree builds, I don't see the issue with setting a sane default for those cases.

Copy link
Member

@nickdesaulniers nickdesaulniers Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We already want to support out-of-tree builds

What do you mean? Folks still need to use llvm/ or runtimes/ as the base if they use cmake. If they're not using cmake, then setting a default namespace for cmake is irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this does is change __llvm_libc_____ into __llvm_libc is this a blocking issue or can I merge the patch as-is.

if(LLVM_VERSION_MAJOR)
set(default_namespace "__llvm_libc_${LLVM_VERSION_MAJOR}_${LLVM_VERSION_MINOR}_${LLVM_VERSION_PATCH}_${LLVM_VERSION_SUFFIX}")
endif()
set(LIBC_NAMESPACE ${default_namespace}
CACHE STRING "The namespace to use to enclose internal implementations. Must start with '__llvm_libc'."
)

Expand Down Expand Up @@ -232,7 +236,13 @@ else()
set(LIBC_LIBRARY_DIR ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
endif()
if(LIBC_TARGET_OS_IS_GPU)
set(LIBC_INSTALL_INCLUDE_DIR ${CMAKE_INSTALL_INCLUDEDIR}/${LLVM_DEFAULT_TARGET_TRIPLE})
if(LLVM_RUNTIMES_TARGET)
set(LIBC_INSTALL_INCLUDE_DIR ${CMAKE_INSTALL_INCLUDEDIR}/${LLVM_RUNTIMES_TARGET})
elseif(LIBC_TARGET_TRIPLE)
set(LIBC_INSTALL_INCLUDE_DIR ${CMAKE_INSTALL_INCLUDEDIR}/${LIBC_TARGET_TRIPLE})
else()
set(LIBC_INSTALL_INCLUDE_DIR ${CMAKE_INSTALL_INCLUDEDIR}/${LLVM_DEFAULT_TARGET_TRIPLE})
endif()
else()
set(LIBC_INSTALL_INCLUDE_DIR ${CMAKE_INSTALL_INCLUDEDIR})
endif()
Expand Down
14 changes: 12 additions & 2 deletions libc/cmake/modules/prepare_libc_gpu_build.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,20 @@ endif()

set(LIBC_GPU_TEST_ARCHITECTURE "" CACHE STRING "Architecture for the GPU tests")
if(LIBC_TARGET_ARCHITECTURE_IS_AMDGPU)
check_cxx_compiler_flag("-nogpulib -mcpu=native" PLATFORM_HAS_GPU)
# Identify any locally installed NVIDIA GPUs on the system using 'nvptx-arch'.
find_program(LIBC_AMDGPU_ARCH
NAMES amdgpu-arch NO_DEFAULT_PATH
PATHS ${LLVM_BINARY_DIR}/bin ${compiler_path})
if(LIBC_AMDGPU_ARCH)
execute_process(COMMAND ${LIBC_AMDGPU_ARCH}
OUTPUT_VARIABLE arch_tool_output
ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE)
if(arch_tool_output MATCHES "^gfx[0-9]+")
set(PLATFORM_HAS_GPU TRUE)
endif()
endif()
elseif(LIBC_TARGET_ARCHITECTURE_IS_NVPTX)
# Identify any locally installed NVIDIA GPUs on the system using 'nvptx-arch'.
# Using 'check_cxx_compiler_flag' does not work currently due to the link job.
find_program(LIBC_NVPTX_ARCH
NAMES nvptx-arch NO_DEFAULT_PATH
PATHS ${LLVM_BINARY_DIR}/bin ${compiler_path})
Expand Down