[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:46
+  Scopes.push_back(SyncScope::OpenCLSubGroup);
+  return Scopes;
+}
----------------
t-tye wrote:
> Should there be an assert/static_assert in case SyncScope enum grows due to other languages?
It makes sense to add a 'last' enumerator to SyncScope and do a static_assert here that last == OpenCLSubGroup.


================
Comment at: include/clang/Basic/SyncScope.h:59
+    return "opencl_subgroup";
+  }
+}
----------------
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.


================
Comment at: lib/CodeGen/CGAtomic.cpp:696
+    if (S != Default)
+      SI->addCase(Builder.getInt32(static_cast<unsigned>(S)), B);
+
----------------
t-tye wrote:
> Is it documented in the SyncScope enum that the enumerator values are in fact the values used for source language runtime values? Seems if other languages want to use scopes they may may have a different ordering. That would imply that there would be a function to map a SyncScope value to the value used by the source language. For OpenCL the mapping is identity.
> 
> The memory ordering has the isValidAtomicOrderingCABI() that does a similar thing.
The values in the SyncScope enum are the source language values.  We already have a step to translate them into LLVM values when we generate a native LLVM construct.  To the extent that we call into a runtime instead, none of that code has been written to be runtime-agnostic at all, and I've been assuming that we're basically okay with that, at least for now.


https://reviews.llvm.org/D36580