Skip to content

Conversation

@oscardssmith
Copy link
Member

This removes a bunch of compats for unsupported Julia versions, and slightly simplifies the annoying hacks we're making to handle the ABI issues.

@oscardssmith oscardssmith requested a review from DilumAluthge May 13, 2025 21:16
Copy link
Member

@simonbyrne simonbyrne left a comment

Choose a reason for hiding this comment

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

That's a much better idea, I don't know why I didn't do it this way originally.

@oscardssmith
Copy link
Member Author

Also, while I'm cleaning things up, Is the current approach of calling C functions for the basic conversions good? I see ~2x faster performance using the pure Julia UInt128 conversion than the ccalled UInt64 conversion (zen5 linux). If one of you has an aarc64 system (which has native Float128), can you run this benchmark?

julia> @btime Float128(x) setup=x=rand(UInt64)
  10.249 ns (0 allocations: 0 bytes)

@benchmark Float128(UInt128(x)) setup=x=rand(UInt64)
  3.727 ns (0 allocations: 0 bytes)

@simonbyrne
Copy link
Member

On my M2 Mac

julia> @btime Float128(x) setup=x=rand(UInt64)
  2.750 ns (0 allocations: 0 bytes)
5.31167148155784243000000000000000000e+18

julia> @btime Float128(UInt128(x)) setup=x=rand(UInt64)
  2.208 ns (0 allocations: 0 bytes)
6.82926167534499090300000000000000000e+18

Yeah, so that looks faster...

@simonbyrne
Copy link
Member

simonbyrne commented May 14, 2025

You can also clean up these bits as well:

Quadmath.jl/src/Quadmath.jl

Lines 191 to 197 in f6ea87e

if Sys.iswindows()
return reinterpret(Float128,d)
else
y1 = reinterpret(Float64,UInt64(d >> 64))
y2 = reinterpret(Float64,(d % UInt64))
return Float128((VecElement(y2),VecElement(y1)))
end

Quadmath.jl/src/Quadmath.jl

Lines 215 to 221 in f6ea87e

if Sys.iswindows()
return reinterpret(Float128,d)
else
y1 = reinterpret(Float64,UInt64(d >> 64))
y2 = reinterpret(Float64,(d % UInt64))
Float128((VecElement(y2),VecElement(y1)))
end

@oscardssmith
Copy link
Member Author

@simonbyrne I might have gone slightly overboard and moved a lot of the simple implementations into Julia with pretty decent perf gains (usually ~2x sometimes as big as 8x)

@oscardssmith
Copy link
Member Author

so, should we merge this?

@DilumAluthge
Copy link
Member

I'll defer to @simonbyrne

@JeffreySarnoff
Copy link
Member

I'll say. Simon very nice grist for Float128. Floats where the edge [cases] have been honed to refract

@simonbyrne
Copy link
Member

Let's bump the minor version (or make it 1.0) then it's good

@oscardssmith
Copy link
Member Author

Lets go to 1.0!

@JeffreySarnoff
Copy link
Member

I do not agree with going to 1.0 until a few other issues are addressed.
At the very least
NaN, Inf should not show as lowercase
#97

@oscardssmith
Copy link
Member Author

That's easy enough to fix. Would printing as Float128(Inf) and Float128(NaN) seem good to you? (that would match what we do for Float16)

@JeffreySarnoff
Copy link
Member

julia> using Quadmath

julia> Float128(Inf)
inf # should be Inf128

julia> Float128(NaN)
nan # should be NaN128

julia> isdefined(Quadmath, :Inf128), isdefined(Quadmath, :NaN128)
(true, false) # should be (true, true)

@oscardssmith
Copy link
Member Author

Are we willing to rely on Base.Ryu for this? it already has the code and it works really well.

@oscardssmith
Copy link
Member Author

Also, how would we feel about printing/showing these as Inf and NaN rather than Inf128 and NaN128? That would match what we do for Inf32 and other 32 bit types.

@JeffreySarnoff
Copy link
Member

"Also, how would we feel about printing/showing these as Inf and NaN rather than Inf128 and NaN128? That would match what we do for Inf32 and other 32 bit types."
Better

@oscardssmith
Copy link
Member Author

oscardssmith commented Sep 29, 2025

I pushed the changes, and then immediately tested with a non trivial case and it broke (Base.Ryu is fairly non generic and requires a bunch of internal functions to be defined to work). That said, I don't love the snprintf printing either since it doesn't provide a way of doing shortest printing to match Julia.

I've moved it a bit closer to Julia printing now, but there are still a few quirks. For example string(Float128(3.0)) is "3" rather than "3.0". IMO this is close enough to be acceptable.

@oscardssmith
Copy link
Member Author

so... merge?

@JeffreySarnoff JeffreySarnoff merged commit 3f9190f into master Oct 22, 2025
10 checks passed
@JeffreySarnoff JeffreySarnoff deleted the os/cleanup branch October 22, 2025 14:08
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.

5 participants