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 😆
| 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
| 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.
Would it be posisble to add a bench for u256? Similar to
compiler-builtins/libm-test/benches/icount.rs
Lines 114 to 144 in 9c176c2
| // 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?
| debug_assert!(false, "unsafe preconditions not met"); | ||
| unsafe { core::hint::unreachable_unchecked() } |
There was a problem hiding this comment.
Mind either adding a debug_assert to the other unreachable_unchecked cases or removing the one here, just to be consistent? Technically unreachable_unchecked has one that works correctly nowadays, but having an explicit case is still nice imo.
| #[test] | ||
| fn inverse_mul() { | ||
| for x in 0..=u8::MAX { | ||
| for y in 1..=u8::MAX { | ||
| let xy = x.widen_mul(y); | ||
| assert_eq!(xy.checked_narrowing_div_rem(y), Some((x, 0))); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe also check the x.carrying_mul(y, 1) case?
| /// | ||
| /// # 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
| @@ -0,0 +1,163 @@ | |||
| use crate::support::{DInt, HInt, Int, MinInt, u256}; | |||
There was a problem hiding this comment.
If this is something you authored, could you add /* SPDX-License-Identifier: MIT OR Apache-2.0 */?
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