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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In https://reviews.llvm.org/D28691#648267, @rjmccall wrote:

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


I sent an RFC to cfe-dev.

For the synchronization scope, I will add Clang's enums for synchronization scopes in a similar fashion as memory order. Different languages can have their own specific synchronization scopes. Then clang translates them to synchronization scopes used by LLVM atomic instructions.



================
Comment at: lib/CodeGen/CGAtomic.cpp:1033
+  assert(isa<llvm::ConstantInt>(Scope) &&
+      "Non-constant synchronization scope not supported");
+  auto sco = (llvm::SynchronizationScope)(
----------------
rjmccall wrote:
> 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.
Will add diagnostics to Sema.


================
Comment at: lib/Sema/SemaChecking.cpp:2922
+    Scope = IntegerLiteral::Create(Context,
+      llvm::APInt(Context.getTypeSize(Context.IntTy), (uint64_t) 1),
+      Context.IntTy, SourceLocation());
----------------
rjmccall wrote:
> 1 is a magic number here.
Will change that to enum.


https://reviews.llvm.org/D28691