[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

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

> In https://reviews.llvm.org/D28691#820641, @b-sumner wrote:
>
> > In https://reviews.llvm.org/D28691#820595, @rjmccall wrote:
> >
> > > In https://reviews.llvm.org/D28691#820541, @b-sumner wrote:
> > >
> > > > There are other languages for heterogeneous compute that have scopes, although not exposed quite as explicitly as OpenCL.  For example AMD's "HC" language.  And any language making use of clang and targeting SPIR-V would likely use these builtins.  I think a more generic prefix is appropriate, and "scoped" tells us exactly when these are needed.
> > >
> > >
> > > But would those languages use the same language design for these scopes as OpenCL if they did expose them, as opposed to some more elaborate scoping specification?  My objection is not that the concept is inherently OpenCL-specific, it's that the presentation in the language might be inherently OpenCL-specific, which makes staying in the opencl namespace is prudent.
> >
> >
> > Are you envisioning a language far enough from C/C++ that a standard library or header would not be able to map a scoped atomic operation into a call to one of these new builtins?  Would we expect more of such languages than languages that would do such a mapping?
>
>
> If you're using Clang as a frontend for your language, it must be similar enough to C to call a builtin function.  That's not at issue.  The central question here is whether these builtins are meaningfully general outside of OpenCL.  The concept of heterogenous computation is certainly not specific to OpenCL;  however, these builtins are defined in terms of scopes — "work item", "work group", "device", and "all svm devices" — which, it seems to me, are basically only defined by reference to the OpenCL architecture.  A different heterogenous compute environment might reasonably formalize scopes in a completely different way; for example, it might wish to be more explicit about exactly which peers / devices to synchronize with.
>
> SPIR is explicitly defined on top of the OpenCL model.  Users should be able to use OpenCL builtins when targeting it.  That does not make those builtins more general than OpenCL.
>
> John.


The scope concept in OpenCL is fairly generic. And the builtins just take one extra argument on top of the existing C11 builtin style. The OpenCL scopes have of course specific meaning to the OpenCL model, but there is nothing preventing other uses of the scope feature. As far as I understand atomic scope implementation in LLVM is fairly generic wrt scope types and it is intended for broader functionality than just OpenCL. So I would vote for having this as generic as possible in Clang too even though I don't think name prefix `__opencl` is preventing from using this feature in other languages unless we would disallow the builtins in the other language dialects. Which is not the case with the current patch because the builtins are added as `ATOMIC_BUILTIN` and not `LANGBUILTIN`. We have for example used C11 builtin in OpenCL already. So other cases are possible too.



================
Comment at: include/clang/Basic/SyncScope.h:25-29
+  SingleThread      = 0,
+  WorkGroup         = 1,
+  Device            = 2,
+  System            = 3,
+  SubGroup          = 4,
----------------
t-tye wrote:
> Since the builtins are being named as __opencl then should these also be named as opencl_ since they are the memory scopes for OpenCL using the OpenCL numeric values?
> 
> If another language wants to use memory scopes, would it then add its own langx_* names in a similar way that is done for address spaces where the LangAS enumeration type has values for each distinct language. Each target is then responsible for mapping each language scope to the appropriate target specific scope as is done for address spaces?
> 
> If so then the builtins are really supporting the concept of memory scopes and are not language specific as this enumeration can support multiple languages in the same way as the LangAS enumeration supports multiple languages. If so the builtins would be better named to reflect this as @b-sumner suggested.
We generally prefix the names of OpenCL specific implementations. So perhaps we should do some renaming here in case we don't intend this to be generic implementation.


================
Comment at: lib/Sema/SemaChecking.cpp:3160
+            Op == AtomicExpr::AO__opencl_atomic_load)
+                ? 0
+                : 1);
----------------
Could we merge this and the line after please.


https://reviews.llvm.org/D28691