Only enable SIMD on targets which feature SSE #77
Conversation
mkroening
changed the title
There was a problem hiding this comment.
Hmmm... OK, so the high level problem here is that memchr is being compiled in an environment where SSE isn't supported, and that causes compilation to fail. I agree that that is a problem that we should fix, although I would really prefer that we get a reproduction. #57 had issues with that too. Is there anyway I can actually test this fix and maybe add it to CI?
As for the solution in this PR, I have two concerns:
- Is
CARGO_CFG_TARGET_FEATUREactually set even when not explicitly set by the end user? If not, then this approach would disable SIMD optimizations in the vast majority of cases and thus would be a non-starter. - Do we need to check for SSE2, SSE42 and AVX too?
|
Thanks for the quick reply! I added a simple custom target without SSE and a CI job, building the project for this target (requires Regarding your concerns:
|
|
This PR is on crates.io in |
|
Thanks a lot! |
Fixed in memchr 2.3.4 by BurntSushi/memchr#77.
|
@mkroening I ended up removing the CI test added in this PR. I did it because it was failing in CI. I did not understand the error message and I don't have the time to debug it. I think going forward, one of the following three things should happen:
|
This was added in #77 to catch regressions, but it has starting [failing on CI] in a way that I don't understand. Since this is a niche use case, I don't feel compelled to spend the time investigating. On #77, I suggested that we have one of three directions forward: 1. Support for testing this in CI needs to become more robust. 2. Its failure modes need to be better understandable by me. 3. We don't test it at all and instead rely on patches from stakeholders to fix things if they break. (This is the new status quo.) [failing on CI]: https://github.com/BurntSushi/memchr/actions/runs/8030404672/job/21937751272
The old CI test was removed (#77 (comment)) due to rust-lang/rust#116672 causing a mismatch of data layouts with the custom target (`src/tests/x86_64-soft_float.json`): ```console error: data-layout for target `x86_64-soft_float-10047705440633310713`, `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128`, differs from LLVM target's `x86_64-unknown-none` default layout, `e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128` ``` This PR removes the unused custom target and reintroduces the CI test with the now-available tier 2 target [`x86_64-unknown-none`](https://doc.rust-lang.org/nightly/rustc/platfor m-support/x86_64-unknown-none.html). This makes this test much more robust, since we don't have to update a custom target ourselves and can even use stable Rust now. PR #146
I just ran into issue #57, which is caused by compiling this crate for targets that do not support SSE instructions. This is common for operating system kernels.
I don't consume this crate directly but through a dependency. It would be nice to have this crate compile without having to manually specify
memchr_disable_auto_simd.This PR makes the build script check if the target supports SSE and disables SIMD optimizations if not.
What do you think?