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