[MS] Implement __iso_volatile loads/stores as builtins

[mstorsjo]

Summary

These are supposed to produce the same as normal volatile pointer loads/stores. When -volatile:ms is specified, normal volatile pointers are forced to have atomic semantics (as is the default on x86 in MSVC mode). In that case, these builtins should still produce non-atomic volatile loads/stores without acquire/release semantics, which the new test verifies.

These are only available on ARM (and on AArch64, although clang doesn't support AArch64/Windows yet).

This implements what is missing for PR30394, making it possible to compile C++ for ARM in MSVC mode with MSVC headers.

[majnemer]

IMO, this should be implemented in clang CodeGen so that we don't get extra acquire/release barriers with /volatile:ms but that might be overkill; feel free to disregard this.

[mstorsjo]

Changed to implement it as builtins, as requested. I had to put this at the bottom in EmitBuiltinExpr (which returns RValues) instead of in EmitARMBuiltinExpr (which returns Value*), since the returned Value* for stores is nullptr. A nullptr return value from EmitTargetBuiltinExpr indicates that it wasn't handled, this triggered errors about the store builtins being unsupported.

[majnemer]

lib/CodeGen/CGBuiltin.cpp

2597–2611

I think you could sink this into EmitARMBuiltinExpr. You don't really need or want the fancy CGF machinery here. I'd just return the result of CreateAlignedStore/CreateAlignedLoad which should handle the EmitTargetBuiltinExpr issue.

[mstorsjo]

Implemented using CreateAlignedStore and CreateAlignedLoad instead.

I'm less confident in what this actually does though - e.g. is the CreateBitCast part necessary at all? I just based this on code I found in the same file calling CreateAlignedStore. (If I comment out the CreateBitCast line, it still works just as fine, at least for the unit test - but what about actual use?)

Compared to the previous patch, the generated IR now lacks a !tbaa !3 marker at the end - is that an issue?

[majnemer]

LGTM

test/CodeGen/ms-volatile-arm.c

3

You don't need -fms-volatile.

[mstorsjo]

test/CodeGen/ms-volatile-arm.c

3

Well, originally, the point was to clarify that these volatile stores end up without atomic semantics, regardless of whether -volatile:ms has been specified. The original version of this patch (with an inline implementation in intrin.h, just using a normal volatile pointer) wouldn't pass this test, but would pass if I remove this option. But if you prefer, I can leave it out.

[majnemer]

test/CodeGen/ms-volatile-arm.c

3

Makes sense.

include/clang/Basic/BuiltinsARM.def

 LANGBUILTIN(__wfe, "v", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(__wfi, "v", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(__sev, "v", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(__sevl, "v", "", ALL_MS_LANGUAGES)
 
 LANGBUILTIN(__dmb, "vUi", "nc", ALL_MS_LANGUAGES)
 LANGBUILTIN(__dsb, "vUi", "nc", ALL_MS_LANGUAGES)
 LANGBUILTIN(__isb, "vUi", "nc", ALL_MS_LANGUAGES)
+LANGBUILTIN(__iso_volatile_load8,   "ccCD*",     "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(__iso_volatile_load16,  "ssCD*",     "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(__iso_volatile_load32,  "iiCD*",     "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(__iso_volatile_load64,  "LLiLLiCD*", "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(__iso_volatile_store8,  "vcD*c",     "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(__iso_volatile_store16, "vsD*s",     "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(__iso_volatile_store32, "viD*i",     "n", ALL_MS_LANGUAGES)
+LANGBUILTIN(__iso_volatile_store64, "vLLiD*LLi", "n", ALL_MS_LANGUAGES)
 LANGBUILTIN(__ldrexd, "WiWiCD*", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(_MoveFromCoprocessor, "UiIUiIUiIUiIUiIUi", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(_MoveFromCoprocessor2, "UiIUiIUiIUiIUiIUi", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(_MoveToCoprocessor, "vUiIUiIUiIUiIUiIUi", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(_MoveToCoprocessor2, "vUiIUiIUiIUiIUiIUi", "", ALL_MS_LANGUAGES)
 
 #undef BUILTIN
 #undef LANGBUILTIN

lib/CodeGen/CGBuiltin.cpp

 static Value *EmitNontemporalLoad(CodeGenFunction &CGF, const CallExpr *E) {
   Value *Address = CGF.EmitScalarExpr(E->getArg(0));
 
   LValue LV = CGF.MakeNaturalAlignAddrLValue(Address, E->getType());
   LV.setNontemporal(true);
   return CGF.EmitLoadOfScalar(LV, E->getExprLoc());
 }
 
+static Value *EmitVolatileStore(CodeGenFunction &CGF, const CallExpr *E) {
+  Value *Address = CGF.EmitScalarExpr(E->getArg(0));
+  Value *Val = CGF.EmitScalarExpr(E->getArg(1));
+
+  LValue LV = CGF.MakeNaturalAlignAddrLValue(Address, E->getArg(1)->getType());
+  // Call the full version of EmitStoreOfScalar directly, to override the volatile
+  // flag without actually setting volatile in the type. This avoids
+  // LValueIsSuitableForInlineAtomic picking it up and transforming it into an
+  // atomic store.
+  CGF.EmitStoreOfScalar(Val, LV.getAddress(), true, LV.getType(),
+                        LV.getAlignmentSource(), LV.getTBAAInfo(), false,
+                        LV.getTBAABaseType(), LV.getTBAAOffset(),
+                        LV.isNontemporal());
+  return nullptr;
+}
+
+static Value *EmitVolatileLoad(CodeGenFunction &CGF, const CallExpr *E) {
+  Value *Address = CGF.EmitScalarExpr(E->getArg(0));
+
+  LValue LV = CGF.MakeNaturalAlignAddrLValue(Address, E->getType());
+  // Call the full version of EmitLoadOfScalar directly, to override the volatile
+  // flag without actually setting volatile in the type. This avoids
+  // LValueIsSuitableForInlineAtomic picking it up and transforming it into an
+  // atomic load.
+  return CGF.EmitLoadOfScalar(LV.getAddress(), true, LV.getType(),
+                              E->getExprLoc(), LV.getAlignmentSource(),
+                              LV.getTBAAInfo(), LV.getTBAABaseType(),
+                              LV.getTBAAOffset(), LV.isNontemporal());
+}
+
 static RValue EmitBinaryAtomic(CodeGenFunction &CGF,
                                llvm::AtomicRMWInst::BinOp Kind,
                                const CallExpr *E) {
   return RValue::get(MakeBinaryAtomicValue(CGF, Kind, E));
 }
 
 /// Utility to insert an atomic instruction based Instrinsic::ID and
 /// the expression node, where the return value is the result of the
if (RetTy != V->getType()) {
       assert(V->getType()->canLosslesslyBitCastTo(RetTy) &&
              "Must be able to losslessly bit cast result type");
       V = Builder.CreateBitCast(V, RetTy);
     }
 
     return RValue::get(V);
   }
 
+  switch (getTarget().getTriple().getArch()) {
+  case llvm::Triple::arm:
+  case llvm::Triple::armeb:
+  case llvm::Triple::thumb:
+  case llvm::Triple::thumbeb:
+    switch (BuiltinID) {
+    case ARM::BI__iso_volatile_load8:
+    case ARM::BI__iso_volatile_load16:
+    case ARM::BI__iso_volatile_load32:
+    case ARM::BI__iso_volatile_load64:
+      return RValue::get(EmitVolatileLoad(*this, E));
+    case ARM::BI__iso_volatile_store8:
+    case ARM::BI__iso_volatile_store16:
+    case ARM::BI__iso_volatile_store32:
+    case ARM::BI__iso_volatile_store64:
+      // EmitVolatileStore returns nullptr, but we want to
+      // return that RValue here. If handled via EmitTargetBuiltinExpr
+      // below, the returned Value *V will be nullptr, and we will
+      // continue on to declaring the builtin unsupported below, even
+      // though it was handled correctly.
+      return RValue::get(EmitVolatileStore(*this, E));
+    }
+    break;
+  default:
+    break;
+  }
+
   // See if we have a target specific builtin that needs to be lowered.
   if (Value *V = EmitTargetBuiltinExpr(BuiltinID, E))
     return RValue::get(V);
 
   ErrorUnsupported(E, "builtin function");
 
   // Unknown builtin, for now just dump it out and return undef.
   return GetUndefRValue(E->getType());

test/CodeGen/ms-volatile-arm.c

+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -triple thumbv7-win32 -emit-llvm -fms-extensions -fms-volatile -o - < %s | FileCheck %s
+
+void test1(int volatile *p, int v) {
+  __iso_volatile_store32(p, v);
+  // CHECK-LABEL: @test1
+  // CHECK: store volatile {{.*}}, {{.*}}
+}
+int test2(const int volatile *p) {
+  return __iso_volatile_load32(p);
+  // CHECK-LABEL: @test2
+  // CHECK: load volatile {{.*}}
+}