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.

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

Jump to bottom

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

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 4 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
69 changes: 58 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,64 @@ if(C_SUPPORTS_WERROR_UNGUARDED_AVAILABILITY_NEW)
set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -Werror=unguarded-availability-new")
endif()

function(logf128_test testname definition)
unset(LOGF128_TEST_RUN CACHE)
unset(LOGF128_TEST_COMPILE CACHE)
try_run(
LOGF128_TEST_RUN
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using try_run tends to lead to strange results with cross-compiling; do we really need it here? _Float128/__float128 should always mean what we want it to, and we can use __LDBL_MANT_DIG__ for long double.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

x86 hosts with 80bit long doubles can compile logf128 but will return an error code. __LDBL_MANT_DIG__ does work fine for hosts with 128bit long doubles, but this will also disable the fold for x86 targets with fp80 that can still perform an accurate fold with __float128 types. I'm struggling to think of an alternative way to select the most valid and best type at compile time to use without try_run based on the following criteria:

Logf128 symbol availability
Logf128 function prototype availability
Long double total size (Needs to be 16 bytes)
Long double mantissa size (Rules out fp80)
GCC version(12+ won't accept anything other than _Float128 for logf128)
If we're compiling with GCC or Clang (Clang needs to declare the logf128 prototype)
__float128 availability
_Float128 availability

Copy link
Collaborator

Choose a reason for hiding this comment

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

You only need to check __LDBL_MANT_DIG__ if you're trying to use the type long double. You can assume __float128 and _Float128 have the right format.

Copy link
Contributor Author

@MDevereau MDevereau Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Firstly, thanks for your patience with this review.

I'm still unsure how to differentiate in a header file between _Float128 and long double for GCC12+, where trying to pass in anything other than GCC 12+'s dedicated _Float128 type will cause a compilation error. A condition along the lines of

#if (__LDBL_MANT_DIG__ == 113) && !defined(__LONG_DOUBLE_IBM128__) && (__SIZEOF_INT128__ == 16)
typedef long double float128;
#else

will be valid for GCC12 and GCC11 while it is actually only valid for GCC11: long double isn't valid for logf128 anymore after GCC12, but the host still has a long double mantissa size of 113.

I think you're suggesting I do typedef decltype(logf128(0.)) float128; only when long double is known to have a mantissa of 113, and worry about hosts that have access to float128 but have a manstissa of not 113 in a following patch. I just wanted to double check.

LOGF128_TEST_COMPILE
${CMAKE_CURRENT_BINARY_DIR}
${CMAKE_CURRENT_BINARY_DIR}/logf128_${testname}.cpp
LINK_LIBRARIES m
)
if(LOGF128_TEST_RUN)
set (LLVM_HAS_LOGF128 true CACHE INTERNAL "")
set(${definition} true CACHE INTERNAL "")
message(STATUS "LLVM: found logf128 with type ${testname}")
add_compile_definitions(${definition})
endif()
endfunction()

file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/logf128_long_double.cpp"
"
extern \"C\" {
long double logf128(long double);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd prefer to have CMake tests which #include <cmath>, to verify that the header actually works.

Along those lines, I don't think float128.h should be declaring logf128.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I ended up doing things this way is because Clang on x86 doesn't pick up the logf128 symbol when including cmath. Clang just doesn't seem to have the magic definitions to unlock it like GCC does. It's probably okay to include cmath for long double and _Float128, but for clang I think we do need to explicitly declare the function prototype to get it working on x86.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the initial patch, let's just handle the case where logf128 is actually declared in math.h. The case where glibc actually provides logf128, but doesn't declare it, is a weird edge case, and I'd prefer to handle it in a followup patch.

}
int main() {
long double value = logf128(32.0);
if (value > 3.465730L & value < 3.465740L)
return 1;
return 0;
}")

file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/logf128___float128.cpp"
"
extern \"C\" {
__float128 logf128(__float128);
}
int main() {
__float128 value = logf128(32.0);
if (value > 3.465730L & value < 3.465740L)
return 1;
return 0;
}")

file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/logf128__Float128.cpp"
"
extern \"C\" {
_Float128 logf128(_Float128);
}
int main() {
_Float128 value = logf128(32.0);
if (value > 3.465730L & value < 3.465740L)
return 1;
return 0;
}")

