rjmccall requested changes to this revision.
rjmccall added a comment.
This revision now requires changes to proceed.

This patch changes the language design of the atomic builtins, which is outside 
the normal scope of patch review.  You need to post an RFC to cfe-dev.  I've 
gone ahead and made some material comments, but the concept itself needs debate.

Your proposed language design exposes LLVM internals (the specific values used 
by llvm::SynchronizationScope) directly to users; that is also unacceptable.

Comment at: lib/AST/Expr.cpp:3917
   case AO__c11_atomic_init:
+    return 2;
   case AO__c11_atomic_load:
Add an extra newline here, please, to be consistent with the other cases in the 

Comment at: lib/CodeGen/CGAtomic.cpp:1033
+  assert(isa<llvm::ConstantInt>(Scope) &&
+      "Non-constant synchronization scope not supported");
+  auto sco = (llvm::SynchronizationScope)(
It is not acceptable to test this exclusively with an assertion; you need to be 
checking that the argument is an integer constant expression in Sema.

Comment at: lib/Sema/SemaChecking.cpp:2922
+    Scope = IntegerLiteral::Create(Context,
+      llvm::APInt(Context.getTypeSize(Context.IntTy), (uint64_t) 1),
+      Context.IntTy, SourceLocation());
1 is a magic number here.


cfe-commits mailing list

Reply via email to