[PATCH] MS ABI: Implement /volatile:ms

[David Majnemer]

================
Comment at: include/clang/Driver/CLCompatOptions.td:206
@@ +205,3 @@
+def _SLASH_volatile_iso : Option<["/", "-"], "volatile:iso", KIND_FLAG>, Group<_SLASH_volatile_Group>,
+  Flags<[CLOption, DriverOption]>, HelpText<"Volatile loads and stores have standard C++ semantics">;
+def _SLASH_volatile_ms  : Option<["/", "-"], "volatile:ms", KIND_FLAG>, Group<_SLASH_volatile_Group>,
----------------
hans wrote:
> nit: I'd skip "C++": just "standard semantics" is shorter (and more accurate in the C case I guess)
Done.

================
Comment at: include/clang/Driver/CLCompatOptions.td:208
@@ -205,1 +207,3 @@
+def _SLASH_volatile_ms  : Option<["/", "-"], "volatile:ms", KIND_FLAG>, Group<_SLASH_volatile_Group>,
+  Flags<[CLOption, DriverOption]>, HelpText<"Volatile loads and stores will have acquire and release semantics">;
 
----------------
hans wrote:
> 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?
Done.

================
Comment at: lib/CodeGen/CGAtomic.cpp:1017
@@ +1016,3 @@
+  bool AtomicIsInline = !AI.shouldUseLibcall();
+  return CGM.getCodeGenOpts().MSVolatile && IsVolatile && AtomicIsInline;
+}
----------------
rnk wrote:
> Early return false if !CGOpts.MSVolatile before checking struct types for volatile members (not O(1))?
Sema has already figured it out so it is cheap to query.

================
Comment at: lib/CodeGen/CGAtomic.cpp:1023
@@ +1022,3 @@
+/// performing such an operation can be performed without a libcall.
+bool CodeGenFunction::TypeIsSuitableForInlineAtomic(QualType Ty,
+                                                    bool IsVolatile) const {
----------------
rnk wrote:
> Make the leading letter lowercase? "LValueIsSuitable..." starts with an initialism, so it stays upper.
Done.

================
Comment at: lib/CodeGen/CodeGenFunction.h:2152-2153
@@ +2151,4 @@
+
+  RValue EmitAtomicLoad(LValue LV, SourceLocation SL,
+                        AggValueSlot Slot = AggValueSlot::ignored()) {
+    llvm::AtomicOrdering AO;
----------------
rnk wrote:
> Big enough to make not inline?
Done.

================
Comment at: lib/CodeGen/CodeGenFunction.h:2171
@@ +2170,3 @@
+                       bool IsVolatile, bool isInit);
+  void EmitAtomicStore(RValue rvalue, LValue lvalue, bool isInit) {
+    bool IsVolatile = lvalue.isVolatileQualified();
----------------
rnk wrote:
> ditto
Done.

================
Comment at: test/Driver/cl-options.c:126
@@ -125,1 +125,3 @@
 
+// RUN: %clang_cl /volatile:iso -### -- %s 2>&1 | FileCheck -check-prefix=VOLATILE-ISO %s
+// VOLATILE-ISO-NOT: "-fms-volatile"
----------------
hans wrote:
> maybe add a test without /volatile and check that -fms-volatile doesn't get passed by default?
-fms-volatile is passed by default depending on whether or not you are targeting x86.

http://reviews.llvm.org/D7580

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