[Patch][OpenCL] CL2.0 atomic types, diagnostics

[Anastasia Stulova]

@Sameer or anyone else, are there any more comments to this patch?

Thanks,
Anastasia


-----Original Message-----
From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Anastasia Stulova
Sent: 12 February 2015 17:16
To: 'Sahasrabuddhe, Sameer'; cfe-commits at cs.uiuc.edu
Subject: RE: [Patch][OpenCL] CL2.0 atomic types, diagnostics

Hi Sameer,

Patch updated with your comments!

A few issues here:

1. I am not entirely sure what extra steps we need to do in the compiler for initialization of atomic variables, since program scope atomic variables can only be initialized with constant values and atomic_init function seems to have no restrictions on the type of the value passed in.

2.  'atomic_int' (aka '_Atomic(int)') comes from standard TypePrinter, because we implement CL2.0 atomics by aliasing them with C11 atomics. That saves us a lot of implementation effort and avoids code duplication.

Regards,
Anastasia



-----Original Message-----
From: Sahasrabuddhe, Sameer [mailto:sameer.sahasrabuddhe at amd.com] 
Sent: 16 December 2014 05:24
To: Anastasia Stulova; cfe-commits at cs.uiuc.edu
Subject: Re: [Patch][OpenCL] CL2.0 atomic types, diagnostics


 > diff --git a/include/clang/Basic/DiagnosticSemaKinds.td
b/include/clang/Basic/DiagnosticSemaKinds.td
 > index 75e78bf..9f077c9 100644
 > --- a/include/clang/Basic/DiagnosticSemaKinds.td
 > +++ b/include/clang/Basic/DiagnosticSemaKinds.td
 > @@ -7175,6 +7175,9 @@ 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">;

Could we rephrase this? Perhaps "assigning directly to atomic objects is not allowed". The assumption is that initialization will be checked elsewhere, so it does not have to be mentioned here. Should that be part of this patch?

 >  } // end of sema category
 >
 >  let CategoryName = "OpenMP Issue" in {  > diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp  > index 76e3612..b403e53 100644  > --- a/lib/Sema/SemaExpr.cpp  > +++ b/lib/Sema/SemaExpr.cpp  > @@ -9489,6 +9489,20 @@ ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
 >        return ExprError();
 >    }
 >
 > +  if (getLangOpts().OpenCL) {
 > +    // OpenCLC v2.0 s6.13.11.1 allows atomic variables to be 
initialized by
 > +    // the ATOMIC_VAR_INIT macro.
 > +    if (LHSExpr->getType()->isAtomicType() ||
 > +        RHSExpr->getType()->isAtomicType()) {
 > +      SourceRange SR(LHSExpr->getLocStart(), RHSExpr->getLocEnd());
 > +      if (BO_Assign == Opc)
 > +        Diag(OpLoc, diag::err_atomic_init_constant) << SR;

I think the recommended style is "Opc == BO_Assign" (but I am not sure).

 > +      else
 > +        ResultTy = InvalidOperands(OpLoc, LHS, RHS);
 > +      return ExprError();
 > +    }
 > +  }
 > +
 >    switch (Opc) {
 >    case BO_Assign:
 >      ResultTy = CheckAssignmentOperands(LHS.get(), RHS, OpLoc, 
QualType());
 > @@ -9959,6 +9973,14 @@ ExprResult
Sema::CreateBuiltinUnaryOp(SourceLocation OpLoc,
 >    ExprValueKind VK = VK_RValue;
 >    ExprObjectKind OK = OK_Ordinary;
 >    QualType resultType;
 > +  if (getLangOpts().OpenCL) {
 > +    // The only legal unary operation for atomics is '&'.
 > +    if (Opc != UO_AddrOf && InputExpr->getType()->isAtomicType()) {
 > +      return ExprError(Diag(OpLoc, diag::err_typecheck_unary_expr)
 > +                       << InputExpr->getType()
 > +                       << Input.get()->getSourceRange());
 > +    }
 > +  }
 >    switch (Opc) {
 >    case UO_PreInc:
 >    case UO_PreDec:
 > diff --git a/test/Parser/opencl-atomics-cl20.cl
b/test/Parser/opencl-atomics-cl20.cl
 > index d5cecd1..4ba699b1 100644
 > --- a/test/Parser/opencl-atomics-cl20.cl
 > +++ b/test/Parser/opencl-atomics-cl20.cl
 > @@ -31,3 +31,16 @@ void atomic_types_test() {  >  // expected-error at -16 {{use of undeclared identifier 'atomic_size_t'}}  >  // expected-error at -16 {{use of undeclared identifier 'atomic_ptrdiff_t'}}  >  #endif  > +  > +#ifdef CL20  > +void foo(atomic_int * ptr) {}  > +void atomic_ops_test() {  > +  atomic_int i;  > +  foo(&i);  > +// OpenCL v2.0 s6.13.11.8, arithemtic operation 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 {{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')}}

Nitpick: The "aka '_Atomic(int)'" isn't really true for OpenCL 2.0! But I don't have a strong opinion either way.

 > +}
 > +#endif
 >

Sameer.