Skip to content

Commit

Permalink
Fix off-by-one error in rounding truncation in calculate_inverse_coeff()
Browse files Browse the repository at this point in the history
We must use the precise value of 2^nbits(T) in order to get the correct
division in all cases.

....... UGH except now the Int64 tests aren't passing.
  • Loading branch information
NHDaly committed Jun 19, 2024
1 parent 27a3e0f commit e4cb73b
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 1 deletion.
3 changes: 2 additions & 1 deletion src/fldmod-by-const.jl
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ Base.@assume_effects :foldable function calculate_inverse_coeff(::Type{T}, C) wh
# Now, truncate to only the upper half of invcoeff, after we've shifted. Instead of
# bitshifting, we round to maintain precision. (This is needed to prevent off-by-ones.)
# -- This is equivalent to `invcoeff = T(invcoeff >> sizeof(T))`, except rounded. --
invcoeff = _round_to_nearest(fldmod(invcoeff, typemax(UT))..., typemax(UT)) % T
two_to_N = _widen(typemax(UT)) + UT(1) # Precise value for 2^nbits(T) (doesn't fit in T)
invcoeff = _round_to_nearest(fldmod(invcoeff, two_to_N)..., two_to_N ) % T
return invcoeff, toshift
end

Expand Down
24 changes: 24 additions & 0 deletions test/fldmod-by-const_tests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,30 @@ end
end
end

@testset "fixed decimal multiplication - exhaustive 8-bit" begin
@testset for P in (0,1)
@testset for T in (Int8, UInt8)
FD = FixedDecimal{T,P}

function test_multiplies_correctly(fd, x)
big = FixedDecimal{BigInt, P}(fd)
big_mul = big * x
# This might overflow: ...
mul = fd * x
@testset "$fd * $x" begin
# ... so we truncate big to the same size
@test big_mul.i % T == mul.i % T
end
end
@testset for v in typemin(FD) : eps(FD) : typemax(FD)
@testset for v2 in typemin(FD) : eps(FD) : typemax(FD)
test_multiplies_correctly(v, v2)
end
end
end
end
end

@testset "fixed decimal multiplication - exhaustive 16-bit" begin
@testset for P in (0,1,2,3,4)
@testset for T in (Int16, UInt16)
Expand Down

0 comments on commit e4cb73b

Please sign in to comment.