You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/06/09 22:39:01 UTC
[GitHub] [arrow-rs] jhorstmann opened a new pull request, #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
jhorstmann opened a new pull request, #1830:
URL: https://github.com/apache/arrow-rs/pull/1830
# Which issue does this PR close?
<!---
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #1829.
# Rationale for this change
The autovectorized implementation is actually faster, allowing us to simplify the buffer code. Benchmark results are in the linked issue.
<!---
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!---
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
Removal of the `avx512` feature could be a breaking change if someone had it enabled.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] nevi-me merged pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
nevi-me merged PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] nevi-me commented on pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
nevi-me commented on PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830#issuecomment-1153213656
> Add notes about target-cpu to README
> Would someone be able to also benchmark this on ARM, for example on an Apple M1 or AWS graviton, to avoid any regressions there?
>
> I'm assuming that someone who cares about performance enough to enable the `simd` feature would also target a specific cpu baseline. But I would suggest to get some more feedback before merging.
I'll run a benchmark and report back in an hour to two
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830#issuecomment-1151694569
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1830?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1830](https://codecov.io/gh/apache/arrow-rs/pull/1830?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8e28308) into [master](https://codecov.io/gh/apache/arrow-rs/commit/db41b339f99efe3b4df766a7c102e58e3ac736d1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db41b33) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #1830 +/- ##
=======================================
Coverage 83.53% 83.54%
=======================================
Files 200 200
Lines 56798 56798
=======================================
+ Hits 47449 47450 +1
+ Misses 9349 9348 -1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/buffer/ops.rs](https://codecov.io/gh/apache/arrow-rs/pull/1830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2J1ZmZlci9vcHMucnM=) | `96.77% <100.00%> (ø)` | |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.46% <0.00%> (-0.20%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `65.98% <0.00%> (+0.45%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1830?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1830?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [db41b33...8e28308](https://codecov.io/gh/apache/arrow-rs/pull/1830?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830#issuecomment-1152589372
One question. Because the RUSTFLAGS="-Ctarget-cpu=xxx" is for optimizing on specific CPU model. But for existing simd feature, we don't need to specify CPU. Compared with existing simd feature, is the RUSTFLAGS less general?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jhorstmann commented on pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830#issuecomment-1153143778
Would someone be able to also benchmark this on ARM, for example on an Apple M1 or AWS graviton, to avoid any regressions there?
I'm assuming that someone who cares about performance enough to enable the `simd` feature would also target a specific cpu baseline. But I would suggest to get some more feedback before merging.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jhorstmann commented on pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830#issuecomment-1152651450
Thanks for reproducing the results! It's interesting that you also seem to get the effect of the second benchmark function being slower. I don't think it's downclocking related, on my machine the frequency looked constant and cascade lake shouldn't downclock either. Pinning the process to one core also did not make a difference. But I assume this is a effect that will only be visible in a microbenchmark and not have an effect on larger programs
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830#issuecomment-1152112922
I plan to run these benchmarks later today on a server-class machine and confirm there is no performance delta
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830#issuecomment-1152520577
On a Intel Cascade Lake Xeon(R) CPU @ 3.10GHz, specifically a GCP c2-standard-16
### Nightly with defaults
```
buffer_bit_ops and time: [419.91 ns 420.01 ns 420.12 ns]
buffer_bit_ops or time: [550.34 ns 550.43 ns 550.54 ns]
```
### Nightly with simd
```
buffer_bit_ops and time: [252.84 ns 253.71 ns 254.75 ns]
buffer_bit_ops or time: [276.70 ns 276.77 ns 276.85 ns]
```
### Nightly with avx512
```
buffer_bit_ops and time: [338.87 ns 339.09 ns 339.37 ns]
buffer_bit_ops or time: [365.72 ns 365.78 ns 365.86 ns]
```
### Nightly with defaults and RUSTFLAGS="-Ctarget-cpu=native"
```
buffer_bit_ops and time: [177.27 ns 177.32 ns 177.38 ns]
buffer_bit_ops or time: [290.42 ns 290.47 ns 290.52 ns]
```
### Nightly with simd and RUSTFLAGS="-Ctarget-cpu=native"
```
buffer_bit_ops and time: [199.39 ns 199.42 ns 199.45 ns]
buffer_bit_ops or time: [227.88 ns 227.93 ns 227.98 ns]
```
### Nightly with avx512 and RUSTFLAGS="-Ctarget-cpu=native"
```
buffer_bit_ops and time: [199.58 ns 199.64 ns 199.73 ns]
buffer_bit_ops or time: [229.27 ns 229.30 ns 229.34 ns]
```
### Nightly with defaults and RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=-prefer-256-bit"
```
buffer_bit_ops and time: [166.14 ns 166.19 ns 166.26 ns]
buffer_bit_ops or time: [208.24 ns 208.30 ns 208.36 ns]
```
### Nightly with simd and RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=-prefer-256-bit"
```
buffer_bit_ops and time: [197.55 ns 197.58 ns 197.60 ns]
buffer_bit_ops or time: [223.72 ns 223.79 ns 223.86 ns]
```
### Nightly with avx512 and RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=-prefer-256-bit"
```
buffer_bit_ops and time: [200.34 ns 200.38 ns 200.41 ns]
buffer_bit_ops or time: [328.80 ns 328.84 ns 328.89 ns]
```
### Stable with defaults RUSTFLAGS="-Ctarget-cpu=native"
```
buffer_bit_ops and time: [178.72 ns 178.77 ns 178.82 ns]
buffer_bit_ops or time: [294.65 ns 294.69 ns 294.74 ns]
```
### Stable with defaults RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=-prefer-256-bit"
```
buffer_bit_ops and time: [176.34 ns 176.82 ns 177.45 ns]
buffer_bit_ops or time: [200.99 ns 201.08 ns 201.17 ns]
```
# Conclusion
* simd feature is always faster than avx512 feature
* With `target-cpu=native` the LLVM generated `buffer_bit_ops` is faster than the simd version
* With `target-feature=-prefer-256-bit` the LLVM generated code is better than either of the hand-rolled loops
* Performance between stable and nightly is very similar
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jhorstmann commented on pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830#issuecomment-1152660580
> One question. Because the RUSTFLAGS="-Ctarget-cpu=xxx" is for optimizing on specific CPU model. But for existing simd feature, we don't need to specify CPU. Compared with existing simd feature, is the RUSTFLAGS less general?
The simd feature also benefits from specifying the `target-cpu`. Without `target-cpu`, the compiler has to produce code for a common baseline. For x86_64, that baseline includes the SSE2 instructions, which work on 128bit vectors, but misses many useful features added later. I'm guessing here the compiler decides that it's not worthwhile to use sse2 instructions, but it's heuristic seems wrong here.
There probably is some additional compiler flag that would make the standard kernel as fast as the simd kernel also for the default target. We probably can't expect everyone to set several flags though.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] nevi-me commented on pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
nevi-me commented on PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830#issuecomment-1153235877
M1 Pro
* `nosimd` = nightly master with `RUSTFLAGS="-Ctarget-cpu=native"`
* `simd` = nightly master with above and `--features simd`
* `pr1830` = nightly on this PR with the above
* `stable` = stable on this PR with `target-cpu=native` (didn't check master as there should be no difference)
* `stablenoflags` = stable on this PR with no flags
```
group nosimd pr1830 simd stable stablenoflags
----- ------ ------ ---- ------ -------------
buffer_binary_ops/and 1.01 153.2±0.97ns 93.4 GB/sec 1.03 157.6±8.65ns 90.8 GB/sec 1.25 189.8±1.68ns 75.4 GB/sec 1.03 156.6±21.14ns 91.4 GB/sec 1.00 152.4±1.84ns 93.9 GB/sec
buffer_binary_ops/and_with_offset 1.02 536.2±4.21ns 26.7 GB/sec 1.02 533.9±2.03ns 26.8 GB/sec 1.03 541.7±4.97ns 26.4 GB/sec 1.00 524.5±2.23ns 27.3 GB/sec 1.01 527.3±1.98ns 27.1 GB/sec
buffer_binary_ops/or 1.04 156.3±7.65ns 91.5 GB/sec 1.02 154.2±0.76ns 92.8 GB/sec 1.31 197.4±47.84ns 72.5 GB/sec 1.00 150.7±2.41ns 94.9 GB/sec 1.01 151.5±1.14ns 94.4 GB/sec
buffer_binary_ops/or_with_offset 1.02 539.7±4.22ns 26.5 GB/sec 1.02 539.1±2.84ns 26.5 GB/sec 1.04 549.1±4.72ns 26.1 GB/sec 1.00 530.2±3.16ns 27.0 GB/sec 1.01 532.8±6.84ns 26.8 GB/sec
buffer_unary_ops/not 1.17 197.0±43.99ns 48.4 GB/sec 1.09 183.2±3.57ns 52.1 GB/sec 1.00 168.7±3.64ns 56.5 GB/sec 1.08 182.1±4.18ns 52.4 GB/sec 1.08 181.7±3.15ns 52.5 GB/sec
buffer_unary_ops/not_with_offset 1.00 363.8±6.18ns 26.2 GB/sec 1.02 369.1±2.30ns 25.8 GB/sec 1.02 368.6±1.09ns 25.9 GB/sec 1.02 368.8±23.20ns 25.9 GB/sec 1.00 362.6±1.81ns 26.3 GB/sec
```
A bit hard to interpret the results, but what I can see is that stable with a CPU target flag isn't much that better than without. That could make sense as there shouldn't be (m)any differences between ARMv8 CPUs that are supported, unlike x64 where there's all sorts of SIMD versions.
Of the 6 results, 5 are fastest in stable, but not by a large variation compared to the other options, unlike what we saw with x64 on @tustvold 's results.
Seeing as there's no regression, I'll merge this. Thanks @jhorstmann!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jhorstmann commented on a diff in pull request #1830: Remove simd and avx512 bitwise kernels in favor of autovectorization
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on code in PR #1830:
URL: https://github.com/apache/arrow-rs/pull/1830#discussion_r898908332
##########
arrow/src/buffer/ops.rs:
##########
@@ -15,110 +15,8 @@
// specific language governing permissions and limitations
// under the License.
-#[cfg(feature = "simd")]
-use crate::util::bit_util;
-#[cfg(feature = "simd")]
-use packed_simd::u8x64;
-
-#[cfg(feature = "avx512")]
-use crate::arch::avx512::*;
-use crate::util::bit_util::ceil;
-#[cfg(any(feature = "simd", feature = "avx512"))]
-use std::borrow::BorrowMut;
-
use super::{Buffer, MutableBuffer};
-
-/// Apply a bitwise operation `simd_op` / `scalar_op` to two inputs using simd instructions and return the result as a Buffer.
-/// The `simd_op` functions gets applied on chunks of 64 bytes (512 bits) at a time
-/// and the `scalar_op` gets applied to remaining bytes.
-/// Contrary to the non-simd version `bitwise_bin_op_helper`, the offset and length is specified in bytes
-/// and this version does not support operations starting at arbitrary bit offsets.
-#[cfg(feature = "simd")]
-pub fn bitwise_bin_op_simd_helper<SI, SC>(
- left: &Buffer,
- left_offset: usize,
- right: &Buffer,
- right_offset: usize,
- len: usize,
- simd_op: SI,
- scalar_op: SC,
-) -> Buffer
-where
- SI: Fn(u8x64, u8x64) -> u8x64,
- SC: Fn(u8, u8) -> u8,
-{
- let mut result = MutableBuffer::new(len).with_bitset(len, false);
- let lanes = u8x64::lanes();
-
- let mut left_chunks = left.as_slice()[left_offset..].chunks_exact(lanes);
- let mut right_chunks = right.as_slice()[right_offset..].chunks_exact(lanes);
- let mut result_chunks = result.as_slice_mut().chunks_exact_mut(lanes);
-
- result_chunks
- .borrow_mut()
- .zip(left_chunks.borrow_mut().zip(right_chunks.borrow_mut()))
- .for_each(|(res, (left, right))| {
- unsafe { bit_util::bitwise_bin_op_simd(&left, &right, res, &simd_op) };
- });
-
- result_chunks
- .into_remainder()
- .iter_mut()
- .zip(
- left_chunks
- .remainder()
- .iter()
- .zip(right_chunks.remainder().iter()),
- )
- .for_each(|(res, (left, right))| {
- *res = scalar_op(*left, *right);
- });
-
- result.into()
-}
-
-/// Apply a bitwise operation `simd_op` / `scalar_op` to one input using simd instructions and return the result as a Buffer.
-/// The `simd_op` functions gets applied on chunks of 64 bytes (512 bits) at a time
-/// and the `scalar_op` gets applied to remaining bytes.
-/// Contrary to the non-simd version `bitwise_unary_op_helper`, the offset and length is specified in bytes
-/// and this version does not support operations starting at arbitrary bit offsets.
-#[cfg(feature = "simd")]
-pub fn bitwise_unary_op_simd_helper<SI, SC>(
- left: &Buffer,
- left_offset: usize,
- len: usize,
- simd_op: SI,
- scalar_op: SC,
-) -> Buffer
-where
- SI: Fn(u8x64) -> u8x64,
- SC: Fn(u8) -> u8,
-{
- let mut result = MutableBuffer::new(len).with_bitset(len, false);
- let lanes = u8x64::lanes();
-
- let mut left_chunks = left.as_slice()[left_offset..].chunks_exact(lanes);
Review Comment:
This was actually also buggy and should have been sliced using `[left_offset..left_offset+len]`. The effect was that if the buffer was larger then len the remainders would not line up.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org