Treat __sync_synchronize and asm("":::"memory") as stronger fences.

lib/CodeGen/CGBuiltin.cpp

 //===---- CGBuiltin.cpp - Emit LLVM Code for builtins ---------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This contains code to emit Builtin calls as LLVM code.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeGenFunction.h"
 #include "CGObjCRuntime.h"
 #include "CodeGenModule.h"
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/InlineAsm.h" // @LOCALMOD
 #include "llvm/IR/Intrinsics.h"
 
 using namespace clang;
 using namespace CodeGen;
 using namespace llvm;
 
 /// getBuiltinLibFunction - Given a builtin id for a function like
 /// "__builtin_fabsf", return a Function* for "fabsf".
 llvm::Value *CodeGenModule::getBuiltinLibFunction(const FunctionDecl *FD,
                                                   unsigned BuiltinID) {
@@ skipping 994 matching lines @@
   }
 
   case Builtin::BI__sync_synchronize: {
     // We assume this is supposed to correspond to a C++0x-style
     // sequentially-consistent fence (i.e. this is only usable for
     // synchonization, not device I/O or anything like that). This intrinsic
     // is really badly designed in the sense that in theory, there isn't
     // any way to safely use it... but in practice, it mostly works
     // to use it with non-atomic loads and stores to get acquire/release
     // semantics.
+    // @LOCALMOD-START
+    // For PNaCl, surround __sync_synchronize with compiler fences. This
+    // should enforce ordering of more than just atomic memory accesses,
+    // though it won't guarantee that all accesses (e.g. those to
+    // non-escaping objects) will not be reordered.
+    llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, false);
+    std::string AsmString;  // Empty.
+    std::string Constraints("~{memory}");
+    bool HasSideEffect = true;
+    if (getTarget().getTriple().getArch() == llvm::Triple::le32)

[eliben, 2013/08/06 16:26:56]

It would be cleaner to have a single LOCAMOD if-branch, leaving the current code
unmodified in the "else". That's easier to read (clearer what the LOCALMOD
applies to and what was there before), and easier to merge later.

[JF, 2013/08/07 17:58:28]

Done, and as suggested offline the code now uses TargetInfo/getTargetHooks.

+      Builder.CreateCall(
+          llvm::InlineAsm::get(FTy, AsmString, Constraints, HasSideEffect))->
+          addAttribute(llvm::AttributeSet::FunctionIndex,
+                       llvm::Attribute::NoUnwind);
     Builder.CreateFence(llvm::SequentiallyConsistent);
+    if (getTarget().getTriple().getArch() == llvm::Triple::le32)
+      Builder.CreateCall(
+          llvm::InlineAsm::get(FTy, AsmString, Constraints, HasSideEffect))->
+          addAttribute(llvm::AttributeSet::FunctionIndex,
+                       llvm::Attribute::NoUnwind);
+    // @LOCALMOD-END
     return RValue::get(0);
   }
 
   case Builtin::BI__c11_atomic_is_lock_free:
   case Builtin::BI__atomic_is_lock_free: {
     // Call "bool __atomic_is_lock_free(size_t size, void *ptr)". For the
     // __c11 builtin, ptr is 0 (indicating a properly-aligned object), since
     // _Atomic(T) is always properly-aligned.
     const char *LibCallName = "__atomic_is_lock_free";
     CallArgList Args;
@@ skipping 1802 matching lines @@
       break;
     case PPC::BI__builtin_altivec_stvewx:
       ID = Intrinsic::ppc_altivec_stvewx;
       break;
     }
     llvm::Function *F = CGM.getIntrinsic(ID);
     return Builder.CreateCall(F, Ops, "");
   }
   }
 }