[llvm] Change fp128 lowering to use f128 functions by default - #76558
Draft
tgross35 wants to merge 1 commit into
Draft
[llvm] Change fp128 lowering to use f128 functions by default#76558tgross35 wants to merge 1 commit into
fp128 lowering to use f128 functions by default#76558tgross35 wants to merge 1 commit into
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
Contributor
Author
|
@efriedma-quic was looking at this on phabricator |
tgross35
force-pushed
the
f128-math-lowering
branch
7 times, most recently
from
December 30, 2023 01:39
16f30b5 to
f6b6ca7
Compare
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,h -- clang/lib/CodeGen/CodeGenModule.cpp llvm/include/llvm/ADT/APFloat.h llvm/include/llvm/CodeGen/TargetLowering.h llvm/include/llvm/TargetParser/Triple.h llvm/lib/Support/APFloat.cpp llvm/lib/TargetParser/Triple.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index a7c6bc583..2167148c0 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -429,7 +429,8 @@ static void checkDataLayoutConsistency(const TargetInfo &Target,
llvm::reportFatalInternalError(Twine("For target ") + Triple.str() +
"LLVM wants to use `long double` symbols for"
"_Float128 libm call lowering, but clang"
- "specifies `long double` as " + SemName);
+ "specifies `long double` as " +
+ SemName);
}
if (Target.vectorsAreElementAligned() != DL.vectorsAreElementAligned()) {
diff --git a/llvm/lib/TargetParser/Triple.cpp b/llvm/lib/TargetParser/Triple.cpp
index d8e7b751d..5bc3dac3d 100644
--- a/llvm/lib/TargetParser/Triple.cpp
+++ b/llvm/lib/TargetParser/Triple.cpp
@@ -2759,12 +2759,14 @@ bool Triple::lowerF128LibmAsLongDouble() const {
// Note that the logic should be kept in sync with Clang's LongDoubleFormat,
// though defaulting to *f128 is always safe if available.
- if (Print) fprintf(stderr, "CHECK 1\n");
+ if (Print)
+ fprintf(stderr, "CHECK 1\n");
// Windows and Apple always use f64 as `long double`.
if (isOSWindows() || isOSDarwin())
return false;
- if (Print) fprintf(stderr, "CHECK 2\n");
+ if (Print)
+ fprintf(stderr, "CHECK 2\n");
// Android and Ohos use binary128 only on x86-64.
if (isAndroid() || isOHOSFamily()) {
if (isX86_64())
@@ -2772,7 +2774,8 @@ bool Triple::lowerF128LibmAsLongDouble() const {
return false;
}
- if (Print) fprintf(stderr, "CHECK 3\n");
+ if (Print)
+ fprintf(stderr, "CHECK 3\n");
// PowerPC `long double` is roughly:
// - f64 on musl
// - ibm128 most of the time, historically
@@ -2782,21 +2785,24 @@ bool Triple::lowerF128LibmAsLongDouble() const {
if (isPPC())
return false;
- if (Print) fprintf(stderr, "CHECK 4\n");
+ if (Print)
+ fprintf(stderr, "CHECK 4\n");
// Most 64-bit architectures use use binary128, a few are binary128 on both
// 64- and 32-bit.
if (isAArch64() || isLoongArch() || isRISCV() || isSPARC64() || isSystemZ() ||
isVE() || isWasm())
return true;
- if (Print) fprintf(stderr, "CHECK 5\n");
+ if (Print)
+ fprintf(stderr, "CHECK 5\n");
// MIPS64 is usually f128, except on FreeBSD-like operating systems. MIPS32
// is f128 only with the N32 ABI (O32 is `f64`).
if ((isMIPS64() || isABIN32()) &&
!(isOSFreeBSD() || isOSKFreeBSD() || isOSDragonFly()))
return true;
- if (Print) fprintf(stderr, "CHECK 6\n");
+ if (Print)
+ fprintf(stderr, "CHECK 6\n");
// By default, make the safe assumption that `long double !== f128`. This
// also catches x86 (`long double` is x87 `f80`)
return false;
|
tgross35
force-pushed
the
f128-math-lowering
branch
from
December 30, 2023 01:45
f6b6ca7 to
efaf313
Compare
Contributor
|
This is basically the approach I was expecting: we check the type of "long double" when we build the TargetLowering, and pick appropriate names based on that. I expect that for -mlong-double-128, you just want to add a module flag that overrides the default choice. I think I'd prefer to keep the clang type information computation independent from the backend's type information, even if it overlaps. We try to layer the clang frontend so it isn't directly tied to LLVM IR outside of CodeGen. My first thought was that the computation of the defaults should be in the backend, not Triple.h, since nothing else needs it at the moment. But I guess it could be useful outside the backend, so maybe that's fine. (At the moment, all the relevant optimizations just check the type of the call itself, but I can imagine certain optimizations could benefit from being able to compute the type without an existing signature to consult.) |
tgross35
force-pushed
the
f128-math-lowering
branch
5 times, most recently
from
January 13, 2024 09:57
c00254c to
67033b2
Compare
tgross35
force-pushed
the
f128-math-lowering
branch
4 times, most recently
from
January 20, 2024 12:36
2208d1c to
76e30ed
Compare
Contributor
Author
|
I'm struggling a bit with how to handle ABI information since that affects layout (e.g. ARM aapcs), which I think explains most of the errors in https://buildkite.com/llvm-project/github-pull-requests/builds/31198#018d26e2-fd17-4e15-a1eb-08580c189056. This needs to be available at TargetLoweringBase::InitLibcalls, which calls TargetMachine is available at that time, so would it be better to move CLayouts from Triple to TargetMachine? If so subclasses could be used rather than the if block, which more closely follows the Clang side. Also, are there currently any module flags that make it to TargetLowering? Looking for a reference on how get the -mlong-double-128 information. |
Contributor
|
Putting a function in TargetMachine seems reasonable. |
Contributor
|
For the question about querying module flags, we do that in a few different places in codegen; grep for "getModuleFlag". Not sure if there's anything specifically in TargetLowering. |
tgross35
force-pushed
the
f128-math-lowering
branch
2 times, most recently
from
February 27, 2025 09:11
8add5ca to
04e87bd
Compare
Contributor
Author
|
Finally getting around to this after more than a year. @efriedma-quic as an alternative to the current implementation of duplicating The advantage is avoided code duplication and the logic is easier to follow. Also this avoids problems if linking a library built with an unexpected The disadvantage is that frontends that don't know about C's (I handle the f128 support for Rust and would much rather never think about |
Contributor
Author
|
In either case, I need to have the module flags available pretty early and I'm not sure how to do that. Ideally they would be available when |
tgross35
commented
Apr 16, 2025
This was referenced Jul 12, 2025
tgross35
force-pushed
the
f128-math-lowering
branch
from
July 13, 2025 09:37
553bb3a to
2fd1d47
Compare
tgross35
force-pushed
the
f128-math-lowering
branch
2 times, most recently
from
July 13, 2025 09:49
b97047c to
fac24c7
Compare
nikic
reviewed
Jul 13, 2025
Contributor
There was a problem hiding this comment.
I would formulate this in terms of Triple::getDefaultLongDoubleWidth() and add a consistency check in
tgross35
force-pushed
the
f128-math-lowering
branch
from
July 15, 2025 02:25
2599996 to
b4c947b
Compare
tgross35
commented
Jul 15, 2025
| } | ||
|
|
||
| /// Set all libm libcalls for _Float128 to `long double` (`*l`) symbols. | ||
| static void setLongDoubleIsF128Libm(RuntimeLibcallsInfo &Info, |
Contributor
Author
There was a problem hiding this comment.
I think I need to get rid of this function and instead make these functions DefaultRuntimeLibcallImpls based on the f128LibmShouldUseLongDouble in RuntimeLibcalls.td, which should fix the z/OS intrinsics tests (since those get overridden earlier than this is called, in the .td). I could use some tablegen help figuring out the best way to do something like:
multiclass LibmLibcallImpls<string libcall_basename = !toupper(NAME),
string rtbasename = !strconcat(NAME, "X")> {
// f32, f64 ...
if ("TT.f128LibmShouldUseLongDouble()` /* evaluated as code rather than a string */) then {
def NAME#"_ld128"
: RuntimeLibcallImpl<!cast<RuntimeLibcall>(libcall_basename#"_F128"),
!subst("X", "l", rtbasename)>;
} else {
def NAME#"_f128"
: RuntimeLibcallImpl<!cast<RuntimeLibcall>(libcall_basename#"_F128"),
!subst("X", "f128", rtbasename)>;
}
}So each libcall gets the right defaults without manually enumerating a list.
Contributor
Author
There was a problem hiding this comment.
Actually I think I can do this later by building a set based on names that end with _ld128. Hm...
Contributor
There was a problem hiding this comment.
Hold off on this, you're jumping in the middle of my partially merged stack. #148575 removes this
arsenm
reviewed
Jul 15, 2025
Comment on lines
+403
to
+407
| llvm::errs() << "For target `" << Triple.str() | ||
| << "` LLVM wants to use `long double` symbols for `_Float128` " | ||
| "libm call lowering, but clang specifies `long double` as `" | ||
| << SemName << "`\n"; | ||
| abort(); |
Contributor
There was a problem hiding this comment.
This is reinventing reportFatalInternalError. But can we just fix the clang field to be set from the triple in the first place?
| } | ||
|
|
||
| /// Set all libm libcalls for _Float128 to `long double` (`*l`) symbols. | ||
| static void setLongDoubleIsF128Libm(RuntimeLibcallsInfo &Info, |
Contributor
There was a problem hiding this comment.
Hold off on this, you're jumping in the middle of my partially merged stack. #148575 removes this
| /// or an invalid version tuple if this triple doesn't have one. | ||
| LLVM_ABI VersionTuple getMinimumSupportedOSVersion() const; | ||
|
|
||
| /// Return true if `_Float128` libcalls should lower to e.g. `sqrtf` (`long |
tgross35
marked this pull request as draft
July 2, 2026 11:57
tgross35
force-pushed
the
f128-math-lowering
branch
4 times, most recently
from
July 2, 2026 12:42
b250d30 to
dae1b69
Compare
LLVM currently emits calls to `*l` (`long double`) libm symbols for `fp128` intrinsics. This works on platforms where `long double` and `_Float128` are the same type, but is incorrect on many platforms. Change RuntimeLibcalls such that `*f128` libcalls are used by default, which is always safe and correct but may not be available. On platforms where it is likely that `sqrtf128` and similar are not available, keep the current behavior of lowering to `*l` symbols if `long double` is `binary128`. The logic for whether f128 is `long double` is based on the platforms in Clang that set `LongDoubleFormat` to `llvm::APFloat::IEEEquad`. Fixes llvm#44744
tgross35
force-pushed
the
f128-math-lowering
branch
from
July 2, 2026 12:52
dae1b69 to
9af4baa
Compare
You can test this locally with the following command:Build idt from compnerd/ids, then for each changed header:
idt -p build/ --main-file <matching-source.cpp> \
--apply-fixits --inplace <header>View the diff from ids-check here.diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index 8beaa4287..0df8cb3e7 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -1295,7 +1295,7 @@ public:
/// Return true if `_Float128` libcalls should lower to e.g. `sqrtf` (`long
/// double`) rather than the default `sqrtf128`.
- bool lowerF128LibmAsLongDouble() const;
+ LLVM_ABI bool lowerF128LibmAsLongDouble() const;
/// @}
/// @name Static helpers for IDs.
|
🪟 Windows x64 Test Results
Failed Tests(click on a test name to see its output) ClangClang.Analysis/Scalable/cli-errors-compilation-unit-id.cppClang.CodeGen/AArch64/fixed-register-global.cClang.Driver/as-warnings.cClang.Driver/cl-showfilenames.cClang.Misc/time-passes.cIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
🐧 Linux x64 Test ResultsThe build failed before running any tests. Click on a failure below to see the details. lib/Support/CMakeFiles/LLVMSupport.dir/APFloat.cpp.oIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LLVM currently emits calls to
*l(long double) libm symbols forfp128intrinsics. This works on platforms wherelong doubleand_Float128are the same type, but is incorrect on many platforms.Change RuntimeLibcalls such that
*f128libcalls are used by default, which is always safe and correct but may not be available. On platforms wherelong doubleisf128, the current lowering to*lsymbols is kept since they are more available.The logic for whether f128 is
long doubleis based on the platforms in Clang that setLongDoubleFormattollvm::APFloat::IEEEquad.Fixes: #44744
Discourse discussion: https://discourse.llvm.org/t/fp128-math-functions-strange-results/72708
Initial patchset: https://reviews.llvm.org/D157836
Includes PRs:
f128libm libcall lowering (NFC) #148308