mbutrovich commented on code in PR #12232:
URL: https://github.com/apache/arrow-rs/pull/12232#discussion_r4564693444
##########
arrow-buffer/src/divisor.rs:
##########
@@ -470,20 +510,390 @@ unsigned_divisor!(
u64,
u128
);
-signed_divisor!(
+/// Expands to the arguments of `signed_divisor!` that compute with `$wide`, a
type twice as wide
+/// as `$t`
+macro_rules! signed_divisor_wide {
+ ($name:ident, $power_of_two:ident, $add:ident, $t:ty, $wide:ty) => {
+ signed_divisor!(
+ $name,
+ $power_of_two,
+ $add,
+ $t,
+ [const],
+ // m = 1 + floor(2^(N + l - 1) / |divisor|) is between 2^(N - 1)
and 2^N, so m - 2^N
+ // fits in the narrow type
+ |magnitude, l| {
+ let m = 1 + (1 << (<$t>::BITS + l - 1)) / magnitude as $wide;
+ (m - (1 << <$t>::BITS)) as $t
+ },
+ // `n + MULSH(m', n)` in figure 5.1
+ |m, n| n + ((m as $wide * n as $wide) >> <$t>::BITS) as $t,
+ []
+ );
+ };
+}
+
+signed_divisor_wide!(
DivisorI32,
PowerOfTwoDivisorI32,
AddIndicatorDivisorI32,
i32,
i64
);
-signed_divisor!(
+signed_divisor_wide!(
DivisorI64,
PowerOfTwoDivisorI64,
AddIndicatorDivisorI64,
i64,
i128
);
+signed_divisor!(
+ DivisorI128,
+ PowerOfTwoDivisorI128,
+ AddIndicatorDivisorI128,
+ i128,
+ [],
Review Comment:
`DivisorI128::new` and `DivisorI256::new` aren't `const fn`, unlike the
32-bit and 64-bit divisors. The multiplier needs a 256-bit by 128-bit division
for `i128` and a 512-bit by 256-bit division for `i256`. This PR computes them
with
[`i256::wrapping_div`](https://github.com/apache/arrow-rs/blob/d48a64e5a70f8cb35bf55ba3e8ce1c9048e71db8/arrow-buffer/src/bigint/mod.rs#L537)
and with Algorithm D in
[`div_rem`](https://github.com/apache/arrow-rs/blob/d48a64e5a70f8cb35bf55ba3e8ce1c9048e71db8/arrow-buffer/src/bigint/div.rs#L16),
and neither is `const`.
`DivisorI128::new` could be `const` with a `const` division of two 128-bit
words by one, at about 30 lines. The constant divisor in #11421 that would gain
most from `const` is 10^38 for `Display for i256` (#12174), and that's an
`i256`, which would need a `const` Algorithm D. Making `new` a `const fn` later
doesn't break callers, so I left both non-`const` until a caller needs it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
