[PATCH] MS ABI: Implement /volatile:ms

[Dmitry Vyukov]

================
Comment at: lib/CodeGen/CGAtomic.cpp:1011
@@ +1010,3 @@
+  AtomicInfo AI(*this, LV);
+  return CGM.getCodeGenOpts().MSVolatile && !AI.shouldUseLibcall() &&
+         (LV.isVolatile() || hasVolatileMember(LV.getType()));
----------------
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?

================
Comment at: lib/CodeGen/CGAtomic.cpp:1017
@@ +1016,3 @@
+  return CGM.getCodeGenOpts().MSVolatile &&
+         getContext().getTargetInfo().hasBuiltinAtomic(
+             getContext().getTypeSize(Ty), getContext().getTypeAlign(Ty));
----------------
Same here. How presence of builtin atomic affects this?

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

================
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,
----------------
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.

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

http://reviews.llvm.org/D7580

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