Skip to content

[libc] Remove special full build handling for GPU #128572

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 1 commit into from
Feb 24, 2025
Merged

[libc] Remove special full build handling for GPU #128572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion libc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,13 @@ else()
endif()
endif()

option(LLVM_LIBC_FULL_BUILD "Build and test LLVM libc as if it is the full libc" OFF)
# Some targets can only support the full build.
set(default_to_full_build OFF)
if(LIBC_TARGET_OS_IS_GPU)
set(default_to_full_build ON)
endif()

option(LLVM_LIBC_FULL_BUILD "Build and test LLVM libc as if it is the full libc" ${default_to_full_build})
Comment on lines +131 to +137
Copy link
Contributor

@lntue lntue Feb 24, 2025

Choose a reason for hiding this comment

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

hmm, why don't you just overwrite the LLVM_LIBC_FULL_BUILD directly after defining option?

Copy link
Contributor Author

@jhuber6 jhuber6 Feb 24, 2025

Choose a reason for hiding this comment

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

Couldn't decide if that was better since this allows the user the ability to manually override it and see a nice error that it doesn't work.

Copy link
Contributor

@lntue lntue Feb 24, 2025

Choose a reason for hiding this comment

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

I'd prefer to just message that default to full build mode with LIBC_TARGET_OS_IS_GPU and then overwrite it, so that we don't have to introduce extra variable.

Copy link
Contributor Author

@jhuber6 jhuber6 Feb 24, 2025

Choose a reason for hiding this comment

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

I could just make it ${LIBC_OS_IS_GPU} for now, that's a one liner.

Copy link
Contributor

@lntue lntue Feb 24, 2025

Choose a reason for hiding this comment

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

then it will not be so clean for other targets that want to default to full build also. Something like:

option(LLVM_LIBC_FULL_BUILD "Build and test LLVM libc as if it is the full libc" OFF)
if(LIBC_TARGET_OS_IS_GPU)
  message(STATUS "Default to full build mode.")
  set(LLVM_LIBC_FULL_BUILD ON)
endif()

should be clearer to users.

Copy link
Contributor Author

@jhuber6 jhuber6 Feb 24, 2025

Choose a reason for hiding this comment

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

I would've preferred allowing the user to override the default behavior, but I guess I could make this change and then also remove the static check elsewhere.

Copy link
Contributor Author

@jhuber6 jhuber6 Feb 24, 2025

Choose a reason for hiding this comment

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

Also because resetting a cache variable is a pain because you need to force it. Whereas this is just an extra normal variable. Do I really need to do it this way?

Copy link
Contributor

@lntue lntue Feb 24, 2025

Choose a reason for hiding this comment

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

nah, that's ok.

option(LLVM_LIBC_IMPLEMENTATION_DEFINED_TEST_BEHAVIOR "Build LLVM libc tests assuming our implementation-defined behavior" ON)
option(LLVM_LIBC_ENABLE_LINTING "Enables linting of libc source files" OFF)

Expand Down
2 changes: 0 additions & 2 deletions llvm/runtimes/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,15 +535,13 @@ if(build_runtimes)
"-DRUNTIMES_amdgcn-amd-amdhsa_LIBC_GPU_LOADER_EXECUTABLE=$<TARGET_FILE:amdhsa-loader>")
list(APPEND extra_deps amdhsa-loader)
endif()
list(APPEND extra_cmake_args "-DRUNTIMES_amdgcn-amd-amdhsa_LLVM_LIBC_FULL_BUILD=ON")
endif()
if("libc" IN_LIST RUNTIMES_nvptx64-nvidia-cuda_LLVM_ENABLE_RUNTIMES)
if(TARGET nvptx-loader)
list(APPEND extra_cmake_args
"-DRUNTIMES_nvptx64-nvidia-cuda_LIBC_GPU_LOADER_EXECUTABLE=$<TARGET_FILE:nvptx-loader>")
list(APPEND extra_deps nvptx-loader)
endif()
list(APPEND extra_cmake_args "-DRUNTIMES_nvptx64-nvidia-cuda_LLVM_LIBC_FULL_BUILD=ON")
endif()
if(TARGET clang-offload-packager)
list(APPEND extra_deps clang-offload-packager)
Expand Down
Loading