-
-
Notifications
You must be signed in to change notification settings - Fork 264
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
Conversation
... and then add a long double rewrite to convert it to ppc_f128 when lowering
... if -mabi=ieeelongdouble is specified on ppc64
... if IEEE long double ABI is selected
... 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.]
There was a problem hiding this comment.
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.]
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!"); |
There was a problem hiding this comment.
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)
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) ? |
I'd say D doesn't need to, assuming PPC will remain the only architecture (well, AFAIK) with such a |
const llvm::SmallVector<llvm::StringRef> features{}; | ||
const std::string abi = getABI(triple, features); | ||
VersionCondition::addPredefinedGlobalIdent(abi == "elfv1" ? "ELFv1" | ||
: "ELFv2"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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. |
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. |
Based on #4833, with my proposed changes.