Skip to content

Enable logf128 constant folding for hosts with 128bit long double #96287

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

Merged
merged 8 commits into from
Aug 9, 2024
Merged

Enable logf128 constant folding for hosts with 128bit long double #96287

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4cc6905
Enable logf128 constant folding for AArch64
MDevereau Jun 21, 2024
97a0b20
Remove aarch64 constraint
MDevereau Jun 26, 2024
94c6914
Enable optimization without opt-in
MDevereau Jun 26, 2024
422fed8
Check for viable logf128 and use that for float128's typedef
MDevereau Jul 11, 2024
613f6a4
Remove cmake tests
MDevereau Jul 17, 2024
5705316
Lit uses LLVM_HAS_LOGF128 to run tests and is no longer a command lin…
MDevereau Jul 18, 2024
6ceda72
Remove check for __LDBL_MANT_DIG__
MDevereau Aug 6, 2024
cc0a937
Merge branch 'main' into aarch64-logf128
MDevereau Aug 9, 2024
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
13 changes: 2 additions & 11 deletions llvm/cmake/config-ix.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,17 +247,6 @@ else()
set(HAVE_LIBEDIT 0)
endif()

if(LLVM_HAS_LOGF128)
include(CheckCXXSymbolExists)
check_cxx_symbol_exists(logf128 math.h HAS_LOGF128)

if(LLVM_HAS_LOGF128 STREQUAL FORCE_ON AND NOT HAS_LOGF128)
message(FATAL_ERROR "Failed to configure logf128")
endif()

set(LLVM_HAS_LOGF128 "${HAS_LOGF128}")
endif()

# function checks
check_symbol_exists(arc4random "stdlib.h" HAVE_DECL_ARC4RANDOM)
find_package(Backtrace)
Expand All @@ -271,6 +260,8 @@ if(C_SUPPORTS_WERROR_UNGUARDED_AVAILABILITY_NEW)
set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -Werror=unguarded-availability-new")
endif()

check_function_exists(logf128 math.h HAS_LOGF128)

# Determine whether we can register EH tables.
check_symbol_exists(__register_frame "${CMAKE_CURRENT_LIST_DIR}/unwind.h" HAVE_REGISTER_FRAME)
check_symbol_exists(__deregister_frame "${CMAKE_CURRENT_LIST_DIR}/unwind.h" HAVE_DEREGISTER_FRAME)
Expand Down
10 changes: 9 additions & 1 deletion llvm/include/llvm/Support/float128.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,15 @@

namespace llvm {

#if defined(__clang__) && defined(__FLOAT128__) && \
#if !defined(__LONG_DOUBLE_IBM128__) && (__SIZEOF_LONG_DOUBLE__ == 16) && \
(__SIZEOF_INT128__ == 16) && (__LDBL_MANT_DIG__ == 113)
#define HAS_IEE754_FLOAT128
#if (defined(__GNUC__) && __GNUC__ > 12)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of explicitly checking the gcc version, can we typedef decltype(logf128(0.)) float128; or something like that?

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 would require including math.h in this file (which isn't a problem but worth mentioning). It's also possible for hosts to satisfy the condition and fall through to this but not have logf128 (I've seen x86 with clang do this). It's possible to add HAS_LOGF128 to the check, which is set at CMake time in order to get around this, but this isn't really about logf128 and more about the 128 bit floating-point types themselves. Granted, my logf128 patches are the only thing using this, but it still seems incorrect.

Apart from this, it does function well though.

Copy link
Collaborator

@efriedma-quic efriedma-quic Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm mostly concerned that checking for __GNUC__ is overly specific, and might break on compilers you haven't seen, or compilers which don't exist yet (e.g. if clang's support for _Float128 changes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've instead decided to move to a CMake test which checks the availability of logf128, the availability of a true 128bit float type and that type's compatibility with the logf128 symbol to receive accurate answers instead of trying to do this inside a header file.

A problem I faced when trying to run this on an x86 machine was that the size of long doubles were defined to be 16 bytes (when they are actually fp80) however calling logf128 with this type caused error codes to be returned from the function while compiling without issue. Using __float128 which is guaranteed to perform a 128 bit calculation even though the machine uses fp80 works OK though. I'm seeing both x86 with fp80 and aarch64 compile and run this without issues now. Instead of explicitly checking the GCC version, the _Float128 type which is available in GCC12+ is now asserted for availability and correctness and used if it is available.

typedef _Float128 float128;
#else
typedef long double float128;
#endif
#elif defined(__clang__) && defined(__FLOAT128__) && \
defined(__SIZEOF_INT128__) && !defined(__LONG_DOUBLE_IBM128__)
#define HAS_IEE754_FLOAT128
typedef __float128 float128;
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Analysis/ConstantFolding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1784,8 +1784,8 @@ Constant *ConstantFoldFP(double (*NativeFP)(double), const APFloat &V,
}

#if defined(HAS_IEE754_FLOAT128) && defined(HAS_LOGF128)
Constant *ConstantFoldFP128(long double (*NativeFP)(long double),
const APFloat &V, Type *Ty) {
Constant *ConstantFoldFP128(float128 (*NativeFP)(float128), const APFloat &V,
Type *Ty) {
llvm_fenv_clearexcept();
float128 Result = NativeFP(V.convertToQuad());
if (llvm_fenv_testexcept()) {
Expand Down
Loading