[PATCH] MS ABI: Implement /volatile:ms

[David Majnemer]

================
Comment at: lib/CodeGen/CGAtomic.cpp:1011
@@ +1010,3 @@
+  AtomicInfo AI(*this, LV);
+  return CGM.getCodeGenOpts().MSVolatile && !AI.shouldUseLibcall() &&
+         (LV.isVolatile() || hasVolatileMember(LV.getType()));
----------------
dvyukov wrote:
> I don't get the !AI.shouldUseLibcall() part.
> If the atomic operation would use a libcall, then you don't turn volatile access into atomic? Why? How is it related to libcalls?
`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.

================
Comment at: lib/CodeGen/CGAtomic.cpp:1017
@@ +1016,3 @@
+  return CGM.getCodeGenOpts().MSVolatile &&
+         getContext().getTargetInfo().hasBuiltinAtomic(
+             getContext().getTypeSize(Ty), getContext().getTypeAlign(Ty));
----------------
dvyukov wrote:
> Same here. How presence of builtin atomic affects this?
As I mentioned earlier, not all volatile loads or stores will turn into acquire or release loads and stores.

================
Comment at: lib/CodeGen/CGAtomic.cpp:1018
@@ +1017,3 @@
+         getContext().getTargetInfo().hasBuiltinAtomic(
+             getContext().getTypeSize(Ty), getContext().getTypeAlign(Ty));
+}
----------------
dvyukov wrote:
> 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.
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.

================
Comment at: lib/CodeGen/CGExpr.cpp:1141
@@ -1138,3 +1140,3 @@
   // Atomic operations have to be done on integral types.
-  if (Ty->isAtomicType()) {
+  if (Ty->isAtomicType() || VolatileIsAtomic) {
     LValue lvalue = LValue::MakeAddr(Addr, Ty,
----------------
dvyukov wrote:
> Won't this be overly fragile?
> MS semantics effectively turn volatile variables into atomic variables. So the current isAtomicType becomes effectively useless, and whoever uses isAtomicType must also remember to add volatileIsAtomic. And there is nothing that reminds of that. Ty->isAtomicType() looks pretty much like what you need.
> It would be less fragile if you fix the isAtomic predicate itself.
> But I know very little about llvm internals, maybe it is not possible, or a bad idea for some other reason. I don't know.
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.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:836
@@ -834,1 +835,3 @@
+                                        IsSeqCst ? llvm::SequentiallyConsistent
+                                                 : llvm::Unordered);
   // OpenMP, 2.12.6, atomic Construct
----------------
ABataev wrote:
> I'd say that if IsSeqCst is false, Atomic Ordering must be Monotonic rather than Unordered.
> 1. According to OpenMP 4.0 "non-sequentially consistent atomic construct has the same semantics as a
> memory_order_relaxed atomic operation in C++11/C11.". 
> 2. According to http://llvm.org/docs/Atomics.html "Monotonic... This corresponds to the C++11/C11 memory_order_relaxed"
Good catch, will do.

================
Comment at: lib/CodeGen/CodeGenFunction.h:2159
@@ +2158,3 @@
+    } else {
+      IsSeqCst = false;
+      IsVolatile = true;
----------------
dvyukov wrote:
> Here you use IsSeqCst, but in EmitAtomicStore you use AO.
> Please make them consistent.
Sure, will do.

http://reviews.llvm.org/D7580

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/