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.

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

Fix #580 by using a fixed-point implementation for unit conversions using integer representations #615

Draft
wants to merge 39 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
cf9c05f
wip
burnpanck Sep 15, 2024
3635260
disabled two tests which now trigger #614
burnpanck Sep 15, 2024
74f30da
again; fix C++20 compatibility
burnpanck Sep 15, 2024
2f54c76
addessed most review concerns, fixed CI failure
burnpanck Sep 16, 2024
e003a58
fixed and expanded double_width_int implemenation, tried to fix a bug…
burnpanck Sep 16, 2024
60c94da
one more try
burnpanck Sep 16, 2024
d00b330
fixed pedantic error
burnpanck Sep 16, 2024
99d4315
Merge remote-tracking branch 'upstream/master' into feature/fixed-poi…
burnpanck Nov 5, 2024
653d3d2
fix formatting issues
burnpanck Nov 5, 2024
ed2574f
allow use of __(u)int128, and always use std::bit_width and friends
burnpanck Nov 5, 2024
e688ffc
silence pedantic warning about __int128
burnpanck Nov 5, 2024
55d8fd6
cross-platform silencing of pedantic warning
burnpanck Nov 5, 2024
38dcf64
Merge remote-tracking branch 'upstream/master' into feature/fixed-poi…
burnpanck Nov 6, 2024
ad76149
Apply suggestions from code review
burnpanck Nov 6, 2024
95cc9f3
more review-requested changes, good test-coverage of double_width_int…
burnpanck Nov 6, 2024
5f8eb5c
made hi_ and lo_ private members of double_width_int
burnpanck Nov 6, 2024
1b57404
attempt to fix tests on apple clang
burnpanck Nov 6, 2024
f673619
try to work around issues around friend instantiations of double_widt…
burnpanck Nov 6, 2024
f642d37
fix: gcc-12 friend compilation issue workaround
mpusz Nov 9, 2024
b6a6752
implement dedicated facilities to customise scaling of numbers with m…
burnpanck Nov 10, 2024
647ce6b
fixed a few more details
burnpanck Nov 10, 2024
464ecd4
Merge remote-tracking branch 'upstream/master' into feature/fixed-poi…
burnpanck Nov 10, 2024
e933be7
fix a few issues uncovered in CI
burnpanck Nov 11, 2024
6873c8b
fix formatting
burnpanck Nov 11, 2024
65a0ee4
fix module exports - does not yet inlude other review input
burnpanck Nov 11, 2024
0c1971e
addressed most review input
burnpanck Nov 11, 2024
4ef0210
fix includes (and use curly braces for constructor calls in measurmen…
burnpanck Nov 12, 2024
35ed472
first attempt at generating sparse CI run matrix in python; also, can…
burnpanck Nov 12, 2024
329b9f5
Merge branch 'master' into feature/faster-CI
burnpanck Nov 12, 2024
7fa15d2
fix formatting
burnpanck Nov 12, 2024
e464677
don't test Clang 19 just yet; fix cancel-in-progres
burnpanck Nov 12, 2024
cc9ea9d
add cancel-in-progress to all workflows
burnpanck Nov 12, 2024
a51462c
missing checkout in generate-matrix step
burnpanck Nov 12, 2024
f4c8e90
fix boolean conan options in dynamic CI matrix
burnpanck Nov 12, 2024
01f44c6
heed github warning, and use output file instead of set-output comman…
burnpanck Nov 12, 2024
5713243
fix clang 16
burnpanck Nov 12, 2024
ff11878
exclude clang18+debug from freestanding again
burnpanck Nov 12, 2024
b35e241
fix clang on macos-14 (arm64)
burnpanck Nov 12, 2024
ef0e7b3
Merge branch 'feature/faster-CI' into feature/fixed-point-multiplicat…
burnpanck Nov 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions example/measurement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,37 @@ class measurement {
[[nodiscard]] friend constexpr measurement operator+(const measurement& lhs, const measurement& rhs)
{
using namespace std;
return measurement(lhs.value() + rhs.value(), hypot(lhs.uncertainty(), rhs.uncertainty()));
return measurement{std::in_place, lhs.value() + rhs.value(), hypot(lhs.uncertainty(), rhs.uncertainty())};
}

[[nodiscard]] friend constexpr measurement operator-(const measurement& lhs, const measurement& rhs)
{
using namespace std;
return measurement(lhs.value() - rhs.value(), hypot(lhs.uncertainty(), rhs.uncertainty()));
return measurement{std::in_place, lhs.value() - rhs.value(), hypot(lhs.uncertainty(), rhs.uncertainty())};
}

template<typename To, mp_units::Magnitude M>
[[nodiscard]] constexpr measurement<To> scale(std::type_identity<measurement<To>>, M scaling_factor) const
Copy link
Owner

Choose a reason for hiding this comment

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

This type_identity usage here is quite novel. I understand the rationale, but maybe we can find a more traditional way of doing it? @JohelEGP, do you have any ideas here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea comes from boost::hana::type. Would be great if the standard had its own dedicated "type tag".

{
constexpr std::type_identity<To> to_value_type;
return measurement<To>{
std::in_place,
mp_units::scale(to_value_type, scaling_factor, value()),
mp_units::scale(to_value_type, scaling_factor, value()),
};
}

template<mp_units::Magnitude M>
[[nodiscard]] constexpr auto scale(M scaling_factor) const
{
return measurement{
std::in_place,
mp_units::scale(scaling_factor, value()),
mp_units::scale(scaling_factor, value()),
};
}


[[nodiscard]] friend constexpr measurement operator*(const measurement& lhs, const measurement& rhs)
{
const auto val = lhs.value() * rhs.value();
Expand Down Expand Up @@ -127,15 +149,23 @@ class measurement {
private:
value_type value_{};
value_type uncertainty_{};

// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
constexpr measurement(std::in_place_t, value_type val, value_type err) :
Copy link
Owner

Choose a reason for hiding this comment

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

This is not the right way of using in_place. in_place means that we pass a list of arguments for construction of one underlying type.

This is not the case here. We initialize two members with one argument each.

Copy link
Owner

Choose a reason for hiding this comment

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

If you would like to have in-place like construction for type then probably something like piecewise_construct is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just wanted a tag to select the private constructor, indicating "I know what I'm doing - I guarantee that err is positive". It's mostly a performance optimisation though, so, for an example, we could just leave it out completely.

Copy link
Owner

Choose a reason for hiding this comment

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

We can either remove it or use a new dedicated tag. Maybe something similar to:

inline constexpr struct validated_tag {
} validated;

value_(std::move(val)), uncertainty_(std::move(err))
{
}
};

} // namespace


template<typename T>
constexpr bool mp_units::is_scalar<measurement<T>> = true;
template<typename T>
constexpr bool mp_units::is_vector<measurement<T>> = true;

static_assert(mp_units::RepresentationOf<measurement<int>, mp_units::quantity_character::scalar>);
static_assert(mp_units::RepresentationOf<measurement<double>, mp_units::quantity_character::scalar>);

namespace {
Expand Down
1 change: 1 addition & 0 deletions src/core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ add_mp_units_module(
include/mp-units/bits/module_macros.h
include/mp-units/bits/quantity_spec_hierarchy.h
include/mp-units/bits/ratio.h
include/mp-units/bits/scaling.h
include/mp-units/bits/sudo_cast.h
include/mp-units/bits/text_tools.h
include/mp-units/bits/type_list.h
Expand Down
194 changes: 122 additions & 72 deletions src/core/include/mp-units/bits/fixed_point.h
mpusz marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -38,54 +38,70 @@ import std;
#endif
#endif

namespace mp_units {
namespace detail {
namespace mp_units::detail {

template<typename T>
constexpr std::size_t integer_rep_width_v = std::numeric_limits<std::make_unsigned_t<T>>::digits;


// this class synthesizes a double-width integer from two base-width integers.
template<std::integral T>
struct double_width_int {
Copy link
Owner

Choose a reason for hiding this comment

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

If I am not wrong, this class only works at compile-time. If that is the case, then all its interfaces should be consteval. If some older compilers will complain, then we have an MP_UNITS_CONSTEVAL workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the arithmetic operators are needed at runtime for the fixed-point scaling (scaling an i64 with a constexpr fixed-point i64.64 is implemented as a multiplication by an i128 followed by a right-shift by 64 bit). There is a bit of freedom in what rounding behaviour we'd like to have, which would require support for some runtime addition/subtraction as-well.

On the other hand, constructors could be made consteval for the fixed-point use-case. That said, I was just increasing test coverage for the arithmetic operators, which I am currently writing as runtime tests. The goal is to test a significant number of value combinations, and while this could be done at compile-time, I fear for the compilation time :-).

static constexpr bool is_signed = std::is_signed_v<T>;
static constexpr std::size_t base_width = std::numeric_limits<std::make_unsigned_t<T>>::digits;
static constexpr std::size_t base_width = integer_rep_width_v<T>;
static constexpr std::size_t width = 2 * base_width;

using Th = T;
using Tl = std::make_unsigned_t<T>;

constexpr double_width_int() = default;
constexpr double_width_int(const double_width_int&) = default;

constexpr double_width_int& operator=(const double_width_int&) = default;
#if !MP_UNITS_COMP_GCC || MP_UNITS_COMP_GCC > 12
private:
#endif
constexpr double_width_int(Th hi, Tl lo) : hi_(hi), lo_(lo) {}

friend struct double_width_int<std::conditional_t<is_signed, std::make_unsigned_t<T>, std::make_signed_t<T>>>;

constexpr double_width_int(Th hi_, Tl lo_) : hi(hi_), lo(lo_) {}
public:
static constexpr double_width_int from_hi_lo(Th hi, Tl lo) { return {hi, lo}; }

explicit(true) constexpr double_width_int(long double v)
explicit constexpr double_width_int(long double v)
{
constexpr auto scale = int_power<long double>(2, base_width);
constexpr auto iscale = 1.l / scale;
auto scaled = v * iscale;
hi = static_cast<Th>(scaled);
auto resid = (scaled - static_cast<long double>(hi));
hi_ = static_cast<Th>(scaled);
auto resid = (scaled - static_cast<long double>(hi_));
if (resid < 0) {
--hi;
--hi_;
resid += 1;
}
lo = static_cast<Tl>(resid * scale);
lo_ = static_cast<Tl>(resid * scale);
}
template<std::integral U>
requires(is_signed || !std::is_signed_v<U>)
explicit(false) constexpr double_width_int(U v)
{
if constexpr (is_signed) {
hi = v < 0 ? Th{-1} : Th{0};
hi_ = v < 0 ? Th{-1} : Th{0};
} else {
hi = 0;
hi_ = 0;
}
lo = static_cast<Tl>(v);
lo_ = static_cast<Tl>(v);
}

explicit(true) constexpr operator Th() const { return static_cast<Th>(lo); }
template<std::integral U>
explicit constexpr operator U() const
{
if constexpr (integer_rep_width_v<U> > base_width) {
return (static_cast<U>(hi_) << base_width) + static_cast<U>(lo_);
} else {
return static_cast<U>(lo_);
}
}

constexpr auto operator<=>(const double_width_int&) const = default;
[[nodiscard]] constexpr auto operator<=>(const double_width_int&) const = default;

// calculates the double-width product of two base-size integers; this implementation requires at least one of them to
// be unsigned
Expand All @@ -108,21 +124,24 @@ struct double_width_int {
}

template<std::integral Rhs>
friend constexpr auto operator*(const double_width_int& lhs, Rhs rhs)
requires(std::numeric_limits<Rhs>::digits <= base_width)
[[nodiscard]] friend constexpr auto operator*(const double_width_int& lhs, Rhs rhs)
{
// Normal C++ rules; with respect to signedness, the bigger type always wins.
using ret_t = double_width_int;
auto ret = ret_t::wide_product_of(rhs, lhs.lo);
ret.hi += lhs.hi * rhs;
return ret;
using RT = std::conditional_t<std::is_signed_v<Rhs>, std::make_signed_t<Tl>, Tl>;
auto lo_prod = double_width_int<RT>::wide_product_of(rhs, lhs.lo_);
// Normal C++ rules; with respect to signedness, the wider type always wins.
using ret_t = double_width_int<Th>;
return ret_t{static_cast<Th>(lo_prod.hi_) + lhs.hi_ * static_cast<Th>(rhs), lo_prod.lo_};
}
template<std::integral Lhs>
friend constexpr auto operator*(Lhs lhs, const double_width_int& rhs)
requires(std::numeric_limits<Lhs>::digits <= base_width)
[[nodiscard]] friend constexpr auto operator*(Lhs lhs, const double_width_int& rhs)
{
return rhs * lhs;
}
template<std::integral Rhs>
friend constexpr double_width_int operator/(const double_width_int& lhs, Rhs rhs)
requires(std::numeric_limits<Rhs>::digits <= base_width)
[[nodiscard]] friend constexpr double_width_int operator/(const double_width_int& lhs, Rhs rhs)
{
// Normal C++ rules; with respect to signedness, the bigger type always wins.
using ret_t = double_width_int;
Expand All @@ -133,19 +152,19 @@ struct double_width_int {
return lhs / static_cast<Tl>(rhs);
}
} else if constexpr (is_signed) {
if (lhs.hi < 0) {
if (lhs.hi_ < 0) {
return -((-lhs) / rhs);
} else {
using unsigned_t = double_width_int<Tl>;
auto tmp = unsigned_t{static_cast<Tl>(lhs.hi), lhs.lo} / rhs;
return ret_t{static_cast<Th>(tmp.hi), tmp.lo};
auto tmp = unsigned_t{static_cast<Tl>(lhs.hi_), lhs.lo_} / rhs;
return ret_t{static_cast<Th>(tmp.hi_), tmp.lo_};
}
} else {
Th res_hi = lhs.hi / rhs;
Th res_hi = lhs.hi_ / rhs;
// unfortunately, wide division is hard: https://en.wikipedia.org/wiki/Division_algorithm.
// Here, we just provide a somewhat naive implementation of long division.
Tl rem_hi = lhs.hi % rhs;
Tl rem_lo = lhs.lo;
Tl rem_hi = lhs.hi_ % rhs;
Tl rem_lo = lhs.lo_;
Tl res_lo = 0;
for (std::size_t i = 0; i < base_width; ++i) {
// shift in one bit
Expand All @@ -164,65 +183,94 @@ struct double_width_int {

template<std::integral Rhs>
requires(std::numeric_limits<Rhs>::digits <= base_width)
friend constexpr double_width_int operator+(const double_width_int& lhs, Rhs rhs)
[[nodiscard]] friend constexpr double_width_int operator+(const double_width_int& lhs, Rhs rhs)
{
// this follows the usual (but somewhat dangerous) rules in C++; we "win", as we are the larger type.
// -> signed computation only of both types are signed
if constexpr (is_signed, std::is_signed_v<Rhs>) {
if (rhs < 0) return lhs - static_cast<std::make_unsigned_t<Rhs>>(-rhs);
Th rhi = lhs.hi_;
Tl rlo = lhs.lo_;
if constexpr (std::is_signed_v<Rhs>) {
// sign extension; no matter if lhs is signed, negative rhs sign extend
if (rhs < 0) --rhi;
}
Tl ret = lhs.lo + static_cast<Tl>(rhs);
return {lhs.hi + Th{ret < lhs.lo ? 1 : 0}, ret};
rlo += static_cast<Tl>(rhs);
if (rlo < lhs.lo_) {
// carry bit
++rhi;
}
return {rhi, rlo};
}
template<std::integral Lhs>
friend constexpr double_width_int operator+(Lhs lhs, const double_width_int& rhs)
[[nodiscard]] friend constexpr double_width_int operator+(Lhs lhs, const double_width_int& rhs)
{
return rhs + lhs;
}
template<std::integral Rhs>
requires(std::numeric_limits<Rhs>::digits <= base_width)
friend constexpr double_width_int operator-(const double_width_int& lhs, Rhs rhs)
[[nodiscard]] friend constexpr double_width_int operator-(const double_width_int& lhs, Rhs rhs)
{
// this follows the usual (but somewhat dangerous) rules in C++; we "win", as we are the larger type.
// -> signed computation only of both types are signed
if constexpr (is_signed, std::is_signed_v<Rhs>) {
if (rhs < 0) return lhs + static_cast<std::make_unsigned_t<Rhs>>(-rhs);
Th rhi = lhs.hi_;
Tl rlo = lhs.lo_;
if constexpr (std::is_signed_v<Rhs>) {
// sign extension; no matter if lhs is signed, negative rhs sign extend
if (rhs < 0) ++rhi;
}
Tl ret = lhs.lo - static_cast<Tl>(rhs);
return {lhs.hi - Th{ret > lhs.lo ? 1 : 0}, ret};
rlo -= static_cast<Tl>(rhs);
if (rlo > lhs.lo_) {
// carry bit
--rhi;
}
return {rhi, rlo};
}

template<std::integral Lhs>
friend constexpr double_width_int operator-(Lhs lhs, const double_width_int& rhs)
[[nodiscard]] friend constexpr double_width_int operator-(Lhs lhs, const double_width_int& rhs)
{
return rhs + lhs;
Th rhi = 0;
Tl rlo = static_cast<Tl>(lhs);
if constexpr (std::is_signed_v<Lhs>) {
// sign extension; no matter if rhs is signed, negative lhs sign extend
if (lhs < 0) --rhi;
}
rhi -= rhs.hi_;
if (rhs.lo_ > rlo) {
// carry bit
--rhi;
}
rlo -= rhs.lo_;
return {rhi, rlo};
}

constexpr double_width_int operator-() const { return {(lo > 0 ? -1 : 0) - hi, -lo}; }
[[nodiscard]] constexpr double_width_int operator-() const
{
return {(lo_ > 0 ? static_cast<Th>(-1) : Th{0}) - hi_, -lo_};
}

constexpr double_width_int operator>>(unsigned n) const
[[nodiscard]] constexpr double_width_int operator>>(unsigned n) const
{
if (n >= base_width) {
return {static_cast<Th>(hi < 0 ? -1 : 0), static_cast<Tl>(hi >> (n - base_width))};
return {static_cast<Th>(hi_ < 0 ? -1 : 0), static_cast<Tl>(hi_ >> (n - base_width))};
}
return {hi >> n, (static_cast<Tl>(hi) << (base_width - n)) | (lo >> n)};
return {hi_ >> n, (static_cast<Tl>(hi_) << (base_width - n)) | (lo_ >> n)};
}
constexpr double_width_int operator<<(unsigned n) const
[[nodiscard]] constexpr double_width_int operator<<(unsigned n) const
{
if (n >= base_width) {
return {static_cast<Th>(lo << (n - base_width)), 0};
return {static_cast<Th>(lo_ << (n - base_width)), 0};
}
return {(hi << n) + static_cast<Th>(lo >> (base_width - n)), lo << n};
return {(hi_ << n) + static_cast<Th>(lo_ >> (base_width - n)), lo_ << n};
}

static constexpr double_width_int max() { return {std::numeric_limits<Th>::max(), std::numeric_limits<Tl>::max()}; }

Th hi;
Tl lo;
#if !MP_UNITS_COMP_GCC || MP_UNITS_COMP_GCC > 12
private:
#endif
Th hi_;
Tl lo_;
};

#if defined(__SIZEOF_INT128__)
MP_UNITS_DIAGNOSTIC_PUSH
MP_UNITS_DIAGNOSTIC_IGNORE("-Wpedantic")
MP_UNITS_DIAGNOSTIC_IGNORE_PEDANTIC
using int128_t = __int128;
using uint128_t = unsigned __int128;
MP_UNITS_DIAGNOSTIC_POP
Expand All @@ -233,8 +281,6 @@ using uint128_t = double_width_int<std::uint64_t>;
constexpr std::size_t max_native_width = 64;
#endif

template<typename T>
constexpr std::size_t integer_rep_width_v = std::numeric_limits<std::make_unsigned_t<T>>::digits;
template<typename T>
constexpr std::size_t integer_rep_width_v<double_width_int<T>> = double_width_int<T>::width;

Expand Down Expand Up @@ -278,32 +324,36 @@ constexpr auto wide_product_of(Lhs lhs, Rhs rhs)
// and neither always cause underflow or overflow.
template<std::integral T>
struct fixed_point {
using repr_t = double_width_int_for_t<T>;
using value_type = double_width_int_for_t<T>;
static constexpr std::size_t fractional_bits = integer_rep_width_v<T>;

constexpr fixed_point() = default;
constexpr fixed_point(const fixed_point&) = default;

constexpr fixed_point& operator=(const fixed_point&) = default;
explicit constexpr fixed_point(value_type v) : int_repr_(v) {}

explicit constexpr fixed_point(repr_t v) : int_repr_is_an_implementation_detail_(v) {}

explicit constexpr fixed_point(long double v) :
int_repr_is_an_implementation_detail_(static_cast<repr_t>(v * int_power<long double>(2, fractional_bits)))
explicit constexpr fixed_point(long double v)
{
long double scaled = v * int_power<long double>(2, fractional_bits);
int_repr_ = static_cast<value_type>(scaled);
// round away from zero; scaling will truncate towards zero, so we need to do the opposite to prevent
// double rounding.
if (int_repr_ >= 0) {
if (scaled > static_cast<long double>(int_repr_)) int_repr_++;
} else {
if (scaled < static_cast<long double>(int_repr_)) int_repr_--;
}
}

template<std::integral U>
requires(integer_rep_width_v<U> <= integer_rep_width_v<T>)
constexpr auto scale(U v) const
[[nodiscard]] constexpr auto scale(U v) const
{
auto res = v * int_repr_is_an_implementation_detail_;
auto res = v * int_repr_;
return static_cast<std::conditional_t<is_signed_v<decltype((res))>, std::make_signed_t<U>, U>>(res >>
fractional_bits);
}

repr_t int_repr_is_an_implementation_detail_;
private:
value_type int_repr_;
};

} // namespace detail
} // namespace mp_units
} // namespace mp_units::detail
Loading
Loading