Skip to content

Specialize multiplication for Fixed #220

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

Merged
merged 3 commits into from
Aug 25, 2020
Merged

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Aug 21, 2020

This is a sequel to PR #213. This specializes most of the multiplication for Fixed and avoids floating point operations.

A major change is that the rounding mode is changed from RoundNearestTiesUp to RoundNearest. The existing RoundNearestTiesUp and RoundDown (the latter was noted in a comment) modes are now supported by the new unexported function mul_with_rounding.

Unlike multiplication for Normed, the wrapping arithmetic is the default for Fixed.

This PR also improves rem.

Fixes #173, Fixes #219

@codecov
Copy link

codecov bot commented Aug 21, 2020

Codecov Report

Merging #220 into master will increase coverage by 0.28%.
The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
+ Coverage   91.01%   91.29%   +0.28%     
==========================================
  Files           6        6              
  Lines         534      563      +29     
==========================================
+ Hits          486      514      +28     
- Misses         48       49       +1     
Impacted Files Coverage Δ
src/fixed.jl 95.27% <93.93%> (+0.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 134646f...9e7c522. Read the comment docs.

@kimikage kimikage marked this pull request as ready for review August 24, 2020 05:51
@kimikage
Copy link
Collaborator Author

kimikage commented Aug 25, 2020

Benchmark

See Normed version

x_q0f7  = collect(rand(Q0f7,  1000, 1000)); y_q0f7  = collect(rand(Q0f7,  1000, 1000));
x_q0f15 = collect(rand(Q0f15, 1000, 1000)); y_q0f15 = collect(rand(Q0f15, 1000, 1000));
x_q3f4  = collect(rand(Q3f4,  1000, 1000)); y_q3f4  = collect(rand(Q3f4,  1000, 1000));
x_q3f12 = collect(rand(Q3f12, 1000, 1000)); y_q3f12 = collect(rand(Q3f12, 1000, 1000));
z_q3f4  = Q3f4.( rand(1000, 1000));
z_q3f12 = Q3f12.(rand(1000, 1000));


julia> @btime $x_q0f7 .* $y_q0f7;
  96.199 μs (2 allocations: 976.70 KiB)

julia> @btime $x_q0f15 .* $y_q0f15;
  796.499 μs (2 allocations: 1.91 MiB)

julia> @btime saturating_mul.($x_q3f4, $y_q3f4);
  155.601 μs (2 allocations: 976.70 KiB)

julia> @btime saturating_mul.($x_q3f12, $y_q3f12);
  909.100 μs (2 allocations: 1.91 MiB)

julia> @btime checked_mul.($x_q3f4, $z_q3f4);
  1.575 ms (2 allocations: 976.70 KiB)

julia> @btime checked_mul.($x_q3f12, $z_q3f12);
  1.881 ms (2 allocations: 1.91 MiB)

rem (cf. #219 (comment))

julia> x_f32 = rand(Float32, 1000, 1000);

julia> @btime $x_f32 .% Q0f7;
  762.600 μs (2 allocations: 976.70 KiB)

cf. v0.8.4 (RoundNearestTiesUp version)

julia> @btime $x_q0f7 .* $y_q0f7;
  77.900 μs (2 allocations: 976.70 KiB)

julia> @btime $x_q0f15 .* $y_q0f15;
  758.600 μs (2 allocations: 1.91 MiB)

I feel we can improve it a bit more. The Fixed{Int16} multiplication clearly has a speed problem, considering the performance of Fixed{Int8} and Normed{UInt16}, but the cause is not clear. However, I don't plan to work on optimizing it as a priority.

@kimikage kimikage force-pushed the mul_fixed branch 2 times, most recently from 5dbd0cd to 2f9710b Compare August 25, 2020 07:32
This provides the multiplication compatible with v0.8 and earlier as `mul_with_rounding(x, y, RoundNearestTiesUp)`.
This also provides the rounding-down version `mul_with_rounding(x, y, RoundDown)`.
The function is informative and not exported.
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

I haven't worked through the details of how you've implemented this, but since all you're doing is adding tests and getting much better performance, there's definitely no reason to avoid merging this.

@kimikage
Copy link
Collaborator Author

kimikage commented Aug 25, 2020

As an additional note for later readers, the default *, i.e. wrapping_mul, is slightly slower due to the change in rounding mode. Instead, at least for Fixed{Int8} and Fixed{Int16}, the results will agree with the float.

As for the rem, the speed is greatly improved by allowing incorrect results from overflow and rounding as well as Normed.

In order to address the operations related to division, I will first merge this.

@kimikage kimikage merged commit 9ca0d9d into JuliaMath:master Aug 25, 2020
@kimikage kimikage deleted the mul_fixed branch August 25, 2020 23:26
@kimikage kimikage mentioned this pull request Apr 30, 2024
38 tasks
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
)"

This specializes most of the multiplication for `Fixed` and avoids floating point operations.
A major change is that the rounding mode is changed from `RoundNearestTiesUp` to `RoundNearest`.
The existing `RoundNearestTiesUp` and `RoundDown` modes are now supported by the new unexported function `mul_with_rounding`.
This also improves `rem`.
Unlike multiplication for `Normed`, the wrapping arithmetic is the default for `Fixed`.
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
)"

This specializes most of the multiplication for `Fixed` and avoids floating point operations.
A major change is that the rounding mode is changed from `RoundNearestTiesUp` to `RoundNearest`.
The existing `RoundNearestTiesUp` and `RoundDown` modes are now supported by the new unexported function `mul_with_rounding`.
This also improves `rem`.
Unlike multiplication for `Normed`, the wrapping arithmetic is the default for `Fixed`.
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
)"

This specializes most of the multiplication for `Fixed` and avoids floating point operations.
A major change is that the rounding mode is changed from `RoundNearestTiesUp` to `RoundNearest`.
The existing `RoundNearestTiesUp` and `RoundDown` modes are now supported by the new unexported function `mul_with_rounding`.
This also improves `rem`.
Unlike multiplication for `Normed`, the wrapping arithmetic is the default for `Fixed`.
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
)"

This specializes most of the multiplication for `Fixed` and avoids floating point operations.
A major change is that the rounding mode is changed from `RoundNearestTiesUp` to `RoundNearest`.
The existing `RoundNearestTiesUp` and `RoundDown` modes are now supported by the new unexported function `mul_with_rounding`.
This also improves `rem`.
Unlike multiplication for `Normed`, the wrapping arithmetic is the default for `Fixed`.
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
)"

This specializes most of the multiplication for `Fixed` and avoids floating point operations.
A major change is that the rounding mode is changed from `RoundNearestTiesUp` to `RoundNearest`.
The existing `RoundNearestTiesUp` and `RoundDown` modes are now supported by the new unexported function `mul_with_rounding`.
This also improves `rem`.
Unlike multiplication for `Normed`, the wrapping arithmetic is the default for `Fixed`.
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.

Problems with rem for Fixed Changing rounding mode of "mul" (*) for Fixed to RoundNearest
2 participants