The /volatile:ms semantics turn volatile loads and stores into atomic
acquire and release operations. This distinction is important because
volatile memory operations do not form a happens-before relationship
with non-atomic memory. This means that a volatile store is not
sufficient for implementing a mutex unlock routine.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/CodeGen/CGStmtOpenMP.cpp | ||
---|---|---|
836 ↗ | (On Diff #19811) | I'd say that if IsSeqCst is false, Atomic Ordering must be Monotonic rather than Unordered.
memory_order_relaxed atomic operation in C++11/C11.".
|
lib/CodeGen/CGAtomic.cpp | ||
---|---|---|
1011 ↗ | (On Diff #19811) | I don't get the !AI.shouldUseLibcall() part. |
1017 ↗ | (On Diff #19811) | Same here. How presence of builtin atomic affects this? |
1018 ↗ | (On Diff #19811) | In volatileIsAtomic you check that the value is volatile, but here you don't. Please make then consistent. Either check in both or don't check in both. |
lib/CodeGen/CGExpr.cpp | ||
1141 ↗ | (On Diff #19811) | Won't this be overly fragile? |
lib/CodeGen/CodeGenFunction.h | ||
2159 ↗ | (On Diff #19811) | Here you use IsSeqCst, but in EmitAtomicStore you use AO. |
lib/CodeGen/CGAtomic.cpp | ||
---|---|---|
1011 ↗ | (On Diff #19811) | shouldUseLibcall means that than atomic operation is not suitably aligned or sized to be lowered to a load atomic or store atomic instruction. These conditions are identical to the conditions that MSVC uses to say that a volatile load or store will also have acquire or release semantics. |
1017 ↗ | (On Diff #19811) | As I mentioned earlier, not all volatile loads or stores will turn into acquire or release loads and stores. |
1018 ↗ | (On Diff #19811) | I can't check it here because the type being passed in might not be qualified with volatile, it might be a record with a volatile field. I'd rather not make everyone who calls the other overload to have to manually check LV.isVolatile() || hasVolatileMember(LV.getType()). I'll just rename one of these to make it clear what it does. |
lib/CodeGen/CGExpr.cpp | ||
1141 ↗ | (On Diff #19811) | I don't think that is semantically correct. isAtomicType() is used for types which are "qualified" with _Atomic. Moreover, we would still need to be able to determine the different from a real _Atomic qualified type and a MS volatile type in order to determine the memory operation's order. |
lib/CodeGen/CGStmtOpenMP.cpp | ||
836 ↗ | (On Diff #19811) | Good catch, will do. |
lib/CodeGen/CodeGenFunction.h | ||
2159 ↗ | (On Diff #19811) | Sure, will do. |
lib/CodeGen/CGAtomic.cpp | ||
---|---|---|
1011 ↗ | (On Diff #19811) | Doesn't shouldUseLibcall also mean that the value is too fat for backend codegen, so just call a library functions (e.g. a 64-bit store on a 32-bit platform)? That's what I figure out from the name. Perhaps deserves a comment. |
Looks good to me. Feel free to gather other feedback.
lib/CodeGen/CGAtomic.cpp | ||
---|---|---|
1017 ↗ | (On Diff #19848) | Early return false if !CGOpts.MSVolatile before checking struct types for volatile members (not O(1))? |
1023 ↗ | (On Diff #19848) | Make the leading letter lowercase? "LValueIsSuitable..." starts with an initialism, so it stays upper. |
lib/CodeGen/CodeGenFunction.h | ||
2152–2153 ↗ | (On Diff #19848) | Big enough to make not inline? |
2171 ↗ | (On Diff #19848) | ditto |
I just looked at the flag stuff since others seem to have looked at the rest.
include/clang/Driver/CLCompatOptions.td | ||
---|---|---|
206 ↗ | (On Diff #19848) | nit: I'd skip "C++": just "standard semantics" is shorter (and more accurate in the C case I guess) |
208 ↗ | (On Diff #19848) | nit: there's no "will" in the :iso flag description. I'd leave it out here too. Would it be less accurate to just say that loads and stores are atomic? |
test/Driver/cl-options.c | ||
126 ↗ | (On Diff #19848) | maybe add a test without /volatile and check that -fms-volatile doesn't get passed by default? |
include/clang/Driver/CLCompatOptions.td | ||
---|---|---|
206 ↗ | (On Diff #19848) | Done. |
208 ↗ | (On Diff #19848) | Done. |
lib/CodeGen/CGAtomic.cpp | ||
1017 ↗ | (On Diff #19848) | Sema has already figured it out so it is cheap to query. |
1023 ↗ | (On Diff #19848) | Done. |
lib/CodeGen/CodeGenFunction.h | ||
2152–2153 ↗ | (On Diff #19848) | Done. |
2171 ↗ | (On Diff #19848) | Done. |
test/Driver/cl-options.c | ||
126 ↗ | (On Diff #19848) | -fms-volatile is passed by default depending on whether or not you are targeting x86. |