[PATCH] D28691: Support synchronisation scope in Clang atomic builtin functions

[John McCall via Phabricator via cfe-commits]

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 switch.


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


https://reviews.llvm.org/D28691