Skip to content

[libc] Proof of concept of aliasing long double math functions. #132627

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

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

[libc] Proof of concept of aliasing long double math functions. #132627

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d53238e
[libc] Proof of concept of aliasing long double math functions.
lntue Mar 23, 2025
1243671
Add new lines at the EOFs.
lntue Mar 23, 2025
ed922b7
Fix and create common aliasing macro.
lntue Mar 24, 2025
8e1abc1
Remove debugging message.
lntue Mar 24, 2025
268dd92
Add missing semicolons.
lntue Mar 24, 2025
f4d2856
Fix token clashing.
lntue Mar 24, 2025
0d04f79
Remove namespace block from the aliasing macro.
lntue Mar 24, 2025
e2b6c90
Change macro name to LLVM_LIBC_ALIAS.
lntue Mar 25, 2025
0be1f70
Remove extra asm statement for non public packaging.
lntue Mar 25, 2025
95f920a
Change from asm to gnu::alias attribute to also export the symbol.
lntue Mar 27, 2025
08627d4
Add build guards for copysignl.
lntue Mar 27, 2025
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
1 change: 1 addition & 0 deletions libc/cmake/modules/LLVMLibCArchitectures.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ if(LIBC_TARGET_ARCHITECTURE STREQUAL "arm")
set(LIBC_TARGET_ARCHITECTURE_IS_ARM TRUE)
elseif(LIBC_TARGET_ARCHITECTURE STREQUAL "aarch64")
set(LIBC_TARGET_ARCHITECTURE_IS_AARCH64 TRUE)
set(LIBC_TARGET_LONG_DOUBLE_IS_FLOAT128 TRUE)
elseif(LIBC_TARGET_ARCHITECTURE STREQUAL "x86_64")
set(LIBC_TARGET_ARCHITECTURE_IS_X86_64 TRUE)
elseif(LIBC_TARGET_ARCHITECTURE STREQUAL "i386")
Expand Down
4 changes: 4 additions & 0 deletions libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ function(_get_common_compile_options output_var flags)
if(LLVM_LIBC_COMPILER_IS_GCC_COMPATIBLE)
list(APPEND compile_options "-fpie")

if(LIBC_TARGET_LONG_DOUBLE_IS_FLOAT128 OR LIBC_TARGET_LONG_DOUBLE_IS_FLOAT64)
list(APPEND compile_options "-DLIBC_ALIAS_LONG_DOUBLE")
endif()

if(LLVM_LIBC_FULL_BUILD)
# Only add -ffreestanding flag in non-GPU full build mode.
if(NOT LIBC_TARGET_OS_IS_GPU)
Expand Down
44 changes: 35 additions & 9 deletions libc/src/__support/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,47 @@

#define LLVM_LIBC_ATTR(name) EXPAND_THEN_SECOND(LLVM_LIBC_FUNCTION_ATTR_##name)

// MacOS needs to be excluded because it does not support aliasing.
#if defined(LIBC_COPT_PUBLIC_PACKAGING) && (!defined(__APPLE__))
#define LLVM_LIBC_FUNCTION_IMPL(type, name, arglist) \
// MacOS needs to be excluded because it does not support [[gnu::aliasing]].
#ifndef __APPLE__

