alamb commented on code in PR #11322:
URL: https://github.com/apache/arrow-rs/pull/11322#discussion_r4233243564
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -64,6 +67,29 @@ pub fn compress(value: u64, mask: u64) -> u64 {
#[cfg(not(all(target_arch = "x86_64", target_feature = "bmi2")))]
{
+ compress_portable(value, mask)
Review Comment:
Thank you -- this is a nice way to keep this clean
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -51,8 +51,11 @@ use crate::bit_chunk_iterator::BitChunks;
/// assert_eq!(compress(0b1011_0100, 0b0110_1101), 0b0000_1010);
/// ```
//
-// Replace with `value.compress(mask)` when `uint_gather_scatter_bits` is
-// stabilised: <https://github.com/rust-lang/rust/issues/149069>
+// When `uint_gather_scatter_bits` is stabilised
+// (<https://github.com/rust-lang/rust/issues/149069>), measure
+// `value.compress(mask)` against this before switching: its portable
+// version is the constant-time network, which the fallback here beats on
Review Comment:
I am not sure what a constant time network is. Maybe you could link it here?
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -26,7 +26,7 @@ use crate::bit_chunk_iterator::BitChunks;
/// Equivalent to the x86 BMI2 `PEXT` instruction. When compiled with the
/// `bmi2` target feature enabled (for example `-C target-cpu=x86-64-v3`)
/// this lowers to the hardware `pext` instruction; otherwise it falls back
-/// to a portable scalar loop.
+/// to a portable version that picks its method by the mask's popcount.
Review Comment:
`popcount `is somewhat architectural specific -- I recommend something like
"picks its method by the number of set bits in the mask" as I think that would
be clearer
--
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]
