Closed deadalnix closed 1 month ago
Merging. Will release shortly.
@deadalnix
I have unmerged this for now.
I see that you have changed the algorithm. I do not want to do this unless I have a formal proof that the new algorithm is just as correct and much evidence that it is beneficial (faster, etc.).
Thanks for looking into this. I did that quite some time ago. Can you point me where the algorithm changed? I want to consider if this is an error or something legit.
@deadalnix Among other things, you replaced (product.low <= 1)
with (product.low == 0)
. Several other things have been changed. We must validate each one of these changes.
Ha yes. I'll go over this again, why I did this is not 100% clear to me a year later.
I see what the transformation is.
The original code flows as follow:
if (!shouldRoundUp) {
// Neutralize the rounding up.
}
// Round up if required.
The logic is changed as follow:
if (shouldRoundUp) {
// Round up.
}
// Do nothing.
When flipping the condition, (product.low <= 1)
becomes (product.low > 1)
(and the &&
becomes a ||
). So we are doing things differently when product.low == 1
. However, in this case, we know the lower bits of the mantissa are going to be zero, so adding 1 or not won't change the end result after answer.mantissa >>= 1
.
As a follow up to #216 . It's not branchless, but, nevertheless, allow to extract part of the logic so it can be reused.
Running the benchmark shows no difference in term of instruction count, branch and whatnot with the previous code.