-
Notifications
You must be signed in to change notification settings - Fork 0
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
Use widen(Int128)=BitIntgers.Int256 within FD #6
Use widen(Int128)=BitIntgers.Int256 within FD #6
Conversation
885a823
to
4465412
Compare
Here are the benchmark results from this change -- the improvements are really big! :D (The diff is relative to JuliaMath@1768c58.)
|
|
||
# BitIntegers is missing iseven/isodd | ||
# Prevent expensive calculation for Int256 | ||
Base.isodd(a::BitIntegers.Int256) = Base.isodd(a % Int) # only depends on the final bit! :) |
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.
Is this used? If not consider removing?
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.
Ah yes it is. I've added a comment about why it's used. (_round_to_even
calls iseven
, which in turn calls isodd
.)
src/FixedPointDecimals.jl
Outdated
import BitIntegers | ||
|
||
# BitIntegers is missing unsigned(::Type{Int256}) | ||
Base.unsigned(x::T) where T<:BitIntegers.XBS = reinterpret(unsigned(T), x) |
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.
What is XBS? eXtra Big Stick?
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.
Haha i guess it's "eXtended Bit (integers) Signed". Blech!
But also, i just switched it to XBI
, which is "eXtended Bit Integers", which is what I settled on in the PR i sent to that package: rfourquet/BitIntegers.jl#2
It's basically the super type for all the new integer types that package added.
I clarified that in the comments, and also added a TODO comment to delete these lines once the PRs on the base package are merged. Thanks! :)
(Just force-pushed after rebasing in the comments on #5) |
4465412
to
28994ea
Compare
(Back to you!) Thanks! :) |
f03145e
to
6af05e0
Compare
3fcbff8
to
4043855
Compare
src/FixedPointDecimals.jl
Outdated
@@ -505,6 +536,7 @@ Base.@pure function max_exp10(::Type{T}) where {T <: Integer} | |||
end | |||
|
|||
max_exp10(::Type{BigInt}) = -1 | |||
@eval max_exp10(::Type{Int128}) = $(max_exp10(Int128)) |
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.
comment: force evaluation of max_exp10 at compile time ?
54f865b
to
747b628
Compare
Remove allocations for FD{Int128} by switching to widen to Int256 instead of BigInt.
Adds a dependency on the BitIntegers package, but it seems worth it to me.
I'm merging this into #5, so that we can review them in two chunks.