-
Notifications
You must be signed in to change notification settings - Fork 247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better support for mingw-w64 and GCC-compatible compilers #1212
Conversation
The _InterlockedCompareExchange128 intrinsic is missing in mingw-w64, so instead use the __sync_bool_compare_and_swap builtin in GCC-compatible compilers.
The constants _ARM_BARRIER_ISH and _ARM64_BARRIER_ISH are missing in mingw-w64. Use inline assembly to insert the instruction instead.
These intrinsics are missing in mingw-w64 and GCC-compatible compilers. The __iso_volatile_load intrinsics is intended to tell MSVC to use the ISO standard C++ non-atomic volatile load, instead of the MSVC extension which adds other guarantees. For GCC-compatible compilers, the ISO standard volatile load is already the default, so there is no need to use these intrinsics.
I'm not familiar with the GCC intrinsics. Does someone else want to take a stab at reviewing this? @DefaultRyan @oldnewthing @BenJKuhn |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't speak to the completeness of these changes wrt to making mingw work, but the changes themselves seem correct based on a close reading. The use of __iso_volatile_load64 for the MSVC implementation is to avoid a perf hit due to MS-specific behavior described here: https://learn.microsoft.com/en-us/cpp/build/reference/volatile-volatile-keyword-interpretation?view=msvc-170. It should be unnecessary in MinGW, though if it adds the same memory barriers MS does for volatile reads, it may incur a perf penalty. I couldn't find a quick answer to that. It'll be correct and usable though either way.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There are still some intrinsics and defines missing in mingw-w64 (see mstorsjo/llvm-mingw#307) which can all be replaced with other compatible constructions. Verified by compiling a sample program locally for i686, x86_64 and aarch64 (aarch64 build is untested because I don't have a compatible system).
(CI checks for mingw-w64 will be coming soon™)
CC: @mstorsjo @cristianadam