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