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 

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.


cfe-commits mailing list

Reply via email to