-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add support for f128 values in std.fmt.parseFloat. #11254
Add support for f128 values in std.fmt.parseFloat. #11254
Conversation
…it to sqlite's float parsing code and updating that parser to support f128.
Not sure what's going on with the test failure:
|
Also: I found this library, which is MIT licensed: http://www.kerbaya.com/ieee754lib/index.html
Accuracy is lower with less bits, so one wants to use all of them and not upcast things. |
@matu3ba You probably know this, but for clarity I'll mention it: parse_f128.c and the previous implementation of parseFloat that this PR replaces are two separate parsers. This PR is not directly about parse_f128.c - it's about making parseFloat pass tests that were previously failing. Those tests now pass.
Mentioning parse_f128.c may have been a distraction. I only mentioned it because at first my approach for this PR was to port it to zig in order to make parseFloat support f128. Once I noticed that parse_f128.c had typos and my port had strange behavior for some numbers, I abandoned that path because the port was hard enough even when I had faith in the source material. It's possible that parse_f128.c is perfect (besides that one typo this PR corrects) and the issue was in my zig port of it. But the code is so unreadable I abandoned it. Regardless, any potential issues with it are separate from this PR. I'd be happy to write a new GitHub issue about testing parse_f128.c if you think it's worth it. Probably best to forget I mentioned parse_f128.c at all.
144115188075855870 can be represented exactly by both f64 and f128. The previous implementation of parseFloat didn't get it right when parsing to f64 OR f128. Even if it was able to get that specific number correct, the previous parseFloat used f64s internally, and so you always got f64's range and precision, even if asking for a f128.
I compared the behavior of this PR against c's atof and found that they matched for 64 bit floats. atof and that implementation you linked to do not support f128, so I wasn't able to verify any f128 numbers. One of the tests for f128 I manually figured out the 128 bits the number should be parsed as - the test has that written as hex.
I'll look into this... more rigorous testing beyond what we had for the previous implementation of parseFloat may be a separate task.
I agree, and the parser working in a higher bit depth is the point of this PR. I was suggesting that maybe it would be worthwhile to make parseFloat be written generically so that its calculations happen in f64 when an f64 number is requested. This would have benefits for targets which f128 is software emulated/not supported. If a f128 type was requested, then yes, it would work in the higher bit depth for accuracy. |
Really clean and readable implementation - Very nice work 👍 Can you clarify the rounding behavior, and any support for subnormals? Based on the code, those probably depend on the configuration of the FPU/softfloat library at the time of parsing? |
// test range of f128 | ||
// at time of writing (Mar 2021), zig prints f128 values larger than f64 as "inf", | ||
// so I'm not 100% sure this hex literal is the corrent parse of 1e4930 | ||
try expectEqual(@bitCast(u128, try parseFloat(f128, "1e4930")), 0x7ff8136c69ce8adff4397b050cae44c7); |
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.
This looks like it's off by 1 bit to me - should be 0x7ff8136c69ce8adff4397b050cae44c6
Based on a rudimentary calculation in Python, intended to get the error in ulp:
>>> exp = 0x7ff8 - 0x3fff
>>> (10**4930 - 0x1136c69ce8adff4397b050cae44c7 * 2**(exp-112)) / 2**(exp-112)
-1.2945749668158448
>>> (10**4930 - 0x1136c69ce8adff4397b050cae44c6 * 2**(exp-112)) / 2**(exp-112)
-0.2945749668158449
I might have made a mistake in the calculation, of course
Disable the non-LLVM backends in the newly passing test case:
These other backends do not handle f128 yet. |
I'm confused, The failing test cases on the ci server seem unrelated to the Additionally, parseFloat now requires f128 internally - is that going to be an issue for the backends that don't support f128? My musings from my previous comment are related, and I'm curious to get your take:
|
I didn't look closely when I made this suggestion. You will hit the problem that I mentioned next after you solve this one. The command that is failing, on both x86_64-macos and x86_64-linux, is:
All of these tests have float literals in them. Regarding reliance on f128 - it's OK to rely on f128. On targets where such type is not supported by the hardware, zig provides compiler-rt functions for softfloat implementations. However, it would likely be more efficient to limit use to f64 when parsing an f64, and limit use to f32 when parsing an f32, and I would like to see that improvement made eventually. That improvement could come as a follow-up to this PR, or it could be part of this one. |
Another note for future optimization: A lot of the fastest float parsers seem to favor a "fast path" where the math can be done easily in smaller types, and then fall back to slower options only for extremely long/precise literals. This tries to give the "best of both worlds": exact precision/rounding in the difficult case, and fast performance in the common case The fast path can usually be chosen based on the number of digits in the literal, and the decimal exponent. fast_double_parser takes this approach, for example, and I think the stage1 float parser has a few similar tricks |
I would like to repeat my suggestion to port musl's float parsing code (which has already been ported to self-contained .c code in parse_f128.c). If you want to port a different implementation instead (such as the one in this PR), can you provide some benchmark results to compare perf against it? |
Sure! Since I don't have a zig port of musl's parser, I put both the sqlite and musl parsers in a .c file and had them parse If you're satisfied with this, I'll get to making the tests pass and look into making it work in f64/f32 when f128 isn't requested. @topolarity I noticed in this test that this unaltered sqlite implementation differs strtod/musl by 4 ULP for numbers with large exponents on arm64. If Andrew is ok with pursuing this sqlite implementation, I can look into the source of the difference. x64 in rosetta mode
arm64
main.c source
Test Notes |
If it's faster and still correct that is a clear win. Nice! |
I'm taking this opportunity to make a general statement: I don't care about perf of x64 in rosetta mode. It's completely irrelevant to the near future of computing, in which Apple deprecates x86 entirely and we are left with only native ARM software on macOS. Zig is young enough that we can aim a little bit for the future and not the present, and this is one of those times. arm64 perf matters. Native x86_64 perf matters. Rosetta-emulated x86_64 perf does not matter. Same deal with effort spent on compatibility, QA, testing, etc. It's all a waste of time. Given that we have limited efforts to spend, we should spend our efforts on native x86_64 and native arm64 and forget that Rosetta exists. I personally don't even have Rosetta installed. |
Closing stale PR. Feel free to open a new one if you decide to complete this. |
Add f128 support to std.fmt.parseFloat by transitioning it to sqlite's float parser, adapted to support 128 floats. The old implementation worked in f64 internally, and then upcast to f128 at the last moment.
Additionally, this fixes a typo in parse_f128.c - my initial approach for this PR was to port that parser to zig as recommended in #11169 (comment), but found it had issues parsing large f64 values like 144115188075855870, even after fixing the typo. I suspect there may be other issues with parse_f128.c, but due to its style, it's hard to know what else might be wrong. I also explored adapting the existing parser to truly support f128 (and not just upcast from f64 to f128), but in early tests it exhibited incorrect behavior for parsing large f64 values, and it relied on some custom 96bit-integer code that made it difficult to adapt to f128. Sqlite's f64 parser behaved correctly out of the box and is much easier to read, so was easy to adapt to support f128.
I'm not sure what the implications are for relying on f128 math in parseFloat now - vs working in f64 and then casting to f16/f32/f64/f128 at the last moment like the old implementation did. Is f128 math emulated in software for targets that don't natively support it? Should we make parseFloat work in f64 when f128 isn't the requested return type?
Fixes #11169