[PATCH] D30643: [OpenCL] Extended diagnostics for atomic initialization

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8263
+def err_atomic_init_addressspace : Error<
+  "initialization of atomic variables is restricted to variables in global address space">;
 def err_atomic_init_constant : Error<
----------------
bader wrote:
> bader wrote:
> > Anastasia wrote:
> > > echuraev wrote:
> > > > Anastasia wrote:
> > > > > Could we combine this error diag with the one below? I guess they are semantically very similar apart from one is about initialization and another is about assignment?
> > > > I'm not sure that it is a good idea to combine these errors. For example, if developer had declared a variable non-constant and not in global address space he would have got the same message for both errors. And it can be difficult to determine what the exact problem is. He can fix one of the problems but he will still get the same error.
> > > Well, I don't actually see that we check for constant anywhere so it's also OK if you want to drop this bit. Although I think the original intension of this message as I understood was to provide the most complete hint.
> > > 
> > > My concern is that these two errors seem to be reporting nearly the same issue and ideally we would like to keep diagnostic list as small as possible. This also makes the file more concise and messages more consistent.
> > I suggest adding a test case with non-constant initialization case to validate that existing checks cover this case for atomic types already.
> > If so, we can adjust existing diagnostic message to cover both cases: initialization and assignment expression.
> I don't think it's quite true.
> There are two requirements here that must be met the same time. Atomic variables *declared in the global address space* can be initialized only with "compile time constant'.
> If understand the spec correctly this code is also valid:
> 
>   kernel void foo() {
>     static global atomic_int a = 42; // although it's not clear if we must use ATOMIC_VAR_INIT here.
>     ...
>   }
Precisely, but I think checking for compile time constant should be inherited from the general C implementation? I don't think we do anything extra for it.  Regarding the macro I am not sure we can suitably diagnose it anyways...


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8268
+// Atomics
+def err_atomic_init: Error<
+  "atomic variable can only be assigned to a compile time constant or to variables in global adress space">;
----------------
also we should add opencl there:

err_opencl_...


https://reviews.llvm.org/D30643