yaxunl marked 10 inline comments as done. yaxunl added inline comments.
================ Comment at: include/clang/Basic/SyncScope.h:46 + Scopes.push_back(SyncScope::OpenCLSubGroup); + return Scopes; +} ---------------- rjmccall wrote: > 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. Thanks. Will do. ================ Comment at: include/clang/Basic/SyncScope.h:47 + return Scopes; +} + ---------------- rjmccall wrote: > You could just return an ArrayRef to a static const array. Thanks. Will do. ================ Comment at: include/clang/Basic/SyncScope.h:59 + return "opencl_subgroup"; + } +} ---------------- 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. ================ Comment at: lib/CodeGen/CGAtomic.cpp:687 + + auto *SC = Builder.CreateIntCast(Scope, Builder.getInt32Ty(), false); + // If unsupported sync scope is encountered at run time, assume default sync ---------------- rjmccall wrote: > Does Sema not coerce the argument to int? It really should, and then you can > just rely on that here. (You should use CGF.IntTy if you do this, though, in > case you're on a target with a non-32-bit int.) It is coerced to `Context.IntTy`. I will add some tests for non-integral order/scope argument. ================ Comment at: lib/CodeGen/CGAtomic.cpp:696 + if (S != Default) + SI->addCase(Builder.getInt32(static_cast<unsigned>(S)), B); + ---------------- rjmccall wrote: > t-tye wrote: > > rjmccall wrote: > > > 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. > > That sounds reasonable to me. When/if another language comes along the task > > of mapping the multiple ABIs can be done then. Still wanted to make sure it > > is clear that the enum values in the SyncScope enum are documented as BEING > > the OpenCL runtime ABI values and not some arbitrary list as in other enums > > such as the address space enum. > They are documented as being the values expected by the builtins, so they > already can't be arbitrarily changed. > > Now, the values expected by the builtin were chosen to match this specific > runtime, but we had to choose something, and matching a runtime isn't the > worst way of doing that. But now that I think about it, those values seem to > be rather sub-optimal because they don't start at 0, which means that things > like this switch will be less efficient than they could be. And I think the > AST and IRGen would benefit from needing to renumber values; it will make the > code clearer, and it'll help protect against people adding values to this > enum just because those values are used by the runtime, i.e. without fully > realizing that they're expanding a user-facing feature. So let's go ahead > and renumber the values in SyncScope to start at 0 and then re-map them in > IRGen. Will do. Thanks. https://reviews.llvm.org/D36580 _______________________________________________ cfe-commits mailing list firstname.lastname@example.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits