[OpenCL] Extended diagnostics for atomic initialization

[echuraev]

Summary

I saw the same changes in the following review: https://reviews.llvm.org/D17438

I don't know in that way I could determine that atomic variable was initialized by macro ATOMIC_VAR_INIT. Anyway I added check that atomic variables can be initialize only in global scope.
I think that we can discuss this change.

[Anastasia]

include/clang/Basic/DiagnosticSemaKinds.td

8269

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?

lib/Sema/SemaInit.cpp

6498

I would remove S.getLangOpts().OpenCL check, it's redundant in my opinion!

6501

Even thought it's done above too, I don't really see the point of having this variable.

[echuraev]

include/clang/Basic/DiagnosticSemaKinds.td

8269

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.

[Anastasia]

include/clang/Basic/DiagnosticSemaKinds.td

8269

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.

[bader]

include/clang/Basic/DiagnosticSemaKinds.td

8269

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.

[bader]

include/clang/Basic/DiagnosticSemaKinds.td

8269

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.
  ...
}

8272

I suggest removing this comment.
If you are going to add other diagnostic messages specific to OpenCL atomics, then separate them from this list of unordered diagnostics similar to pipe built-in functions below.

[Anastasia]

include/clang/Basic/DiagnosticSemaKinds.td

8269

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...

8273

also we should add opencl there:

err_opencl_...

[bader]

include/clang/Basic/DiagnosticSemaKinds.td

8269

Precisely, but I think checking for compile time constant should be inherited from the general C implementation?

Agree. I suggested checking this above by extending OpenCL tests, but this can be done separately.

[Anastasia]

test/SemaOpenCL/atomic-init.cl

7

Btw, you could keep "initialized" here by using 'select' in the diagnostic message.

11

Could we also add a check for 'static global' variable?

[Anastasia]

test/SemaOpenCL/atomic-init.cl

7

Btw you still keep "assigned" in the error message. What I mean is that we could put "initialized" instead in this case.

[Anastasia]

test/SemaOpenCL/atomic-init.cl

7

This diagnostic seems wrong! How is 1 not a compile time constant?

Didn't we agree to drop the constant bit from the error message and just keep reporting about the program (global visibility) scope? I think the restriction about the scope holds for both initialization and assignment and therefore I have asked to unify the diagnostic...

[Anastasia]

LGTM! Thanks!

include/clang/Basic/DiagnosticSemaKinds.td

def err_reference_pipe_type : Error <
   "pipes packet types cannot be of reference type">;
 def err_opencl_no_main : Error<"%select{function|kernel}0 cannot be called 'main'">;
 def err_opencl_kernel_attr :
   Error<"attribute %0 can only be applied to a kernel function">;
 def err_opencl_return_value_with_address_space : Error<
   "return value cannot be qualified with address space">;
 def err_opencl_constant_no_init : Error<
   "variable in constant address space must be initialized">;
+// Atomics
+def err_atomic_init_addressspace : Error<
+  "initialization of atomic variables is restricted to variables in global address space">;
 def err_atomic_init_constant : Error<
   "atomic variable can only be assigned to a compile time constant"
   " in the declaration statement in the program scope">;
 def err_opencl_implicit_vector_conversion : Error<
   "implicit conversions between vector types (%0 and %1) are not permitted">;
 def err_opencl_invalid_type_array : Error<
   "array of %0 type is invalid in OpenCL">;
 def err_opencl_ternary_with_block : Error<
   "block type cannot be used as expression in ternary expression in OpenCL">;
 def err_opencl_pointer_to_type : Error<
   "pointer to type %0 is invalid in OpenCL">;
 def err_opencl_type_can_only_be_used_as_function_parameter : Error <

lib/Sema/SemaInit.cpp

if (S.getLangOpts().CPlusPlus11 && Entity.getType()->isReferenceType() &&
     // Produce a C++98 compatibility warning if we are initializing a reference
     // from an initializer list. For parameters, we produce a better warning
     // elsewhere.
     Expr *Init = Args[0];
     S.Diag(Init->getLocStart(), diag::warn_cxx98_compat_reference_list_init)
       << Init->getSourceRange();
   }
 
+  // OpenCL v2.0 s6.13.11.1. atomic variables can be initialized in global scope
+  QualType ETy = Entity.getType();
+  Qualifiers TyQualifiers = ETy.getQualifiers();
+  bool HasGlobalAS = TyQualifiers.hasAddressSpace() &&
+                     TyQualifiers.getAddressSpace() == LangAS::opencl_global;
+
+  if (S.getLangOpts().OpenCLVersion >= 200 &&
+      ETy->isAtomicType() && !HasGlobalAS &&
+      Entity.getKind() == InitializedEntity::EK_Variable && Args.size() > 0) {
+    S.Diag(Args[0]->getLocStart(), diag::err_atomic_init_addressspace) <<
+    SourceRange(Entity.getDecl()->getLocStart(), Args[0]->getLocEnd());
+    return ExprError();
+  }
+
   // Diagnose cases where we initialize a pointer to an array temporary, and the
   // pointer obviously outlives the temporary.
   if (Args.size() == 1 && Args[0]->getType()->isArrayType() &&
       Entity.getType()->isPointerType() &&
       InitializedEntityOutlivesFullExpression(Entity)) {
     const Expr *Init = Args[0]->skipRValueSubobjectAdjustments();
     if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Init))
       Init = MTE->GetTemporaryExpr();

test/SemaOpenCL/atomic-init.cl

+// RUN: %clang_cc1 -O0 -cl-std=CL2.0 -fsyntax-only -verify  %s
+
+global atomic_int a1 = 0;
+
+kernel void test_atomic_initialization() {
+  atomic_int a2 = 0; // expected-error {{initialization of atomic variables is restricted to variables in global address space}}
+  private atomic_int a3 = 0; // expected-error {{initialization of atomic variables is restricted to variables in global address space}}
+  local atomic_int a4 = 0; // expected-error {{'__local' variable cannot have an initializer}}
+  global atomic_int a5 = 0; // expected-error {{function scope variable cannot be declared in global address space}}
+}