[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 7 inline comments as done.
yaxunl added inline comments.


================
Comment at: include/clang/Basic/SyncScope.h:89
+  static std::unique_ptr<AtomicScopeABI> create(LangOptions &Opt);
+};
+
----------------
rjmccall wrote:
> The previous design of this header seems more appropriate to me.  Clang defines a builtin with standardized argument values for all targets; we're not trying to allow targets to customize the arguments taken by this builtin, at least not yet.  The sync-scope argument values are the values from this enum. Because they are potentially exposed in source programs, they cannot be lightly changed and re-ordered; that is, they are ABI.  There is a second level of ABI, which is the set of values demanded by target runtime functions.  It is IRGen's responsibility to turn the SyncScope values into those values when generating a call to the runtime; that mapping does not need to be exposed here in the AST.
This function is not intended to create ABI's for different targets, but to create ABI's for different languages. Currently only OpenCL supports atomic scope, but in the future there may be other languages which support atomic scope with a different ABI.


================
Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:
----------------
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.


https://reviews.llvm.org/D36580