[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

[Yaxun Liu via Phabricator via cfe-commits]

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