core.internal.convert: add IEEE Quadruple support #2257
Conversation
|
Thanks for your pull request, @joakim-noah! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + druntime#2257" |
| @@ -106,8 +124,8 @@ private Float parse(bool is_denormalized = false, T)(T x) if(is(Unqual!T == iflo | |||
| private Float parse(bool is_denormalized = false, T:real)(T x_) if(floatFormat!T != FloatFormat.Real80) | |||
| { | |||
| Unqual!T x = x_; | |||
| assert(floatFormat!T != FloatFormat.DoubleDouble && floatFormat!T != FloatFormat.Quadruple, | |||
| "doubledouble and quadruple float formats are not supported in CTFE"); | |||
| static assert(floatFormat!T != FloatFormat.DoubleDouble, | |||
There was a problem hiding this comment.
This assert was never actually triggered at compile-time.
| enum NAN = Float(-1, 0x7fff, 0); | ||
| enum NNAN = Float(-1, 0x7fff, 1); | ||
| enum NAN = Float(0, 0x7fff, 0, 0x80000000000000UL); | ||
| enum NNAN = Float(0, 0x7fff, 1, 0x80000000000000UL); |
There was a problem hiding this comment.
I didn't bother adding 0's everywhere else the Float constructor was called, as this is the only 112-bit mantissa where the most significant bits in mantissa2 needed to be set.
|
This is a poorly named PR, as this isn't android specific. You infact fix all platforms that have 128bit IEEE reals. |
src/core/internal/convert.d
Outdated
| { | ||
| x *= 2.0L^^FloatTraits!T.MANTISSA; | ||
| auto fl = parse!true(x); | ||
| ulong mant = fl.mantissa >> (FloatTraits!T.MANTISSA - fl.exponent); | ||
| return shiftrRound(mant); | ||
| } | ||
|
|
||
| @safe pure nothrow @nogc | ||
| private Float denormalizedMantissa(T)(T x, uint sign) if(floatFormat!T == FloatFormat.Quadruple) |
There was a problem hiding this comment.
This doesn't overload the above functions and so should have a distinct name, ie: floatDenormalizedMantissa.
Having a look at all uses of the other function, they are all Float(denormalizedMantissa(x), 0, sign), so they all could be refactored to return a Float as well as sharing the same signature as this.
There was a problem hiding this comment.
Good point, I'll fix it.
|
@joakim-noah ping |
|
I want to get back to testing that second commit, but busy with other stuff first. Will get back to the second commit next week probably, and once that's up, this pull should be ready to go. If you've any further feedback on this commit, we can discuss it in the meantime. |
|
Btw, is you pinging me now in any way related to the current gcc submission? If so, feel free to use and modify this patch in any way you want to there: I'm making it available under the same Boost license as the rest of druntime. |
|
No, I was porting druntime to risc-v, and went about implementing this myself, only to find that you've already done it. |
|
This one patch is actually blocking all ports that have 128-bit reals from being buildable. So this a high priority fix IMO. Your other patch can wait for another PR. |
joakim-noah
changed the title
|
Fine with me, added another commit with the change you asked for. Feel free to squash the two and merge once the CI passes. |
…ecision to the floating-point parser.
|
Looks like the FreeBSD64 part of the auto-tester hung, so squashed the commits to get it to restart. |
|
CI passed, ready to go. |
| @@ -314,21 +354,49 @@ private uint binLog2(T)(const T x) | |||
| } | |||
|
|
|||
| @safe pure nothrow @nogc | |||
| private ulong denormalizedMantissa(T)(T x) if(floatFormat!T == FloatFormat.Real80) | |||
| private Float denormalizedMantissa(T)(T x, uint sign) if(floatFormat!T == FloatFormat.Real80) | |||
There was a problem hiding this comment.
I don't suppose these could be named floatDenormalizedMantissa or floatWithDenormalizedMantissa to make it more clear from the caller what they return.
Not too fussed either way.
There was a problem hiding this comment.
Private function only used in this module at CTFE, don't think it's worth kicking off the CI again for this trivial rename.
|
Bikeshed nit, but otherwise LGTM. |
Backport from upstream druntime 2.083 for AArch64. Reviewed-on: dlang/druntime#2257 git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@266222 138bc75d-0d04-0410-961f-82ee72b054a4
Backport from upstream druntime 2.083 for AArch64. Reviewed-on: dlang/druntime#2257 From-SVN: r266222
This is a WIP pull, as I'm going to add another commit that reworks how druntime initializes TLS data on Android before passing it to the GC, based on a suggestion of Martin from the ldc team.I wanted to get this first math commit up now to run it through the CI and review, as this one should be done.I've tested this commit with ldc 0.17 natively on Android/AArch64 and linux/AArch64, and by cross-compiling with ldc 1.11 master for Android/AArch64 (all tests build and pass, but only because they're all truncated at 80-bit precision), along with making sure it didn't break anything on linux/x64. With this patch, all the druntime unit tests finally compile and pass on AArch64.