Skip to content

[libc][math][c23] Optimize fabsf16 on x86 with Clang #104869

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 1 commit into
base: main
Choose a base branch
from
Open

[libc][math][c23] Optimize fabsf16 on x86 with Clang #104869

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
4 changes: 3 additions & 1 deletion libc/src/math/generic/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,10 +563,12 @@ add_entrypoint_object(
HDRS
../fabsf16.h
DEPENDS
libc.src.__support.macros.properties.types
libc.src.__support.FPUtil.basic_operations
libc.src.__support.FPUtil.fp_bits
libc.src.__support.macros.properties.architectures
libc.src.__support.macros.properties.compiler
libc.src.__support.macros.properties.cpu_features
libc.src.__support.macros.properties.types
COMPILE_OPTIONS
-O3
FLAGS
Expand Down
17 changes: 14 additions & 3 deletions libc/src/math/generic/fabsf16.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,30 @@

#include "src/math/fabsf16.h"
#include "src/__support/FPUtil/BasicOperations.h"
#include "src/__support/FPUtil/FPBits.h"
#include "src/__support/common.h"
#include "src/__support/macros/config.h"
#include "src/__support/macros/properties/architectures.h"
#include "src/__support/macros/properties/compiler.h"
#include "src/__support/macros/properties/cpu_features.h"

namespace LIBC_NAMESPACE_DECL {

LLVM_LIBC_FUNCTION(float16, fabsf16, (float16 x)) {
// For x86, GCC generates better code from the generic implementation.
// https://godbolt.org/z/K9orM4hTa
#if defined(__LIBC_MISC_MATH_BASIC_OPS_OPT) && \
!(defined(LIBC_TARGET_ARCH_IS_X86) && defined(LIBC_COMPILER_IS_GCC))
defined(LIBC_TARGET_CPU_HAS_FAST_FLOAT16_OPS)
return __builtin_fabsf16(x);
#elif defined(LIBC_TARGET_ARCH_IS_X86) && defined(LIBC_COMPILER_IS_CLANG)
// Prevent Clang from generating calls to slow soft-float conversion
// functions on x86. See https://godbolt.org/z/hvo6jbnGz.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a codegen bug. it's wrong to introduce a canonicalizing extend when legalizing fabs. Don't workaround it here?


using FPBits = fputil::FPBits<float16>;
using StorageType = typename FPBits::StorageType;

static constexpr volatile StorageType ABS_MASK = FPBits::EXP_SIG_MASK;

return FPBits(static_cast<StorageType>(FPBits(x).uintval() & ABS_MASK))
.get_val();
#else
return fputil::abs(x);
#endif
Expand Down
Loading