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

[libc][cmake] move _get_{common|hermetic}_test_compile_options to LLVMLibCTestRules.cmake #122549

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nickdesaulniers
Copy link
Member

@nickdesaulniers nickdesaulniers commented Jan 10, 2025

They're only used in that file, which is more appropriate than
LLVMLibCCompileOptionRules.cmake since they're strictly related to tests.

Remove unused flags param from _get_hermetic_test_compile_options,
and flags that were already set by a prior call to
_get_common_test_compile_options.

…Rules.cmake

It's only used in that file, which is more appropriate than
LLVMLibCCompileOptionRules.cmake since it's strictly related to tests.
@llvmbot
Copy link
Member

llvmbot commented Jan 10, 2025

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

It's only used in that file, which is more appropriate than
LLVMLibCCompileOptionRules.cmake since it's strictly related to tests.


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

2 Files Affected:

  • (modified) libc/cmake/modules/LLVMLibCCompileOptionRules.cmake (-23)
  • (modified) libc/cmake/modules/LLVMLibCTestRules.cmake (+25)
diff --git a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
index 8dcee1ec422464..785ea049448520 100644
--- a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
+++ b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
@@ -272,26 +272,3 @@ function(_get_common_test_compile_options output_var c_test flags)
   endif()
   set(${output_var} ${compile_options} PARENT_SCOPE)
 endfunction()
-
-function(_get_hermetic_test_compile_options output_var flags)
-  _get_common_test_compile_options(compile_options "" "${flags}")
-
-  list(APPEND compile_options "-fpie")
-  list(APPEND compile_options "-ffreestanding")
-  list(APPEND compile_options "-fno-exceptions")
-  list(APPEND compile_options "-fno-rtti")
-
-  # The GPU build requires overriding the default CMake triple and architecture.
-  if(LIBC_TARGET_ARCHITECTURE_IS_AMDGPU)
-    list(APPEND compile_options
-         -Wno-multi-gpu -nogpulib -mcpu=${LIBC_GPU_TARGET_ARCHITECTURE} -flto
-         -mcode-object-version=${LIBC_GPU_CODE_OBJECT_VERSION})
-  elseif(LIBC_TARGET_ARCHITECTURE_IS_NVPTX)
-    list(APPEND compile_options
-         "SHELL:-mllvm -nvptx-emit-init-fini-kernel=false"
-         -Wno-multi-gpu --cuda-path=${LIBC_CUDA_ROOT}
-         -nogpulib -march=${LIBC_GPU_TARGET_ARCHITECTURE} -fno-use-cxa-atexit)
-  endif()
-
-  set(${output_var} ${compile_options} PARENT_SCOPE)
-endfunction()
diff --git a/libc/cmake/modules/LLVMLibCTestRules.cmake b/libc/cmake/modules/LLVMLibCTestRules.cmake
index 84f3125d557eae..fa9c83440a376f 100644
--- a/libc/cmake/modules/LLVMLibCTestRules.cmake
+++ b/libc/cmake/modules/LLVMLibCTestRules.cmake
@@ -359,6 +359,31 @@ if(NOT MSVC AND NOT LIBC_CC_SUPPORTS_NOSTDLIBPP)
   string(STRIP ${LIBGCC_S_LOCATION} LIBGCC_S_LOCATION)
 endif()
 
+function(_get_hermetic_test_compile_options output_var flags)
+  _get_common_test_compile_options(compile_options "" "${flags}")
+
+  list(APPEND compile_options "-fpie")
+  list(APPEND compile_options "-ffreestanding")
+  list(APPEND compile_options "-fno-exceptions")
+  list(APPEND compile_options "-fno-rtti")
+
+  # The GPU build requires overriding the default CMake triple and architecture.
+  if(LIBC_TARGET_ARCHITECTURE_IS_AMDGPU)
+    list(APPEND compile_options
+         -Wno-multi-gpu -nogpulib -mcpu=${LIBC_GPU_TARGET_ARCHITECTURE} -flto
+         -mcode-object-version=${LIBC_GPU_CODE_OBJECT_VERSION})
+  elseif(LIBC_TARGET_ARCHITECTURE_IS_NVPTX)
+    list(APPEND compile_options
+         "SHELL:-mllvm -nvptx-emit-init-fini-kernel=false"
+         -Wno-multi-gpu --cuda-path=${LIBC_CUDA_ROOT}
+         -nogpulib -march=${LIBC_GPU_TARGET_ARCHITECTURE} -fno-use-cxa-atexit)
+  # elseif(explicit_target_triple AND NOT CMAKE_COMPILER_IS_GNUCXX)
+  #   list(APPEND compile_options "--target=${explicit_target_triple}")
+  endif()
+
+  set(${output_var} ${compile_options} PARENT_SCOPE)
+endfunction()
+
 # DEPRECATED: Use add_hermetic_test instead.
 #
 # Rule to add an integration test. An integration test is like a unit test

Comment on lines 362 to 363
function(_get_hermetic_test_compile_options output_var flags)
_get_common_test_compile_options(compile_options "" "${flags}")
Copy link
Member Author

Choose a reason for hiding this comment

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

The three callsites don't pass flags. Maybe I can clean that up, too.

@nickdesaulniers
Copy link
Member Author

I should move _get_common_test_compile_options, too for all of the same reasons.

@nickdesaulniers nickdesaulniers changed the title [libc][cmake] move _get_hermetic_test_compile_options to LLVMLibCTestRules.cmake [libc][cmake] move _get_{common|hermetic}_test_compile_options to LLVMLibCTestRules.cmake Jan 10, 2025
Comment on lines 63 to 66
list(APPEND compile_options "-fpie")
list(APPEND compile_options "-ffreestanding")
list(APPEND compile_options "-fno-exceptions")
list(APPEND compile_options "-fno-rtti")
Copy link
Member Author

Choose a reason for hiding this comment

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

These are duplicated 100% from _get_common_test_compile_options which was just called. I'm going to remove them.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

3 participants