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.

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

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Mar 5, 2024

Summary:
This patch fixes some issues with building the GPU target manually
without the runtimes bootstrapping build. This fixes the install
directory and sets the default namespace to something more sensible if
not set. Also I got rid of the check on -mcpu=native. it was a neat
trick, but CMake in its INFINITE wisdom does not allow you to set link
flags on the compiler flag check. So I just went with the old tool
usage.

Summary:
This patch fixes some issues with building the GPU target manually
without the runtimes bootstrapping build. This fixes the install
directory and sets the default namespace to something more sensible if
not set. Also I got rid of the check on `-mcpu=native`. it was a neat
trick, but CMake in its INFINITE wisdom does not allow you to set link
flags on the compiler flag check. So I just went with the old tool
usage.
@llvmbot
Copy link
Member

llvmbot commented Mar 5, 2024

@llvm/pr-subscribers-libc

Author: Joseph Huber (jhuber6)

Changes

Summary:
This patch fixes some issues with building the GPU target manually
without the runtimes bootstrapping build. This fixes the install
directory and sets the default namespace to something more sensible if
not set. Also I got rid of the check on -mcpu=native. it was a neat
trick, but CMake in its INFINITE wisdom does not allow you to set link
flags on the compiler flag check. So I just went with the old tool
usage.


Full diff: https://github.com/llvm/llvm-project/pull/84042.diff

2 Files Affected:

  • (modified) libc/CMakeLists.txt (+13-3)
  • (modified) libc/cmake/modules/prepare_libc_gpu_build.cmake (+12-2)
diff --git a/libc/CMakeLists.txt b/libc/CMakeLists.txt
index 0b72b1c5481651..dd2313e6e8e033 100644
--- a/libc/CMakeLists.txt
+++ b/libc/CMakeLists.txt
@@ -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_")
+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'."
 )
 
@@ -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()
@@ -281,7 +291,7 @@ if(LLVM_LIBC_ENABLE_LINTING)
           result : '${CLANG_TIDY_RESULT}'
           ")
       endif()
-      string(REGEX MATCH "[0-9]+" CLANG_TIDY_VERSION "${CLANG_TIDY_OUTPUT}")
+      string(REGEX MATCH "+" CLANG_TIDY_VERSION "${CLANG_TIDY_OUTPUT}")
       string(REGEX MATCH "[0-9]+" CLANG_MAJOR_VERSION
         "${CMAKE_CXX_COMPILER_VERSION}")
 
diff --git a/libc/cmake/modules/prepare_libc_gpu_build.cmake b/libc/cmake/modules/prepare_libc_gpu_build.cmake
index 7e9fc746ea91d9..2de4cb8d82b28b 100644
--- a/libc/cmake/modules/prepare_libc_gpu_build.cmake
+++ b/libc/cmake/modules/prepare_libc_gpu_build.cmake
@@ -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})

@@ -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

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

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.

@@ -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.

fair enough

@jhuber6 jhuber6 merged commit a30233f into llvm:main Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants