Make SSE inclusion conditional for target features #526
Conversation
| config.flag_if_supported("-msse4.2"); | ||
| let target_feature = env::var("CARGO_CFG_TARGET_FEATURE").unwrap(); | ||
| let target_features: Vec<_> = target_feature.split(",").collect(); | ||
| if target_features.contains(&"sse2") { |
There was a problem hiding this comment.
Does it change the behavior to not enable sse2 etc by default? If that's the case, CHANGELOG.md should be updated accordingly so developers are aware about the performance degradation once they upgrade the rocksdb crate.
There was a problem hiding this comment.
I believe SSE2 is in the set of default features for x86-64, at least it is for me if I run
# rustc --print=cfg
debug_assertions
target_arch="x86_64"
target_endian="little"
target_env="gnu"
target_family="unix"
target_feature="fxsr"
target_feature="sse"
target_feature="sse2"
target_os="linux"
target_pointer_width="64"
target_vendor="unknown"
unixIt is, however, a change in behavior for SSE4.2 etc. since those are not always enable by default. The user should of course use -C target-cpu=<native-or-other-appropriate-arch> to make sure they get accelerated/compatible binaries.
Should we amend the changelog regarding SSE4.2 et al? Do you have a suggestion for the wording?
There was a problem hiding this comment.
Maybe something like "RocksDB is not compiled with SSE4 instructions anymore unless the corresponding features are enabled in rustc"?
| config.flag_if_supported("-msse4.1"); | ||
| config.flag_if_supported("-msse4.2"); | ||
| let target_feature = env::var("CARGO_CFG_TARGET_FEATURE").unwrap(); | ||
| let target_features: Vec<_> = target_feature.split(",").collect(); |
There was a problem hiding this comment.
| let target_features: Vec<_> = target_feature.split(",").collect(); | |
| let target_features: Vec<_> = target_feature.split(',').collect(); |
Of course, it's not critical here. But I think the better way to use char rather than &str as a pattern when you use one character for splitting.
- SSE instructions are used according to the target CPU specifications - via rust-rocksdb/rust-rocksdb#526 - fixes bioconda/bioconda-recipes#28719
- SSE instructions are used according to the target CPU specifications - via rust-rocksdb/rust-rocksdb#526 - fixes bioconda/bioconda-recipes#28719
Since
0.12.3/gh-295 the underlying library gets compiled with SSE4.1/SSE4.2/PCLMUL instructions unconditionally.This prevents building the library for backwards compatible binaries, e.g., for
target-cpu=noconayou'd want a binary without SSE4 instructions.With the changes proposed here, one still can freely build for
target-cpu=nehalem(ortarget-cpu=westmereto get PCLMUL), or justtarget-cpu=nativegiven a recent enough machine and for local deployment. And additionallytarget-cpu=noconawould yield the desired compatible build.(Context here is that
rust-rocksdbis used by some programs we distribute as binaries for Bioconda and currently/usually target older CPU architectures there.)