[libc] Special case PPC double double for print#136614
Conversation
jhuber6 requested review from lntue, michaelrj-google, overmighty, petrhosek and tstellar
@llvm/pr-subscribers-backend-powerpc @llvm/pr-subscribers-libc Author: Joseph Huber (jhuber6) ChangesSummary: Fixes: #136596 Full diff: https://github.com/llvm/llvm-project/pull/136614.diff 2 Files Affected:
diff --git a/libc/src/__support/FPUtil/FPBits.h b/libc/src/__support/FPUtil/FPBits.h index bee8d0a8dc47d..608459d53d432 100644 --- a/libc/src/__support/FPUtil/FPBits.h+++ b/libc/src/__support/FPUtil/FPBits.h@@ -38,6 +38,7 @@ enum class FPType { IEEE754_Binary64, IEEE754_Binary128, X86_Binary80, + PPC_DoubleDouble, }; // The classes hierarchy is as follows: @@ -138,6 +139,15 @@ template <> struct FPLayout<FPType::X86_Binary80> { LIBC_INLINE_VAR static constexpr int FRACTION_LEN = SIG_LEN - 1; }; +// TODO: This needs an FPRepSem interface as well.+template <> struct FPLayout<FPType::PPC_DoubleDouble> {+ using StorageType = UInt128;+ LIBC_INLINE_VAR static constexpr int SIGN_LEN = 1;+ LIBC_INLINE_VAR static constexpr int EXP_LEN = 11;+ LIBC_INLINE_VAR static constexpr int SIG_LEN = 106;+ LIBC_INLINE_VAR static constexpr int FRACTION_LEN = SIG_LEN - 1;+};+ // FPStorage derives useful constants from the FPLayout above. template <FPType fp_type> struct FPStorage : public FPLayout<fp_type> { using UP = FPLayout<fp_type>; @@ -790,6 +800,8 @@ template <typename T> LIBC_INLINE static constexpr FPType get_fp_type() { return FPType::IEEE754_Binary64; else if constexpr (__LDBL_MANT_DIG__ == 64) return FPType::X86_Binary80; + else if constexpr (__LDBL_MANT_DIG__ == 106)+ return FPType::PPC_DoubleDouble; else if constexpr (__LDBL_MANT_DIG__ == 113) return FPType::IEEE754_Binary128; } diff --git a/libc/src/stdio/printf_core/core_structs.h b/libc/src/stdio/printf_core/core_structs.h index 4c3b81ff018ab..a0e9a3d0d2c10 100644 --- a/libc/src/stdio/printf_core/core_structs.h+++ b/libc/src/stdio/printf_core/core_structs.h@@ -56,7 +56,8 @@ struct FormatSection { int precision = -1; // Needs to be large enough to hold a long double. - fputil::FPBits<long double>::StorageType conv_val_raw;+ fputil::internal::FPLayout<fputil::get_fp_type<long double>()>::StorageType+ conv_val_raw; void *conv_val_ptr; char conv_name; |
✅ With the latest revision this PR passed the C/C++ code formatter. |
libc/src/__support/FPUtil/FPBits.h Outdated
| using StorageType = | ||
| typename internal::FPLayout<get_fp_type<T>()>::StorageType; |
There was a problem hiding this comment.
This feels like a bit of a hack. Is there a reason we can't add the FPRepSem and leave this as-is?
There was a problem hiding this comment.
Conceptually no, but there's no user of those functions yet and it is non-trivial. I'm not a floating point expert and didn't want to spend a few days double checking the binary format of PPC's double just to close this bug report.
There was a problem hiding this comment.
That's fair. In that case, I'd say file a bug, add a TODO, and I'll approve this so we can move on.
There was a problem hiding this comment.
There's already a TODO to add the full FPRepSem interface, but I don't think it's too bad to remove one layer of indirection for this.
There was a problem hiding this comment.
it's not one layer of indirection though, it's four. I don't feel comfortable saying this is definitely safe, especially given this is an interface we're exporting. This is a very bad place for technical debt.
Summary: Hard code this to a uint128 so it can be built on PPC.
jhuber6 changed the title 
Summary:
We use the storage class for
long doublein the printingimplementations. We don't fully support the PPC double double type,
which that maps to, but we can stub out just the support needed for the
print interface to works. This required using the internal interface for
storage type, but it should be good enough.
Fixes: #136596