sameerds requested changes to this revision.
sameerds added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13651
+ llvm::getConstantStringInfo(Scope, scp);
+ SSID = getLLVMContext().getOrInsertSyncScopeID(scp);
+
----------------
JonChesterfield wrote:
> sameerds wrote:
> > saiislam wrote:
> > > sameerds wrote:
> > > > This seems to be creating a new ID for any arbitrary string passed as
> > > > sync scope. This should be validated against
> > > > LLVMContext::getSyncScopeNames().
> > > As the FE is not aware about specific target and implementation of sync
> > > scope for that target, getSyncScopeNames() here returns llvm'sdefault
> > > sync scopes, which only supports singlethreaded and system as valid
> > > scopes. Validity checking of memory scope string is being intentionally
> > > left for the later stages which deal with the generated IR.
> > That's pretty strange. At this point, Clang should know what the target is,
> > and it should have a chance to update the list of sync scopes somewhere.
> > @arsenm, do you see a way around this?
> There is already sufficient IR level checking for the string at the
> instruction level. Warning in clang as well could be a nicer user experience,
> but that seems low priority to me.
If there is some checking happening anywhere, then that needs to be
demonstrated in a testcase where the input high-level program passes an illegal
string as the scope argument.
================
Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9
+ // CHECK: fence syncscope("workgroup") seq_cst
+ __builtin_memory_fence(__ATOMIC_SEQ_CST, "workgroup");
+
----------------
JonChesterfield wrote:
> sameerds wrote:
> > JonChesterfield wrote:
> > > saiislam wrote:
> > > > sameerds wrote:
> > > > > Orderings like `__ATOMIC_SEQ_CST` are defined for C/C++ memory
> > > > > models. They should not be used with the new builtin because this new
> > > > > builtin does not follow any specific language model. For user
> > > > > convenience, the right thing to do is to introduce new tokens in the
> > > > > Clang preprocessor, similar to the `__ATOMIC_*` tokens. The
> > > > > convenient shortcut is to just tell the user to supply numerical
> > > > > values by looking at the LLVM source code.
> > > > >
> > > > > From llvm/Support/AtomicOrdering.h, note how the numerical value for
> > > > > `__ATOMIC_SEQ_CST` is 5, but the numerical value for the LLVM
> > > > > SequentiallyConsistent ordering is 7. The numerical value 5 refers to
> > > > > the LLVM ordering "release". So, if the implementation were correct,
> > > > > this line should result in the following unexpected LLVM IR:
> > > > > fence syncscope("workgroup") release
> > > > As you pointed out, the range of acquire to sequentiallly consistent
> > > > memory orders for llvm::AtomicOrdering is [4, 7], while for
> > > > llvm::AtomicOrderingCABI is [2, 5]. Enums of C ABI was taken to ensure
> > > > easy of use for the users who are familiar with C/C++ standard memory
> > > > model. It allows them to use macros like __ATOMIC_ACQUIRE etc.
> > > > Clang CodeGen of the builtin internally maps C ABI ordering to llvm
> > > > atomic ordering.
> > > What language, implemented in clang, do you have in mind that reusing the
> > > existing __ATOMIC_* macros would be incorrect for?
> > I think we agreed that this builtin exposes the LLVM fence exactly. That
> > would mean it takes arguments defined by LLVM. If you are implementing
> > something different from that, then it first needs to be specified
> > properly. Perhaps you could say that this is a C ABI compatible builtin,
> > that happens to take target specific scopes? That should cover OpenCL whose
> > scope enum is designed to be compatible with C.
> >
> > Whatever it is that you are trying to implement here, it definitely does
> > not expose a raw LLVM fence.
> The llvm fence, in text form, uses a symbol for the memory scope. Not an enum.
>
> This symbol is set using these macros for the existing atomic builtins. Using
> an implementation detail of clang instead is strictly worse, by layering and
> by precedent.
>
> ABI is not involved here. Nor is OpenCl.
The `__ATOMIC_*` symbols in Clang quite literally represent the C/C++ ABI. See
the details in AtomicOrdering.h and InitPreprocessor.cpp. I am not opposed to
specifying that the builtin expects these symbols, but then it is incorrect to
say that the builtin exposes the raw LLVM builtin. It is a C-ABI-compatible
builtin that happens to take target-specific scope as a string argument. And
that would also make it an overload of the already existing builting
__atomic_fence().
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75917/new/
https://reviews.llvm.org/D75917
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
