Add patch to make SIMD instructions optional at runtime on Windows #93
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Because the CMake build was inadvertently lacking the intended feature, previously enabling SIMD instructions like SSE and AVX would require that those instructions always be present at runtime on Windows. This patch makes the compiler use those flags only on the appropriate source files so that the instructions are optional at runtime as intended.
…nda-forge-pinning 2023.04.06.12.28.58
ryanvolz
requested review from
alexbw,
egpbos,
grlee77,
jakirkham and
jschueller
as code owners
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
jschueller
approved these changes
Apr 7, 2023
|
FYI, the upstream PR has been merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
[ ] Reset the build number to0(if the version changed)conda-smithy(Use the phrase@conda-forge-admin, please rerenderin a comment in this PR for automated rerendering)Over at @conda-forge/gnuradio, I've been receiving reports for a little while from Windows users who experienced crashes whenever an FFT provided by the
fftwlibrary was required. At first I thought it was because the builds had AVX support enabled but not SSE2, resulting in my prior PR to fix that. But the reports of crashes have continued, and I think I finally have the issue fixed now with this PR.Basically, with CMake, enabling SSE2/AVX support would cause the entire library to be built with those SIMD flags, resulting in SIMD instructions being inserted in the library and not gated by the check for whether the runtime CPU supported them. So users without AVX support were seeing crashes because the library hard required AVX instructions. The fix is to patch CMakeLists.txt so that enabled SIMD flags are only applied to the specific files that are intended to use them, which is exactly what is done with configure/make.
I've put in a PR upstream to get the patch incorporated there, but I'm sure my suffering users would love a fixed conda-forge package in the meantime. I've tested this with a locally rebuilt package and the crash does indeed disappear (running in a Windows VM and using the Intel Software Development Emulator to disable AVX instructions), and I've asked for users with actual hardware to test as well.