Skip to content
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

[WIP] use jl_strtof_c to convert strings to float #49699

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

inkydragon
Copy link
Member

@inkydragon inkydragon commented May 9, 2023

ref: #49689
This pr tries to fix the precision problem when rounding doubles to floats on *nix platforms.
The rounding problem on the windows platform will be fixed in the next pr.

indirect conversion (string->double->float) will lose precision

before pr

Version 1.10.0-DEV.1255 (2023-05-10) Commit e204e20

julia --lisp

> (string->number "17.328679084777833")
17.32867908477783

> (float (string->number "17.328679084777833"))
17.328678f

> (string->number "17.328679084777833f0")
17.328678f

> (float (string->number "17.328679084777833f0"))
17.328678f

julia

julia> 17.328679084777833f0
17.328678f0

julia> 17.328679084777833f0 == 17.32868f0
false

after pr

julia --lisp

> (string->number "17.328679084777833")
17.32867908477783

> (float (string->number "17.328679084777833"))
17.328678f

> (string->number "17.328679084777833f0")
17.32868f

> (float (string->number "17.328679084777833f0"))
17.32868f
  • fix (float xxxx)?

julia

julia> 17.328679084777833f0
17.328678f0

julia> 17.328679084777833f0 == 17.32868f0
false
  • fix rounding for 17.328679084777833f0

src/flisp/read.c Outdated

// NOTE(#49689): DO NOT convert double to float directly,
// indirect conversion (string->double->float) will lose precision.
f = jl_strtof_c(tok, &end);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call (plus the call to jl_strtod_c above) can be sunk inside the conditional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved jl_strtof_c into the branch of if (pval).
But left jl_strtod_c untouched.

julia/src/flisp/read.c

Lines 42 to 46 in e204e20

d = jl_strtod_c(tok, &end);
if (*end == '\0') {
if (pval) *pval = mk_double(fl_ctx, d);
return 1;
}

Calling jl_strtod_c updates char *end before we can do dereference *end.
So I don't think we can move jl_strtod_c.

src/flisp/read.c Outdated Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented May 11, 2023

Somewhat unrelatedly, I recently considered vendor-ing a copy of gdtoa from mingw, with the locale support disabled. It is only about 250k of code, so probably quite reasonable to consider. This led me on a journey to update the copy of gdtoa in mingw upstream itself first though, which has led to discovering that gdtoa had introduced a new bug in strtod("nan"). Anyways, the current state of that work is here: https://github.com/JuliaLang/julia/commits/jn/import-strtod

@inkydragon
Copy link
Member Author

inkydragon commented May 12, 2023

Somewhat unrelatedly

The rounding problem on the windows platform will be fixed in the next pr.

My next plan was to use strtod directly from msvcrt, and then try to change the current strtod implementation on Windows to strtof.
It looks like your branch imports the new strtod/strtof implementation, and our goals appear to be the same.

@vtjnash
Copy link
Member

vtjnash commented May 12, 2023

strtod directly from msvcrt

This one is significantly worse quality, and does not conform to the current C standards.

@gbaraldi
Copy link
Member

gbaraldi commented May 12, 2023

What about LLVM's libc implementation. It doesn't look super complicated (It's c++ but doesn't seem to use any super fancy C++isms) https://github.com/llvm/llvm-project/blob/c13ed1cc75781fccc7cec91017e84341c3047229/libc/src/__support/str_to_float.h#L1079

@vtjnash
Copy link
Member

vtjnash commented Feb 6, 2024

Apparently that implementation should be much (up to 10x) faster than gdtoa used elsewhere, by virtue of being more modern (and failing occasionally): the design for that llvm code is apparently from https://nigeltao.github.io/blog/2020/eisel-lemire.html#testing

Seems worthwhile to vendor that C++ code if we can from LLVM. As mentioned there, this algorithm also exists already in Julia: https://github.com/JuliaData/Parsers.jl/blob/589b9d0f80998ec284874b300da0932557d33513/src/floats.jl#L349

@vtjnash vtjnash added the forget me not PRs that one wants to make sure aren't forgotten label Feb 6, 2024
@inkydragon inkydragon removed the forget me not PRs that one wants to make sure aren't forgotten label Feb 7, 2024
@inkydragon
Copy link
Member Author

inkydragon commented Feb 7, 2024

Do we still need this fix (for flisp)?
Since "JuliaSyntax is the default parser now".

The rounding of Float32 is still incorrect on Windows. Rounds correctly on Linux.
Maybe port the LLVM implementation in another pr?

julia> 17.328679084777833f0
17.328678f0

julia> versioninfo()
Julia Version 1.10.0
Commit 3120989f39 (2023-12-25 18:01 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 32 × 13th Gen Intel(R) Core(TM) i9-13900HX
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, goldmont)
  Threads: 1 on 32 virtual cores

@vtjnash
Copy link
Member

vtjnash commented Feb 7, 2024

We may still need it for some of the default parse implementations in base. I would hope copying in that llvm code would work well enough that we could just do that and vendor a working copy of the algorithm that way with minimal fuss. Though maybe we could also just import that existing Julia code, and avoid calling into C at all for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants