fix: support linux/arm64 builds - #1090
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restricts the compilation of x86-specific SIMD pybind modules (SSE, AVX2, AVX512) to x86 architectures in CMakeLists.txt and updates the instruction detection preprocessor checks in decide.cpp. Feedback on the changes points out that the updated preprocessor check using defined(_WIN32) will incorrectly evaluate to true on Windows ARM64, where the x86-specific cpuid intrinsic is unsupported, and suggests restricting the Windows check to x86/x64 architectures to prevent compilation failures.
|
I /think/ the failures are unrelated to the changes I've made here, but let me know if they should be looked into 🫡 For context this change is useful for me to be able to support multi-arch docker images for qsim across arm/amd, the support for macos seems to prove out that the CPU is OK, this resulted from me looking into what would be needed to close the gap. No urgency on my part so if there are requested changes/improvements I'm all ears! |
|
@mhucka does this change make sense to you? It would help my container using mac colleagues greatly so I'd love to see it included, happy to iterate however makes sense :) |
The previous directives supported macos arm but did not correctly trigger off of macosx, this applies the same codepaths for linux arm, supporting them natively.
a52aed3 to
343feee
Compare
|
@Cynocracy Thank you for this work, and sorry for the late reply. Yes, that failure on Windows is something new that's showing up in other CI failures. Something in the GitHub Windows environment (or maybe a third-party dependency brought in by qsim) must have changed. I need to dig in and fix the root cause. |
|
No worries at all! Thanks for letting me know :) If I can be of assistance, happy to take a stab at debugging. |
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
A trivial change suggestion; otherwise, looking good.
The CI failure on Windows should be fixed now with a PR merged today. When you make changes and update the branch on this PR, it should pass. |
github-actions
Bot
added
size: XS
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thank you for your work on this!
|
Thank you for the reviews and feedback! :) Excited to try this change out |
|
If you have access to AMD hardware and have additional improvements for qsim, we'd be happy to take them. |
2 participants
The previous directives supported macos arm but did not correctly trigger off of macosx, this applies the same codepaths for linux arm, supporting them natively.