logf128_test("long_double" HAS_LONG_DOUBLE_LOGF128)
logf128_test("__float128" HAS__FLOAT128_LOGF128)
logf128_test("_Float128" HAS_FLOAT128_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
4 changes: 2 additions & 2 deletions llvm/include/llvm/ADT/APFloat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ class IEEEFloat final : public APFloatBase {
Expected<opStatus> convertFromString(StringRef, roundingMode);
APInt bitcastToAPInt() const;
double convertToDouble() const;
#ifdef HAS_IEE754_FLOAT128
#if defined(HAS_IEE754_FLOAT128)
float128 convertToQuad() const;
#endif
float convertToFloat() const;
Expand Down Expand Up @@ -1267,7 +1267,7 @@ class APFloat : public APFloatBase {
/// \pre The APFloat must be built using semantics, that can be represented by
/// the host float type without loss of precision. It can be IEEEquad and
/// shorter semantics, like IEEEdouble and others.
#ifdef HAS_IEE754_FLOAT128
#if defined(HAS_IEE754_FLOAT128)
float128 convertToQuad() const;
#endif

Expand Down
15 changes: 9 additions & 6 deletions llvm/include/llvm/Support/float128.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,18 @@

namespace llvm {

#if defined(__clang__) && defined(__FLOAT128__) && \
defined(__SIZEOF_INT128__) && !defined(__LONG_DOUBLE_IBM128__)
#ifdef HAS_FLOAT128_LOGF128
typedef _Float128 float128;
#define HAS_IEE754_FLOAT128
#elif HAS__FLOAT128_LOGF128
typedef __float128 float128;
#elif defined(__FLOAT128__) && defined(__SIZEOF_INT128__) && \
!defined(__LONG_DOUBLE_IBM128__) && \
(defined(__GNUC__) || defined(__GNUG__))
extern "C" {
float128 logf128(float128);
}
#define HAS_IEE754_FLOAT128
#elif HAS_LONG_DOUBLE_LOGF128
typedef long double float128;
#define HAS_IEE754_FLOAT128
typedef _Float128 float128;
#endif

} // namespace llvm
Expand Down
6 changes: 0 additions & 6 deletions llvm/lib/Analysis/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,3 @@ add_llvm_component_library(LLVMAnalysis
Support
TargetParser
)

include(CheckCXXSymbolExists)
check_cxx_symbol_exists(logf128 math.h HAS_LOGF128)
if(HAS_LOGF128)
target_compile_definitions(LLVMAnalysis PRIVATE HAS_LOGF128)
endif()
8 changes: 4 additions & 4 deletions llvm/lib/Analysis/ConstantFolding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1783,9 +1783,9 @@ Constant *ConstantFoldFP(double (*NativeFP)(double), const APFloat &V,
return GetConstantFoldFPValue(Result, Ty);
}

#if defined(HAS_IEE754_FLOAT128) && defined(HAS_LOGF128)
Constant *ConstantFoldFP128(long double (*NativeFP)(long double),
const APFloat &V, Type *Ty) {
#if defined(HAS_IEE754_FLOAT128)
Constant *ConstantFoldFP128(float128 (*NativeFP)(float128), const APFloat &V,
Type *Ty) {
llvm_fenv_clearexcept();
float128 Result = NativeFP(V.convertToQuad());
if (llvm_fenv_testexcept()) {
Expand Down Expand Up @@ -2116,7 +2116,7 @@ static Constant *ConstantFoldScalarCall1(StringRef Name,
if (IntrinsicID == Intrinsic::canonicalize)
return constantFoldCanonicalize(Ty, Call, U);

#if defined(HAS_IEE754_FLOAT128) && defined(HAS_LOGF128)
#if defined(HAS_IEE754_FLOAT128)
if (Ty->isFP128Ty()) {
if (IntrinsicID == Intrinsic::log) {
float128 Result = logf128(Op->getValueAPF().convertToQuad());
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Support/APFloat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3721,7 +3721,7 @@ double IEEEFloat::convertToDouble() const {
return api.bitsToDouble();
}

#ifdef HAS_IEE754_FLOAT128
#if defined(HAS_IEE754_FLOAT128)
float128 IEEEFloat::convertToQuad() const {
assert(semantics == (const llvm::fltSemantics *)&semIEEEquad &&
"Float semantics are not IEEEquads");
Expand Down Expand Up @@ -5351,7 +5351,7 @@ double APFloat::convertToDouble() const {
return Temp.getIEEE().convertToDouble();
}

#ifdef HAS_IEE754_FLOAT128
#if defined(HAS_IEE754_FLOAT128)
float128 APFloat::convertToQuad() const {
if (&getSemantics() == (const llvm::fltSemantics *)&semIEEEquad)
return getIEEE().convertToQuad();
Expand Down
Loading