-
-
Notifications
You must be signed in to change notification settings - Fork 271
ppc64(le): Add an option to use IEEE long double ABI on Linux [2] #4840
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
Changes from 16 commits
05ccbb7
7802f8d
d892d68
84c6a2b
eb7c41c
b0ab201
a698caf
4e7800d
26034e5
f6d20f4
5180b4d
4151349
ae4c0d8
5195def
96c455c
12df0e4
e61fcd7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -404,6 +404,23 @@ void addModuleFlags(llvm::Module &m) { | |
| opts::fCFProtection == opts::CFProtectionType::Full) { | ||
| m.setModuleFlag(ModuleMinFlag, "cf-protection-branch", ConstantOneMetadata); | ||
| } | ||
|
|
||
| // Target specific flags | ||
| const auto ModuleErrFlag = llvm::Module::Error; | ||
| switch (global.params.targetTriple->getArch()) { | ||
| case llvm::Triple::ppc64: | ||
| case llvm::Triple::ppc64le: | ||
| if (target.RealProperties.mant_dig == 113) { | ||
| const auto ConstantIEEE128String = llvm::MDString::get(gIR->context(), "ieeequad"); | ||
| 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); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see Clang emit this information, I am guessing this is to avoid mixing compilation units with different float ABIs when doing LTO. |
||
| } | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| } // anonymous namespace | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -26,13 +26,34 @@ | |||||
| using namespace dmd; | ||||||
| using llvm::APFloat; | ||||||
|
|
||||||
| enum class RealPrecision : uint8_t { Default, Double, Quad, DoubleDouble }; | ||||||
| static llvm::cl::opt<RealPrecision, false> realPrecision{ | ||||||
| "real-precision", | ||||||
| llvm::cl::ZeroOrMore, | ||||||
| llvm::cl::Hidden, | ||||||
| llvm::cl::init(RealPrecision::Default), | ||||||
| llvm::cl::desc("Override the precision of the `real` type"), | ||||||
| llvm::cl::values(clEnumValN(RealPrecision::Double, "double", | ||||||
| "Use double precision (64-bit)"), | ||||||
| clEnumValN(RealPrecision::Quad, "quad", | ||||||
| "Use IEEE quad precision (128-bit)"), | ||||||
| clEnumValN(RealPrecision::DoubleDouble, "doubledouble", | ||||||
| "Use IBM double double precision (128-bit)"))}; | ||||||
|
|
||||||
| namespace { | ||||||
| // Returns the LL type to be used for D `real` (C `long double`). | ||||||
| llvm::Type *getRealType(const llvm::Triple &triple) { | ||||||
| using llvm::Triple; | ||||||
|
|
||||||
| auto &ctx = getGlobalContext(); | ||||||
|
|
||||||
| // If user specified double or quad precision, use it unconditionally. | ||||||
| if (realPrecision == RealPrecision::Double) { | ||||||
| return LLType::getDoubleTy(ctx); | ||||||
| } else if (realPrecision == RealPrecision::Quad) { | ||||||
| return LLType::getFP128Ty(ctx); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tested if this will blow up x86 builds?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.]
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Plus there are some compiler assumptions wrt.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see. Then that's fine by me. [Although LLVM is quite fragile when operating beyond its comfort zone]
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, and for all other architectures, it should throw an error.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 But yeah, we can bail out as a compromise if this option is used for non-ppc64 targets. |
||||||
| } | ||||||
|
|
||||||
| // Android: x86 targets follow ARM, with emulated quad precision for x64 | ||||||
| if (triple.getEnvironment() == llvm::Triple::Android) { | ||||||
| return triple.isArch64Bit() ? LLType::getFP128Ty(ctx) | ||||||
|
|
@@ -64,9 +85,34 @@ llvm::Type *getRealType(const llvm::Triple &triple) { | |||||
| case Triple::wasm64: | ||||||
| return LLType::getFP128Ty(ctx); | ||||||
|
|
||||||
| case Triple::ppc64: | ||||||
| case Triple::ppc64le: | ||||||
| if (realPrecision == RealPrecision::DoubleDouble) { | ||||||
| return LLType::getPPC_FP128Ty(ctx); | ||||||
| } | ||||||
| if (triple.isMusl()) { // Musl uses double | ||||||
| return LLType::getDoubleTy(ctx); | ||||||
| } | ||||||
| #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) { | ||||||
|
||||||
| 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?
Outdated
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)
JohanEngelen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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.
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.
That's a bad detection: big-endian ppc can use ELFv2, and little-endian ppc can also use ELFv1: https://godbolt.org/z/s5eceeqxs.