[PATCH] D28691: Support synchronisation scope in Clang atomic builtin functions
John McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 17 08:57:07 PST 2017
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
More information about the cfe-commits
mailing list