libm: define and implement trait NarrowingDiv for unsigned integer division
#1011
Conversation
quaternic
force-pushed
the
narrowing-div
branch
from
8671beb to
aa1fadc
Compare
| impl_narrowing_div_primitive!(u32); | ||
| impl_narrowing_div_primitive!(u64); | ||
| impl_narrowing_div_primitive!(u128); | ||
| impl_narrowing_div_recurse!(u256); |
There was a problem hiding this comment.
Since this one is only used once, it can probably just be an impl without the macro. Please no f256 😆
There was a problem hiding this comment.
The macro was convenient for more thorough testing by using the recursive construction all the way from u16, and I'd want to do that for any future changes to it.
| unsafe fn unchecked_narrowing_div_rem(self, n: Self::H) -> (Self::H, Self::H); | ||
|
|
||
| /// Returns `Some((self / n, self % n))` when `self.hi() < n`. | ||
| fn checked_narrowing_div_rem(self, n: Self::H) -> Option<(Self::H, Self::H)> { |
There was a problem hiding this comment.
Nit: from std's convention, this can just be narrowing_div_rem
There was a problem hiding this comment.
Which convention? Option-returning methods on e.g. u32 all seem to be named checked_*
| if self.hi() >= n { | ||
| unsafe { core::hint::unreachable_unchecked() } | ||
| } | ||
| ((self / n as $D) as Self::H, (self % n as $D) as Self::H) |
There was a problem hiding this comment.
Could this use the DInt/HInt traits? Bit more clear than as.
(self / n.widen()).lo(), (self % n.widen()).lo()
It would be good to add a note that we're not doing anything special here since it optimizes well for primitives
There was a problem hiding this comment.
With that, I think this could even be a trait impl impl<D> NarrowingDiv for D where D: ops::Div + ops::Rem. The macro isn't too bad, though
There was a problem hiding this comment.
I'll change them to use (self / n.widen()).cast().
It would be good to add a note that we're not doing anything special here since it optimizes well for primitives
They actually don't optimize as well as they could (at least on x86):
https://rust.godbolt.org/z/q5e8vc3nv
... but that's a separate issue to look into. (LLVM: llvm/llvm-project#115158)
Anyway, it's good enough IMO, and I'll add a note on the macro.
| // Normalize the divisor by shifting the most significant one | ||
| // to the leading position. `n != 0` is implied by `self.hi() < n` | ||
| let lz = n.leading_zeros(); | ||
| let a = self << lz; | ||
| let b = n << lz; |
There was a problem hiding this comment.
Does it make any sense to check if a.hi() == 0 and do a normal div in a smaller type in that case?
There was a problem hiding this comment.
Yes, that could make sense, and it should be even better to check for self.hi() == 0 before normalizing the divisor. However, I don't think that case would be hit very often. I also like how the current construction only depends on NarrowingDiv for the smaller type.
| /// | ||
| /// # Safety | ||
| /// Requires that `n.leading_zeros() == 0` and `a < n`. | ||
| unsafe fn div_three_digits_by_two<U>(a0: U, a: U::D, n: U::D) -> (U, U::D) |
There was a problem hiding this comment.
Maybe div_three_3x_by_2x or so? Just thinking that the inputs aren't really digits
There was a problem hiding this comment.
They are digits in base R = 2.pow(U::BITS), and this function does division of a 3-digit number by a 2-digit number in that base. Admittedly the signature is rather weird, passing the "high two thirds" of the 3-digit number as one U::D and the least significant third as a U.
New utility in
libm::support:trait NarrowingDivfor dividingu2N / uNwhen the quotient fits inuNu256 / u128This is the inverse operation of unsigned widening multiplication:
The trait API is based on x86's
div-instruction: quotient overflow happens exactly when the high half of the dividend is greater or equal to the divisor, which includes division by zero.Split from #1002