[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

8289

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

6511

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

6514

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

[echuraev]

include/clang/Basic/DiagnosticSemaKinds.td

8289

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

8289

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

8289

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

8287–8289

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.

8289

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

[Anastasia]

include/clang/Basic/DiagnosticSemaKinds.td

8288

also we should add opencl there:

err_opencl_...

8289

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

[bader]

include/clang/Basic/DiagnosticSemaKinds.td

8289

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">;
-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_atomic_init: Error<
+  "atomic variable can be %select{assigned|initialized}0 to a variable only "
+  "in global address space">;
 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">;

lib/Sema/SemaExpr.cpp

ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
   if (getLangOpts().OpenCL) {
     QualType LHSTy = LHSExpr->getType();
     QualType RHSTy = RHSExpr->getType();
     // OpenCLC v2.0 s6.13.11.1 allows atomic variables to be initialized by
     // the ATOMIC_VAR_INIT macro.
     if (LHSTy->isAtomicType() || RHSTy->isAtomicType()) {
       SourceRange SR(LHSExpr->getLocStart(), RHSExpr->getLocEnd());
       if (BO_Assign == Opc)
-        Diag(OpLoc, diag::err_atomic_init_constant) << SR;
+        Diag(OpLoc, diag::err_opencl_atomic_init) << 0 << SR;
       else
         ResultTy = InvalidOperands(OpLoc, LHS, RHS);
       return ExprError();
     }
 
     // OpenCL special types - image, sampler, pipe, and blocks are to be used
     // only with a builtin functions and therefore should be disallowed here.
     if (LHSTy->isImageType() || RHSTy->isImageType() ||

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_opencl_atomic_init) << 1 <<
+    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/Parser/opencl-atomics-cl20.cl

 
 #ifdef CL20
 void foo(atomic_int * ptr) {}
 void atomic_ops_test() {
   atomic_int i;
   foo(&i);
 // OpenCL v2.0 s6.13.11.8, arithemtic operations are not permitted on atomic types.
   i++; // expected-error {{invalid argument type 'atomic_int' (aka '_Atomic(int)') to unary expression}}
-  i = 1; // expected-error {{atomic variable can only be assigned to a compile time constant in the declaration statement in the program scope}}
+  i = 1; // expected-error {{atomic variable can be assigned to a variable only in global address space}}
   i += 1; // expected-error {{invalid operands to binary expression ('atomic_int' (aka '_Atomic(int)') and 'int')}}
   i = i + i; // expected-error {{invalid operands to binary expression ('atomic_int' (aka '_Atomic(int)') and 'atomic_int')}}
 }
 #endif

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() {
+  a1 = 1; // expected-error {{atomic variable can be assigned to a variable only in global address space}}
+  atomic_int a2 = 0; // expected-error {{atomic variable can be initialized to a variable only in global address space}}
+  private atomic_int a3 = 0; // expected-error {{atomic variable can be initialized to a variable only 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}}
+  static global atomic_int a6 = 0;
+}