-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
<atomic> refactoring to support C11 #2846
<atomic> refactoring to support C11 #2846
Conversation
…atomic> (they are now in xatomic.h)
…te the C++ version of _Check_memory_order.
This comment was marked as resolved.
This comment was marked as resolved.
When did "respect debug codegen" rule became stronger than "avoid control flow in macros"? Also the change looks somewhere on the way to nowhere, because I hope that #83 can be resolved someday, and macroizing will complicate the resolution. |
Discussed in Discord, apparently it is indeed the best solution we can have. |
To elaborate: The switch over memory order has an optimization barrier inside it, meaning codegen is bad even in release mode. Thus, we need to directly do that code without the switch for atomic stores with default seq_cst memory order. We could use a free function for this, but it would mean extra calls in debug mode (I really want a forceinline that works in debug mode). I'm pretty neutral on the maintainability of this, the macros are kinda nasty, but it does concentrate the logic for the "base" functions in one place. I see it as a maintainability toss-up. That said, if others feel strongly one way or the other, I'm fine with not merging this, it's not like this PR was wasted time or anything since the work of extracting everything needed to be done anyway, in order to appear in vcruntime. The reason I did this in the first place is that C11 atomics need these "base" atomic functions in vcruntime (as they are compiler specific: clang will do its own thing here). Because C11 atomics and C++ atomics must share the same ABI we need to make sure the implementations stay in sync. Normally I would just move things into vcruntime and have the STL use them, but that would be taking currently open-source code, some of which actually was written by the community (including @AlexGuteniev, If I remember correctly) and making it proprietary. I think this would be a bit rude, and it would also mean we could no longer see improvements to All that said: we can just as well merge this after vcruntime is already open source, and at that point the extracted code can actually be shared. I don't expect huge changes to atomics, and the real danger appears if/when we change the ABI of atomics. In the meantime, we could manually mirror any improvements from |
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Thanks for refactoring this code to avoid duplication and divergence! ☢️ 🎉 😸 |
This extracts functionality that will be common to both C++'s types and C11's atomic operations to the top of
<atomic>
with a note about mirroring any changes to vcruntime.Except for the changed
_Check_memory_order
function all extracted code is in macros, some of them are a bit cursed. Still, I attempted to preserve debug codegen, except for a tiny change in load(). In particular the optimization of skipping the big memory order switch when you call a method that always uses seq_cst memory order is preserved.A new enum was added for specifying memory order in this shared code, the stdatomic.h header will avoid defining the actual
memory_order
enum in c++ mode to avoid stepping on the definition in<atomic>
for pre-C++-20 modes. This required adding some casts to both atomic and memory.Store functions were somewhat simplified by the new macros, although they do border on preprocessor abuse. The macros are not safe to call with trailing
;
s, because msvc will generate code fordo {} while(0)
loops.