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 3 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
6 changes: 6 additions & 0 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 @@ -301,6 +301,12 @@ using min_width_uint_t =
template<std::size_t N>
using min_width_int_t = make_signed_t<min_width_uint_t<N>>;

// TODO: other standard floating point types (half-width floats?)
template<std::size_t N>
using min_digit_float_t =
std::conditional_t<(N <= std::numeric_limits<float>::digits), float,
std::conditional_t<(N <= std::numeric_limits<double>::digits), double, long double>>;

template<typename T>
using double_width_int_for_t = std::conditional_t<is_signed_v<T>, min_width_int_t<integer_rep_width_v<T> * 2>,
min_width_uint_t<integer_rep_width_v<T> * 2>>;
Expand Down
27 changes: 15 additions & 12 deletions src/core/include/mp-units/bits/scaling.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ using minimal_floating_point_type =
template<typename To, typename T>
constexpr auto cast_integral(const T& value)
{
if constexpr (std::is_integral_v<T>) {
if constexpr (std::is_integral_v<value_type_t<T>>) {
return static_cast<To>(value);
} else {
return value;
Copy link
Owner

Choose a reason for hiding this comment

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

If this is meant to be a floating point value, then !treat_as_floating_point should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. The specification of cast_integral<To>(const From)& (an implementation detail) is "cast to To, if From is integral". The point is that the default implementation of scale basically has two separate cases:

  • "use operator*(float)": Applied whenever at least one of From and To is treat_as_floating_point.
  • "use a fixed-point multiplication": Can only be directly be applied to standard integral types, so is used just for those, plus custom types which are implicitly convertible and have an integral value_type.

So the first case is much broader. For standard types, this in particular includes the case where From is integer and To is floating-point. There, just multiplying the integer with some floating-point type may cause precision-loss warnigs to emitted. Those are "bogus", because whenever To is explicitly specified, we're in fact in an "explicit cast" path. So we need to do an explicit cast here. But on the other hand, we cannot unilaterally do an explicit cast, because many types that should go through that path are not at all convertible to that floating-point type (e.g. std::complex).

There are other ways to handle this. We could also just silence the warning locally, or branch-out the mixed int/float paths further above.

Expand All @@ -63,8 +63,10 @@ struct floating_point_scaling_factor_type {
template<std::floating_point T>
struct floating_point_scaling_factor_type<T> : std::type_identity<T> {};

// try to choose the smallest standard floating-point type which can represent the integer exactly (has at least as many
// mantiassa bits as the integer is wide)
template<std::integral T>
struct floating_point_scaling_factor_type<T> : std::common_type<T, float> {};
struct floating_point_scaling_factor_type<T> : std::type_identity<min_digit_float_t<std::numeric_limits<T>::digits>> {};

template<typename T>
requires requires { typename scaling_traits<T>::floating_point_scaling_factor_type; }
Expand All @@ -74,8 +76,6 @@ struct floating_point_scaling_factor_type<T> :

template<Magnitude auto M>
struct floating_point_scaling_impl {
static constexpr Magnitude auto num = _numerator(M);
static constexpr Magnitude auto den = _denominator(M);
template<typename T>
static constexpr T ratio = [] {
using U = long double;
Expand All @@ -88,9 +88,9 @@ struct floating_point_scaling_impl {
using U = minimal_floating_point_type<typename floating_point_scaling_factor_type<To>::type,
typename floating_point_scaling_factor_type<From>::type>;
if constexpr (_is_integral(M)) {
return static_cast<To>(cast_integral<U>(value) * _get_value<U>(num));
return static_cast<To>(cast_integral<U>(value) * _get_value<U>(M));
} else if constexpr (_is_integral(_pow<-1>(M))) {
return static_cast<To>(cast_integral<U>(value) / _get_value<U>(den));
return static_cast<To>(cast_integral<U>(value) / _get_value<U>(_pow<-1>(M)));
} else {
return static_cast<To>(cast_integral<U>(value) * ratio<U>);
}
Expand Down Expand Up @@ -118,7 +118,7 @@ struct floating_point_scaling_traits {
static constexpr Rep scale_from(M, const From& value)
{
return floating_point_scaling_impl<M{}>::template scale<Rep>(value);
};
}

template<Magnitude M>
static constexpr auto scale(M, const Rep& value)
Expand All @@ -131,8 +131,6 @@ struct floating_point_scaling_traits {

template<Magnitude auto M>
struct fixed_point_scaling_impl {
static constexpr Magnitude auto num = _numerator(M);
static constexpr Magnitude auto den = _denominator(M);
template<std::integral T>
static constexpr auto ratio = [] {
using U = long double;
Expand All @@ -145,9 +143,9 @@ struct fixed_point_scaling_impl {
{
using U = std::common_type_t<value_type_t<From>, value_type_t<To>>;
if constexpr (_is_integral(M)) {
return static_cast<To>(static_cast<value_type_t<From>>(value) * _get_value<U>(num));
return static_cast<To>(static_cast<value_type_t<From>>(value) * _get_value<U>(M));
} else if constexpr (_is_integral(_pow<-1>(M))) {
return static_cast<To>(static_cast<value_type_t<From>>(value) / _get_value<U>(den));
return static_cast<To>(static_cast<value_type_t<From>>(value) / _get_value<U>(_pow<-1>(M)));
} else {
return static_cast<To>(ratio<U>.scale(static_cast<value_type_t<From>>(value)));
}
Expand All @@ -161,7 +159,7 @@ struct fixed_point_scaling_traits {
static constexpr Rep scale_from(M, const From& value)
{
return fixed_point_scaling_impl<M{}>::template scale<Rep>(value);
};
}

template<Magnitude M>
static constexpr auto scale(M, const Rep& value)
Expand Down Expand Up @@ -201,6 +199,8 @@ concept HasScalingTraits = !std::convertible_to<decltype(select_scaling_traits<T

} // namespace detail

MP_UNITS_EXPORT_BEGIN
Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned before, we should not provide public API in .../bits/....


template<typename To, Magnitude M, typename From>
requires detail::HasScalingTraits<To, From> ||
requires(const From& value) { value.scale(std::type_identity<To>{}, M{}); }
Expand All @@ -226,4 +226,7 @@ constexpr auto scale(M scaling_factor, const From& value)
}
}


MP_UNITS_EXPORT_END

} // namespace mp_units
2 changes: 1 addition & 1 deletion src/core/include/mp-units/framework/customization_points.h
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ struct quantity_values {
* return an instance of @c Rep that approximates `scaling_factor * value`, another element of $\mathcal{V}$.
* This needs to be defined at least for `From = Rep`, as well as any other representation
* types for which interoperability is desired.
* - `template <typename To, Magnitude M> static constexpr auto scale(M scaling_factor, const Rep &value)`:
* - `template <Magnitude M> static constexpr auto scale(M scaling_factor, const Rep &value)`:
* Given an element of $\mathcal{V}$ represented by @c value and, a real number represented by @c scaling_factor,
* return an approximation of `scaling_factor * value`, another element of $\mathcal{V}$.
* Contrary to the `scale_from` case, here, the result representation is unspecified.
Expand Down
11 changes: 4 additions & 7 deletions test/runtime/fixed_point_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,24 +76,21 @@ std::vector<std::tuple<T...>> cartesian_product(const std::vector<T>&... src)


template<std::integral T>
using half_width_int_t = std::conditional_t<std::is_signed_v<T>, min_width_int_t<integer_rep_width_v<T> / 2>,
min_width_uint_t<integer_rep_width_v<T> / 2>>;
template<std::integral T>
using double_width_int_t = std::conditional_t<std::is_signed_v<T>, min_width_int_t<integer_rep_width_v<T> * 2>,
min_width_uint_t<integer_rep_width_v<T> * 2>>;
using half_width_int_for_t = std::conditional_t<std::is_signed_v<T>, min_width_int_t<integer_rep_width_v<T> / 2>,
min_width_uint_t<integer_rep_width_v<T> / 2>>;

template<std::integral Hi, std::unsigned_integral Lo>
requires(integer_rep_width_v<Hi> == integer_rep_width_v<Lo>)
auto combine_bits(Hi hi, Lo lo)
{
using ret_t = double_width_int_t<Hi>;
using ret_t = double_width_int_for_t<Hi>;
return (static_cast<ret_t>(hi) << integer_rep_width_v<Lo>)+static_cast<ret_t>(lo);
}

template<std::integral T, typename V>
void check(double_width_int<T> value, V&& visitor)
{
using DT = double_width_int_t<T>;
using DT = double_width_int_for_t<T>;
auto as_standard_int = static_cast<DT>(value);
auto expected = visitor(as_standard_int);
auto actual = visitor(value);
Expand Down
1 change: 1 addition & 0 deletions test/static/custom_rep_test_min_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ struct std::common_type<min_impl<T>, U> : std::type_identity<min_impl<std::commo
template<typename U, typename T>
struct std::common_type<U, min_impl<T>> : std::type_identity<min_impl<std::common_type_t<T, U>>> {};


namespace {

using namespace mp_units;
Expand Down
Loading