#if defined(LIBC_COPT_PUBLIC_PACKAGING)
#define LLVM_LIBC_FUNCTION(type, name, arglist) \
LLVM_LIBC_ATTR(name) \
LLVM_LIBC_FUNCTION_ATTR decltype(LIBC_NAMESPACE::name) \
__##name##_impl__ __asm__(#name); \
__##name##_impl__ asm(#name); \
decltype(LIBC_NAMESPACE::name) name [[gnu::alias(#name)]]; \
type __##name##_impl__ arglist
#else
#define LLVM_LIBC_FUNCTION_IMPL(type, name, arglist) type name arglist
#endif

// This extra layer of macro allows `name` to be a macro to rename a function.
#define LLVM_LIBC_ALIASING_FUNCTION(name, func) \
namespace LIBC_NAMESPACE_DECL { \
Copy link
Contributor

@michaelrj-google michaelrj-google Mar 24, 2025

Choose a reason for hiding this comment

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

why is this namespacing done in the macro? Why not just put the macro in the namespace in the source file?

Copy link
Contributor Author

@lntue lntue Mar 24, 2025

Choose a reason for hiding this comment

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

it's because when adding aliases for copysignl, we will need to include copysignl.h header. So to keep them in just one #if ... #endif block for ease of use, it's cleaner to add the namespace to the macro than having 1 line macro in the namespace all the time, like:

#if ...
#include "copysignl.h"

namespace LIBC_NAMESPACE_DECL {
  LLVM_LIBC_ALIASING_FUNCTION(copysignl, copysignf128);
}
#endif

vs

#if ...
#include "copysignl.h"

LLVM_LIBC_ALIASING_FUNCTION(copysignl, copysignf128);
#endif

Copy link
Contributor

@michaelrj-google michaelrj-google Mar 24, 2025

Choose a reason for hiding this comment

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

I'd argue that having two #ifdefs is more readable if it means that we can have:

#if ...
#include "copysignl.h"
#endif 

namespace LIBC_NAMESPACE_DECL{
<normal stuff>

#if ...
LLVM_LIBC_ALIASING_FUNCTION(copysignl, copysignf128);
#endif 
}

That way it's clear what is and isn't in the namespace and you don't have to repeat the namespace

Copy link
Contributor Author

@lntue lntue Mar 24, 2025

Choose a reason for hiding this comment

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

from my experience with modifying a lot of these #if blocks, it's much less error-prone when they are contained in just one single #if ... #endif block in a file than spread into multiple blocks. On the other hand, since there are no implementation inside these aliasing, I think the namespace block can be removed from the macro, and replaced with fully-qualified name.

decltype(LIBC_NAMESPACE::name) name [[gnu::alias(#func)]]; \
asm(#name " = " #func); \
Copy link
Member

@petrhosek petrhosek Mar 25, 2025

Choose a reason for hiding this comment

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

Why do you need this block? This is what gnu::alias should do.

Copy link
Contributor Author

@lntue lntue Mar 25, 2025

Choose a reason for hiding this comment

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

We provide both LIBC_NAMESPACE::name C++ symbol and name symbols, and in here, we alias them both to the one that was added by LLVM_LIBC_FUNCTION. For aliasing C symbol to C symbol, I found that asm(" x = y ") is the most portable so far, but for aliasing C++ symbol to C symbol, [[gnu::alias]] seems to be the only reasonable option, but it does not work everywhere.

Copy link
Member

@petrhosek petrhosek Mar 25, 2025

Choose a reason for hiding this comment

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

Why do we need to alias the C++ symbol?

Copy link
Contributor Author

@lntue lntue Mar 25, 2025

Choose a reason for hiding this comment

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

So right now (before this patch), we export 2 public symbols in the library:

  • The C++ namespaced function: LIBC_NAMESPACE::copysign
  • The C standard function: copysign
    and they are aliased to each other.
    And the same for LIBC_NAMESPACE::copysignl and copysignl.
    So in total we have 4 exported symbols, with 2 separate implementations.

With this PR, we still export all those 4 symbols, but making all of them aliased to the same implementation (copysign).

And for the testing (aka .__internal__ targets), previously we have exported only C++ namespaced symbols LIBC_NAMESPACE::copysign and LIBC_NAMESPACE::copysignl with 2 separate implementations. So now to maintain the same structure / implementation, with this PR, we also alias LIBC_NAMESPACE::copysignl with LIBC_NAMESPACE::copysign.

But it's a bit tricky to directly aliasing 2 C++ symbols in a general and portable way. So instead added a C symbol __copysign_impl__ and aliased both C++ symbols to it, following the same construct for the .__internal__ targets. And it's actually cleaner this way.

} \
static_assert(true, "Require semicolon")
Copy link
Member

@petrhosek petrhosek Mar 26, 2025

Choose a reason for hiding this comment

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

I find it a bit unusual for the top-level macros to require semicolon at the end, and it's not common in other libc implementations, see for example: https://sourceware.org/git/?p=glibc.git;a=blob;f=string/basename.c;h=dd2c8182dbc5df71b89100aab33ae98ff4e176ff;hb=HEAD#l28. So my preference would be to avoid requiring semicolons at the end of LLVM_LIBC_ALIAS.

Copy link
Contributor Author

@lntue lntue Mar 26, 2025

Choose a reason for hiding this comment

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

I found that forcing the semicolons at the end of these top level macros plays much nicer with clang-format, especially when they are inside #if blocks. I started to add those to newer top level test macros, and we don't have to occasionally fight with clang-format.

Copy link
Contributor

@michaelrj-google michaelrj-google Apr 2, 2025

Choose a reason for hiding this comment

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

I don't have a strong opinion one way or another, but if we want to commit to this, we should add it to the docs and file an issue for fixing the other macros.

#else
#define LLVM_LIBC_FUNCTION(type, name, arglist) \
LLVM_LIBC_FUNCTION_IMPL(type, name, arglist)
LLVM_LIBC_ATTR(name) \
LLVM_LIBC_FUNCTION_ATTR decltype(LIBC_NAMESPACE::name) \
__##name##_impl__ asm("__" #name "_impl__"); \
decltype(LIBC_NAMESPACE::name) name [[gnu::alias("__" #name "_impl__")]]; \
type __##name##_impl__ arglist

#define LLVM_LIBC_ALIASING_FUNCTION(name, func) \
namespace LIBC_NAMESPACE_DECL { \
decltype(LIBC_NAMESPACE::name) name [[gnu::alias("__" #func "_impl__")]]; \
asm(#name " = __" #func "_impl__"); \
Copy link
Member

@petrhosek petrhosek Mar 25, 2025

Choose a reason for hiding this comment

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

Why do you need this block? This is what gnu::alias should do.

Copy link
Contributor Author

@lntue lntue Mar 25, 2025

Choose a reason for hiding this comment

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

Just noticed I overly exported the C symbol for .__internal__ target also. Removed the asm() statement here.

} \
static_assert(true, "Require semicolon")
#endif // LIBC_COPT_PUBLIC_PACKAGING

#else

#define LLVM_LIBC_FUNCTION(type, name, arglist) type name arglist

#define LLVM_LIBC_ALIASING_FUNCTION(name, func) \
static_assert(true, "Require semicolon")

#endif // !__APPLE__

namespace LIBC_NAMESPACE_DECL {
namespace internal {
Expand Down
30 changes: 25 additions & 5 deletions libc/src/math/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,30 @@ function(add_math_entrypoint_object name)
# We prefer machine specific implementation if available. Hence we check
# that first and return early if we are able to add an alias target for the
# machine specific implementation.
get_fq_target_name("${LIBC_TARGET_ARCHITECTURE}.${name}" fq_machine_specific_target_name)
if(NOT ARGN)
set(alias_entrypoint ${name})
else()
set(alias_entrypoint ${ARGN})
endif()

get_fq_target_name("${LIBC_TARGET_ARCHITECTURE}.${alias_entrypoint}" fq_machine_specific_target_name)
if(TARGET ${fq_machine_specific_target_name})
add_entrypoint_object(
${name}
ALIAS
DEPENDS
.${LIBC_TARGET_ARCHITECTURE}.${name}
.${LIBC_TARGET_ARCHITECTURE}.${alias_entrypoint}
)
return()
endif()

get_fq_target_name("generic.${name}" fq_generic_target_name)
get_fq_target_name("generic.${alias_entrypoint}" fq_generic_target_name)
if(TARGET ${fq_generic_target_name})
add_entrypoint_object(
${name}
ALIAS
DEPENDS
.generic.${name}
.generic.${alias_entrypoint}
)
return()
endif()
Expand All @@ -40,6 +46,20 @@ function(add_math_entrypoint_object name)
)
endfunction()

function(add_long_double_math_entrypoint_object name)
get_fq_target_name(${name} fq_double_math_target)
get_fq_target_name("${name}l" fq_long_double_math_target)
get_fq_target_name("${name}f128" fq_float128_math_target)

if(LIBC_TARGET_LONG_DOUBLE_IS_DOUBLE)
add_math_entrypoint_object("${name}l" "${name}")
elseif(LIBC_TARGET_LONG_DOUBLE_IS_FLOAT128)
add_math_entrypoint_object("${name}l" "${name}f128")
else()
add_math_entrypoint_object("${name}l")
endif()
endfunction()

add_math_entrypoint_object(acos)
add_math_entrypoint_object(acosf)
add_math_entrypoint_object(acosf16)
Expand Down Expand Up @@ -88,9 +108,9 @@ add_math_entrypoint_object(ceilf128)

add_math_entrypoint_object(copysign)
add_math_entrypoint_object(copysignf)
add_math_entrypoint_object(copysignl)
add_math_entrypoint_object(copysignf16)
add_math_entrypoint_object(copysignf128)
add_long_double_math_entrypoint_object(copysign)
Copy link
Contributor

@jhuber6 jhuber6 Mar 24, 2025

Choose a reason for hiding this comment

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

Why is this even necessary? We should just be able to enable this entrypoint like anything else and then just let the preprocessor figure out whether or not it should be an alias.

Copy link
Contributor Author

@lntue lntue Mar 24, 2025

Choose a reason for hiding this comment

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

it wouldn't be a problem for the libc target but it will be a problem for tests. Because our tests for copysignl only include libc.src.math.copysignl target, and we cannot have copysignl in one target aliasing to copysign in libc.src.math.copysign target.

Copy link
Contributor

@jhuber6 jhuber6 Mar 24, 2025

Choose a reason for hiding this comment

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

But they're still independent targets, right? It's just that one is a single alias rather than an implementation.

Copy link
Contributor Author

@lntue lntue Mar 24, 2025

Choose a reason for hiding this comment

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

My plan in the future is to add all the variations: copysign, copysigndd, copysignf80, copysignf128, and just alias copysignl to one of them.

Copy link
Contributor

@jhuber6 jhuber6 Mar 24, 2025

Choose a reason for hiding this comment

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

I think the main difficulty is that the alias attribute wants the function to be defined.

Copy link
Contributor Author

@lntue lntue Mar 24, 2025

Choose a reason for hiding this comment

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

So to answer to your question about the cmake changes: we could leave the cmake inplace, and make copysignl.cpp generate nothing when copysignl function is aliased to copysign or copysignf128, but then we will need to change the testing dependency for copysignl_test, because copysignl symbol is not defined in libc.src.math.copysignl -> libc.src.math.generic.copysignl anymore, due to copysignl symbol now has to be in the same translation unit with copysign or copysignf128. So it is actually easier to let libc.src.copysignl point to libc.src.math.generic.copysign or libc.src.math.generic.copysignf128 instead.


add_math_entrypoint_object(cos)
add_math_entrypoint_object(cosf)
Expand Down
9 changes: 9 additions & 0 deletions libc/src/math/generic/copysign.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "src/__support/FPUtil/ManipulationFunctions.h"
#include "src/__support/common.h"
#include "src/__support/macros/config.h"
#include "src/__support/macros/properties/types.h"

namespace LIBC_NAMESPACE_DECL {

Expand All @@ -22,3 +23,11 @@ LLVM_LIBC_FUNCTION(double, copysign, (double x, double y)) {
}

} // namespace LIBC_NAMESPACE_DECL

#if defined(LIBC_TYPES_LONG_DOUBLE_IS_FLOAT64) && \
defined(LIBC_ALIAS_LONG_DOUBLE)
#include "src/math/copysignl.h"

LLVM_LIBC_ALIASING_FUNCTION(copysignl, copysign);

#endif // LIBC_TYPES_LONG_DOUBLE_IS_FLOAT64
8 changes: 8 additions & 0 deletions libc/src/math/generic/copysignf128.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,11 @@ LLVM_LIBC_FUNCTION(float128, copysignf128, (float128 x, float128 y)) {
}

} // namespace LIBC_NAMESPACE_DECL

#if defined(LIBC_TYPES_LONG_DOUBLE_IS_FLOAT128) && \
defined(LIBC_ALIAS_LONG_DOUBLE)
#include "src/math/copysignl.h"

LLVM_LIBC_ALIASING_FUNCTION(copysignl, copysignf128);

#endif // LIBC_TYPES_LONG_DOUBLE_IS_FLOAT128
Loading