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

[John McCall via Phabricator via cfe-commits]

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