rjmccall added inline comments.
================
Comment at: include/clang/Basic/SyncScope.h:59
+ return "opencl_subgroup";
+ }
+}
----------------
yaxunl wrote:
> rjmccall wrote:
> > t-tye wrote:
> > > Should there be a default/assert/static_assert to allow SyncScope enum to
> > > grow due to other languages?
> > There will already be a warning from -Wswitch about this, which should be
> > sufficient.
> >
> > But we do often add llvm_unreachable after switches like this.
> I tried adding
>
> ```
> default:
> llvm_unreachable("Invalid sync scope");
> ```
>
> However, it results in error:
>
> ```
> error: default label in switch which covers all enumeration values
> [-Werror,-Wcovered-switch-default]
> ```
> I guess I'd better leave it as it is since we already have -Wswitch.
Yeah, the idiom is to put the llvm_unreachable after the switch, not in a
default. Check out the uses in Type.cpp for examples: some of them are in
specific cases, saying those cases are illlegal due to some precondition, but
most of them are just after the switch saying that the switch is covered. (The
semantics of switch in C are that switch values with no corresponding case just
skip the whole switch unless there's a default.)
================
Comment at: include/clang/Basic/SyncScope.h:89
+ static std::unique_ptr<AtomicScopeABI> create(LangOptions &Opt);
+};
+
----------------
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.
================
Comment at: include/clang/Basic/SyncScope.h:92
+/// \brief Defines the synch scope ABI for OpenCL.
+class AtomicScopeOpenCLABI : public AtomicScopeABI {
+public:
----------------
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.
https://reviews.llvm.org/D36580
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
