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.

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

ppc64(le): Add an option to use IEEE long double ABI on Linux [2] #4840

Merged
merged 17 commits into from
Feb 20, 2025

Conversation

kinke
Copy link
Member

@kinke kinke commented Feb 18, 2025

Based on #4833, with my proposed changes.

liushuyu and others added 16 commits February 15, 2025 19:15
... and then add a long double rewrite to convert it to ppc_f128 when
lowering
... if -mabi=ieeelongdouble is specified on ppc64
... to avoid a LLVM bug on multiple platforms
This will allow the user to specify -real-precision=quad + -mabi=elfv1
together without changing how LDC parses mabi options
... the system is using the new ABI that supports IEEE 754R long double
instead of the legacy IBM double double format
gen/target.cpp Outdated
#if defined(__linux__) && defined(__powerpc64__)
// for a PowerPC64 Linux build:
// default to the C++ host compiler's `long double` ABI
switch (std::numeric_limits<long double>::digits) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use:

Suggested change
switch (std::numeric_limits<long double>::digits) {
switch (__LDBL_MANT_DIG__) {

This one is defined by the compiler directly and I have tested to contain correct information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine by me. - We should probably only forward the host precision when targeting a Linux ppc64 target, not e.g. default to IEEE quad when cross-compiling to a ppc64 FreeBSD target.

Copy link
Member

Choose a reason for hiding this comment

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

when changed to the compiler macro, could this be #if sequence such that failure happens during compilation rather than at runtime?

return LLType::getDoubleTy(ctx);
} else if (realPrecision == RealPrecision::Quad) {
return LLType::getFP128Ty(ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested if this will blow up x86 builds?

Copy link
Member Author

Choose a reason for hiding this comment

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

Android x86_64 is already using it, so I guess that's fine. - This is an advanced option (hence hidden), for pros only - druntime and Phobos need to match too etc. (no problem for simple betterC stuff in wasm, just need to compile everything with the override). If LLVM can't cope with this format for a target's codegen, it'll error out. [I'm a fan of giving users full control and letting them explore the compiler/LLVM limits.]

Copy link
Member Author

Choose a reason for hiding this comment

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

[Plus there are some compiler assumptions wrt. real precision, in existing TargetABI implementations - as the real precision wasn't configurable so far.]

Copy link
Contributor

Choose a reason for hiding this comment

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

[I'm a fan of giving users full control and letting them explore the compiler/LLVM limits.]

I see. Then that's fine by me. [Although LLVM is quite fragile when operating beyond its comfort zone]

Copy link
Member

Choose a reason for hiding this comment

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

RealPrecision::DoubleDouble should be handled here too (i.e. the user has set it explicitly)

Copy link
Contributor

Choose a reason for hiding this comment

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

RealPrecision::DoubleDouble should be handled here too (i.e. the user has set it explicitly)

Right, and for all other architectures, it should throw an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

RealPrecision::DoubleDouble should be handled here too (i.e. the user has set it explicitly)

That I wanted to avoid though. I originally only wanted to expose the 2 standard IEEE formats as overrides, but unfortunately we need a way to make a native-PPC64 build with default IEEE quad ABI still cross-compile to the old doubledouble ABI. (I still don't wanna expose x87 if we can help it - although admittedly it could be useful for MSVC targets, to unlock the x87 real, which the compiler itself could use as real_t - bye-bye dmd.root.longdouble...)

But yeah, we can bail out as a compromise if this option is used for non-ppc64 targets.

gen/target.cpp Outdated
return LLType::getDoubleTy(ctx);
default:
llvm_unreachable("Unexpected host C++ 'long double' precision for a "
"PowerPC64 target!");
Copy link
Member

Choose a reason for hiding this comment

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

perhaps print the actual __LDBL_MANT_DIG__ number in the diagnostic? (will help troubleshooting when this error triggers)

@JohanEngelen
Copy link
Member

considering there is a difference in C++ mangling, should the D mangling scheme also be extended to differentiate the types? (would help with troubleshooting linking with binaries that have been compiled with different compiler settings, notably with prebuilt druntime/phobos) ?

@kinke
Copy link
Member Author

kinke commented Feb 18, 2025

should the D mangling scheme also be extended to differentiate the types?

I'd say D doesn't need to, assuming PPC will remain the only architecture (well, AFAIK) with such a long double ABI change. [And users now starting to use -real-precision should IMO remain an uncommon special case, for wasm and the like, not needing the nicety of undefined symbols.]

const llvm::SmallVector<llvm::StringRef> features{};
const std::string abi = getABI(triple, features);
VersionCondition::addPredefinedGlobalIdent(abi == "elfv1" ? "ELFv1"
: "ELFv2");
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://github.com/dlang/dmd/blob/e082ce247afa6cf41c552ec57db2e95c3f7c0dac/druntime/src/etc/valgrind/valgrind.h#L154-L159, little-endian uses v2, big-endian v1.

That's a bad detection: big-endian ppc can use ELFv2, and little-endian ppc can also use ELFv1: https://godbolt.org/z/s5eceeqxs.

m.setModuleFlag(ModuleErrFlag, "float-abi", ConstantIEEE128String);
} else if (target.RealProperties.mant_dig == 106) {
const auto ConstantIBM128String = llvm::MDString::get(gIR->context(), "doubledouble");
m.setModuleFlag(ModuleErrFlag, "float-abi", ConstantIBM128String);
Copy link
Member Author

Choose a reason for hiding this comment

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

I take it this is just informational, or is this supported by some toolchains?

Copy link
Contributor

Choose a reason for hiding this comment

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

I take it this is just informational, or is this supported by some toolchains?

I see Clang emit this information, I am guessing this is to avoid mixing compilation units with different float ABIs when doing LTO.

RealProperties.max_exp = 1024;
RealProperties.min_exp = -968;
RealProperties.max_10_exp = 308;
RealProperties.min_10_exp = -291;
Copy link
Member Author

Choose a reason for hiding this comment

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

I was surprised by this being quite a bit greater than double.min_10_exp (-307). How did you get these numbers? Querying GDC or the C++ compiler?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was surprised by this being quite a bit greater than double.min_10_exp (-307). How did you get these numbers? Querying GDC or the C++ compiler?

Yes, the numbers are obtained from GCC/Clang. IBM stated due to the peculiarity of the type, those values are best-effort approximations.

@kinke
Copy link
Member Author

kinke commented Feb 19, 2025

@liushuyu: Are you able to/have you already tested this on ppc64le, checking whether the results haven't suffered since your original PR?

@liushuyu
Copy link
Contributor

@liushuyu: Are you able to/have you already tested this on ppc64le, checking whether the results haven't suffered since your original PR?

I have tested your changes on a POWER 9 system and it seems okay.
Some parts of phobos are failing, but these are also failing in my PR (also these are not compiler issue).

@kinke
Copy link
Member Author

kinke commented Feb 20, 2025

Perfect, thx for testing! So I guess we're set and ready to merge? I'm happy with the overall changes.

@liushuyu
Copy link
Contributor

Perfect, thx for testing! So I guess we're set and ready to merge? I'm happy with the overall changes.

I think it's ready for the merge.

@kinke kinke merged commit 3726a93 into ldc-developers:master Feb 20, 2025
19 of 20 checks passed
@kinke kinke deleted the ppc64-d-ieee754-fix-new branch February 20, 2025 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants