rjmccall added inline comments.

================
Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:
----------------
yaxunl wrote:
> rjmccall wrote:
> > Please correct me if I'm wrong, but I believe you don't get to define the 
> > "ABI for OpenCL" any more than I get to define the "ABI for C".  You're 
> > defining the ABI for your specific OpenCL implementation, but Clang might 
> > reasonably support other implementations with their own ABIs.  So this name 
> > is inappropriate.
> > 
> > Now, if I understand how SPIR works, SPIR does require some sort of stable 
> > lowering of these atomic operations in order to ensure that the resulting 
> > LLVM IR can be ingested into an arbitrary implementation.  (Ot at least 
> > that was true prior to SPIR V?)  Therefore, SPIR needs to specify a 
> > lowering for these atomic operations, and that probably includes ABI values 
> > for specific sync-scopes.  But the appropriate name for that would be the 
> > "SPIR ABI", not the "OpenCL ABI".  And, of course, you would need to be 
> > sure that you're implementing the lowering that SPIR actually wants.
> The ABI is not intended for a specific OpenCL implementation, but for 
> extending to other languages. For example, we have a downstream C++ based 
> language called HCC, which may support atomic scope with an ABI different 
> than OpenCL atomic scope ABI.
By "ABI" you mean that it might present a different model of synchronization 
scopes to the source language?  Okay, that's great, and it explains a lot of 
things that were very murky about some of our previous conversations.

In that case, I think the appropriate design for these builtins is:

  - They don't need to be in the __builtin_opencl namespace.
  - They *do* need to be enabled only in language modes with well-defined 
sync-scope models, which for now just means OpenCL. 
  - The language mode's sync-scope model should determine everything about the 
scope argument, including its type.  Sema-level validation requires first 
determining the language's sync-scope model.
  - There is no meaningful way to "default" the scope argument on the 
non-scoped builtins the way that we are now.  Instead, the scope argument 
should only exist for the scoped builtins.

Alternatively, if you potentially want the opencl-model builtins to be usable 
from non-opencl languages, the right design is for HCC to use its own set of 
builtins with their own type-checking rules.

In either case, I don't think it's a good idea to call this "ABI", which is 
associated too strongly with target-lowering concepts.  You're really talking 
about a source-language concept.


https://reviews.llvm.org/D36580



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to