Skip to content

[libc] Disable -ffreestanding and -fno-builtin on the GPU build #97636

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 1 commit into from
Jul 9, 2024

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Jul 3, 2024

Summary:
This patch removed the -ffreestanding and -fno-builtin flags from
the publically installed version of the GPU library. The presense of
-fno-builtin caused issues that prevented all inlining in the GPU C
library, see https://discourse.llvm.org/t/rfc-libc-ffreestanding-fno-builtin/75883.
Previously, I attempted to fix this by loosening the restriction that
"no-builtins" functions cannot be inlined into functions without that
attribute. However, that opened up a lot of extra issues that stalled
that approach.

This patch instead removes that and instead passes -fno-builtin-<xyz>
for the few calls that are known to be problematic. I believe this works
in general as the GPU backends do not emit any libcalls and the
implementations of most of these simply reduce to builtins right now.
This is a very useful patch as we can now actually inline calls.

@llvmbot
Copy link
Member

llvmbot commented Jul 3, 2024

@llvm/pr-subscribers-libc

Author: Joseph Huber (jhuber6)

Changes

Summary:
This patch removed the -ffreestanding and -fno-builtin flags from
the publically installed version of the GPU library. The presense of
-fno-builtin caused issues that prevented all inlining in the GPU C
library, see https://discourse.llvm.org/t/rfc-libc-ffreestanding-fno-builtin/75883.
Previously, I attempted to fix this by loosening the restriction that
"no-builtins" functions cannot be inlined into functions without that
attribute. However, that opened up a lot of extra issues that stalled
that approach.

This patch instead removes that and instead passes -fno-builtin-&lt;xyz&gt;
for the few calls that are known to be problematic. I believe this works
in general as the GPU backends do not emit any libcalls and the
implementations of most of these simply reduce to builtins right now.
This is a very useful patch as we can now actually inline calls.


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

1 Files Affected:

  • (modified) libc/cmake/modules/LLVMLibCCompileOptionRules.cmake (+23-3)
diff --git a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
index 28379213029a3..026f54f337ac1 100644
--- a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
+++ b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
@@ -43,9 +43,11 @@ function(_get_common_compile_options output_var flags)
     list(APPEND compile_options "-fpie")
 
     if(LLVM_LIBC_FULL_BUILD)
+      # Only add -ffreestanding flag in non-GPU full build mode.
+      if(LIBC_TARGET_OS_IS_GPU)
+        list(APPEND compile_options "-ffreestanding")
+      endif()
       list(APPEND compile_options "-DLIBC_FULL_BUILD")
-      # Only add -ffreestanding flag in full build mode.
-      list(APPEND compile_options "-ffreestanding")
       # Manually disable standard include paths to prevent system headers from
       # being included.
       if(LIBC_CC_SUPPORTS_NOSTDLIBINC)
@@ -60,7 +62,25 @@ function(_get_common_compile_options output_var flags)
       list(APPEND compile_options "-ffixed-point")
     endif()
 
-    list(APPEND compile_options "-fno-builtin")
+    # Builtin recognition causes issues when trying to implement the builtin
+    # functions themselves. The GPU backends do not use libcalls so we disable
+    # the known problematic ones. This allows inlining during LTO linking.
+    if(LIBC_TARGET_OS_IS_GPU)
+      list(APPEND compile_options "-fno-builtin-bcmp")
+      list(APPEND compile_options "-fno-builtin-strlen")
+      list(APPEND compile_options "-fno-builtin-memmem")
+      list(APPEND compile_options "-fno-builtin-bzero.h")
+      list(APPEND compile_options "-fno-builtin-memcmp.h")
+      list(APPEND compile_options "-fno-builtin-memcpy.h")
+      list(APPEND compile_options "-fno-builtin-memmem.h")
+      list(APPEND compile_options "-fno-builtin-memmove.h")
+      list(APPEND compile_options "-fno-builtin-memset.h")
+      list(APPEND compile_options "-fno-builtin-strcmp.h")
+      list(APPEND compile_options "-fno-builtin-strstr.h")
+    else()
+      list(APPEND compile_options "-fno-builtin")
+    endif()
+
     list(APPEND compile_options "-fno-exceptions")
     list(APPEND compile_options "-fno-lax-vector-conversions")
     list(APPEND compile_options "-fno-unwind-tables")

Summary:
This patch removed the `-ffreestanding` and `-fno-builtin` flags from
the publically installed version of the GPU library. The presense of
`-fno-builtin` caused issues that prevented all inlining in the GPU C
library, see https://discourse.llvm.org/t/rfc-libc-ffreestanding-fno-builtin/75883.
Previously, I attempted to fix this by loosening the restriction that
`"no-builtins"` functions cannot be inlined into functions without that
attribute. However, that opened up a lot of extra issues that stalled
that approach.

This patch instead removes that and instead passes `-fno-builtin-<xyz>`
for the few calls that are known to be problematic. I believe this works
in general as the GPU backends do not emit any libcalls and the
implementations of most of these simply reduce to builtins right now.
This is a very useful patch as we can now actually inline calls.
@jhuber6 jhuber6 force-pushed the GPUFreestanding branch from 3341745 to 24a4f02 Compare July 3, 2024 21:45
Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

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

LGTM from the libc side

@jhuber6 jhuber6 merged commit 221d5c5 into llvm:main Jul 9, 2024
6 checks passed
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
…lvm#97636)

Summary:
This patch removed the `-ffreestanding` and `-fno-builtin` flags from
the publically installed version of the GPU library. The presense of
`-fno-builtin` caused issues that prevented all inlining in the GPU C
library, see
https://discourse.llvm.org/t/rfc-libc-ffreestanding-fno-builtin/75883.
Previously, I attempted to fix this by loosening the restriction that
`"no-builtins"` functions cannot be inlined into functions without that
attribute. However, that opened up a lot of extra issues that stalled
that approach.

This patch instead removes that and instead passes `-fno-builtin-<xyz>`
for the few calls that are known to be problematic. I believe this works
in general as the GPU backends do not emit any libcalls and the
implementations of most of these simply reduce to builtins right now.
This is a very useful patch as we can now actually inline calls.
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.

4 participants