-
Notifications
You must be signed in to change notification settings - Fork 78
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
implement value_cast<ToQ> and value_cast<ToQP> #571
base: master
Are you sure you want to change the base?
Conversation
5b6df8b
to
3bd4f4d
Compare
BTW, instead of |
77113cb
to
c51baae
Compare
…rigin in value_cast, to prevent overflow in more cases.
Given that it is somewhat nontrivial to implement a correct general |
Oh, shall I try to sqash? |
…ude_type_impl when both types are the same
|
||
Overloads are also provided for instances of `quantity_point`. Furthermore, in that case, there is | ||
an overload `value_cast<ToQP>(qp)`, which is roughly equivalent to | ||
`value_cast<typename ToQP::quantity_type>(qp).point_for(ToQP::point_origin)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value_cast<typename ToQP::quantity_type>(qp)
is still a bit controversial 😉
And even if not, then we first have to describe what it does and how to use it (e.g., QP::quantity_type
) before we use it in the description of value_cast<ToQP>(qp)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let me do another take on the documentation, this one here is indeed a bit short.
(std::remove_reference_t<FromQP>::unit != ToQP::unit)) | ||
[[nodiscard]] constexpr QuantityPoint auto sudo_cast(FromQP&& qp) | ||
{ | ||
using qp_type = std::remove_reference_t<FromQP>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are assuming here an integral representation type, which often will not be the case as double
is the default in the library. In cases of a floating point representation type, we should calculate the entire conversion factor in one step and then do only one multiplication operation on the value to get the result. Otherwise, we will not generate assembly equivalent to operations on double
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my previous comment was unclear, I will just note that the relative_point_origin
stores an offset, which has its own unit and representation as well. We could factor in this conversion here as well (instead of calling point_for()
as a separate step).
However, maybe it would be too complex to do it here anyway and the measurable performance gains would not be that big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, those considerations about order of the conversion operations are most relevant for integral types, where both underflow and overflow are likely. It doesn't hurt for double either. However, I'm not sure what you mean by "calculate the entire conversion factor in one step". A change of point-origin requires an addition. A change in unit requires a multiplication. This implementation here does at most one multiplication and at most one addition in all cases. Here is how:
- The initial check for same point-origin directly delegates to the
sudo_cast<Q>
implementation to select the "best" way to do that multiplication, and avoids the addition - The two code-paths for a change in point origin go as follos:
- The first path for where the unit gets smaller and thus the number gets larger:
- cast to the intermediate representation type using
sudo_cast
(no-op in the case of floatingpoints). previously, that was avalue_cast
, but I figured that within asudo_cast
,value_cast
is off limit. - add/subtract the change in reference using
.point_for
; hopefully, the implementation of.point_for
is such that this is a single addition and no multiplication - scale the unit/number and
- cast the to the final representation type using
sudo_cast
.
- cast to the intermediate representation type using
- The second path for where the unit gets larger and thus the number will get smaller (or stay the same):
- cast to the intermediate representation type and
- scale the unit/number using
sudo_cast
. If the intermediate representation type is the same as the source erpresentation and the units remain the same, this is ano_op
. - add/subtract the change in reference using
.point_for
- cast to the final representation type using
sudo_cast
. No-op for floating-point types.
- The first path for where the unit gets smaller and thus the number gets larger:
So I believe this implementation is as efficient as it can be through a careful selection of types. While both code-paths invoke sudo_cast
twice, there is always one where the source and target units match and no multiplication is needed. For floating-point, there is also not cast, so that cast is a no-op. Furthermore, the implementation here does never do any arithmetic itself - it delegates the addition to .point_for
and the multiplication to sudo_cast<Q>
. Personally, I would even prefer to call value_cast
instead, because in each of the two code-paths, one of the casts is a pure representation cast, which would be more clear as value_cast<rep>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, now I understand you are indicating that .point_for
may potentially change the unit and representation type? In that case, you are of course right, we should replace the .point_for
call here with a dedicated implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I'm not sure what you mean by "calculate the entire conversion factor in one step".
I mean something like this:
if constexpr (std::is_floating_point_v<multiplier_type>) {
// this results in great assembly
constexpr auto ratio = val(num) / val(den) * val(irr);
// use precalculated ratio...
}
else // ...
Precalculation of ratio
as a constexpr
variable ensures that the assembly will include only one multiplication by this constant. The else
branch typically results in more assembly instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@burnpanck, are you going to work on this here? Or should we rebase, merge, and then refactor in the next PR?
I just added a bit more detail to the documentation, opting for the analogy to the |
Sure, it is great that you have time to look into this in more detail. All of this is too much for me to address in detail at once by working alone. There are still plenty of things to do in the library. I am so happy when someone comes to help me. Thank you! |
Fixes #568