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