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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
