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.

Sign up for GitHub

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

Jump to bottom

Fix bugs in fixed division #5698

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix bugs in fixed division #5698

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b689dc
Fix bugs in fixed division
hexagonrecursion Dec 27, 2023
43a8208
Add copyright
hexagonrecursion Apr 26, 2024
aa8d723
Use std::abs()
hexagonrecursion Apr 26, 2024
859bf26
Oops: forgot inclide
hexagonrecursion Apr 26, 2024
0ed527c
Add more tests; Fix a bug in test; Better comments
hexagonrecursion Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 20 additions & 15 deletions src/fixed.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,34 +149,39 @@ class fixedf {
}
friend fixedf operator/(const fixedf &a, const fixedf &b)
{
// 128-bit divided by 64-bit, to make sure high bits are not lost
Sint64 quotient_hi = a.v >> (64 - FRAC);
Uint64 quotient_lo = a.v << FRAC;
Sint64 d = b.v;
int isneg = 0;
Sint64 remainder = 0;

if (d < 0) {
d = -d;
isneg = 1;
Uint64 abs_a = a.v, abs_b = b.v;
bool is_neg = false;
if (a.v < 0) {
abs_a = -abs_a;
is_neg = !is_neg;
}

if (b.v < 0) {
abs_b = -abs_b;
is_neg = !is_neg;
}
Copy link
Member

Choose a reason for hiding this comment

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

I would strongly recommend using bitwise ops and std::abs rather than explicit branches, as it will generally compile into optimized code under more scenarios. E.g. something like:

uint64_t abs_a = std::abs(a.v), abs_b = std::abs(b.v);
bool is_neg = int(a.v < 0) ^ int(b.v < 0);

(Note: this is well-defined, true is defined to promote to the integral value 1.)
See this Godbolt for example of how this affects code that would be generated in debug mode: https://godbolt.org/z/6MsTPv8dz

Yes, under -O3 or equivalent this code would produce the same generated assembly, but the above suggestion will both produce better performance in debug mode (wasting less programmer time) and more clearly expresses what is logically occurring in the if statements without the extra cognitive overhead of the order-dependent logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would strongly recommend using bitwise ops and std::abs rather than explicit branches, as it will generally compile into optimized code under more scenarios. E.g. something like:

uint64_t abs_a = std::abs(a.v), abs_b = std::abs(b.v);
bool is_neg = int(a.v < 0) ^ int(b.v < 0);

(Note: this is well-defined, true is defined to promote to the integral value 1.) See this Godbolt for example of how this affects code that would be generated in debug mode: https://godbolt.org/z/6MsTPv8dz

Yes, under -O3 or equivalent this code would produce the same generated assembly, but the above suggestion will both produce better performance in debug mode (wasting less programmer time) and more clearly expresses what is logically occurring in the if statements without the extra cognitive overhead of the order-dependent logic.

Done. Please note that with the requested changes dividing fixed(INT64_MIN) by anything or dividing anything by fixed(INT64_MIN) causes undefined behaviour because std::abs(INT64_MIN) is undefined behaviour.

// 128-bit divided by 64-bit, to make sure high bits are not lost
Uint64 quotient_hi = abs_a >> (64 - FRAC);
Uint64 quotient_lo = abs_a << FRAC;
Uint64 remainder = 0;
for (int i = 0; i < 128; i++) {
Uint64 sbit = (Uint64(1) << 63) & quotient_hi;
// The most significant bit of remainder is 0 because abs_b <= (Uint64(1) << 63)
remainder <<= 1;
if (sbit) remainder |= 1;
if (sbit) {
remainder |= 1;
}
// shift quotient left 1
{
quotient_hi <<= 1;
if (quotient_lo & (Uint64(1) << 63)) quotient_hi |= 1;
quotient_lo <<= 1;
}
if (remainder >= d) {
remainder -= d;
if (remainder >= abs_b) {
remainder -= abs_b;
quotient_lo |= 1;
}
}
return (isneg ? -Sint64(quotient_lo) : quotient_lo);
return is_neg ? -quotient_lo : quotient_lo;
}
friend bool operator==(const fixedf &a, const fixedf &b) { return a.v == b.v; }
friend bool operator!=(const fixedf &a, const fixedf &b) { return a.v != b.v; }
Expand Down
33 changes: 33 additions & 0 deletions src/test/TestFixed.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#include <limits>
Copy link
Member

Choose a reason for hiding this comment

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

Please add:

// Copyright © 2008-2024 Pioneer Developers. See AUTHORS.txt for details
// Licensed under the terms of the GPL v3. See licenses/GPL-3.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I forgot

#include "doctest.h"
#include "fixed.h"

TEST_CASE("Fixed")
{
SUBCASE("operator ==")
{
// 42.0 == 42.0
CHECK(fixed(42, 1) == fixed(42, 1));
// 123.0 != 321.0
CHECK(fixed(123, 1) != fixed(321, 1));
// 7.0 == 7.0
CHECK(fixed(0x7'00000000) == fixed(7, 1));
}

SUBCASE("operator /")
{
// 3.0 / 3.0 == 1.0
CHECK(fixed(3, 1) / fixed(3, 1) == fixed(1, 1));
// -3.0 / 3.0 == -1.0
// There used to be a bug causing this to produce an incorrect result
CHECK(fixed(-3'00000000) / fixed(3, 1) == fixed(-1'00000000));
// 0x40000000.00000000 / 0x40000000.00000001 == 0x00000000.ffffffff (because we round down)
// There used to be a bug causing this to produce an incorrect result
CHECK(fixed(0x40000000'00000000) / fixed(0x40000000'00000001) == fixed(0x00000000'ffffffff));
// fixed(INT_MIN) / fixed(INT_MIN) used to trigger a signed int overflow
Sint64 min = std::numeric_limits<Sint64>::min();
CHECK(fixed(min) / fixed(min) == fixed(1, 1));
// fixed(INT_MIN) / -1.0 used to trigger a signed int overflow
CHECK(fixed(min) / fixed(-1'00000000) == fixed(min) / fixed(-1'00000000));
}
}