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

Thu Feb 26 06:00:25 PST 2015

Hi Sameer,

Thanks for your feedback!

Yes, CL2.0 atomic builtin function are completely missing in Clang and therefore no proper diagnostics can be done at the moment. Regarding the init macro I am not sure how we would do this check since the macro is supposed to be implementation/architecture specific.

Regards,

Anastasia

From: Sameer Sahasrabuddhe [mailto:sameer.sahasrabuddhe at amd.com] 
Sent: 19 February 2015 06:25
To: Anastasia Stulova; cfe-commits at cs.uiuc.edu
Subject: Re: [Patch][OpenCL] CL2.0 atomic types, diagnostics

Hello Anastasia,

LGTM!

My comment about initialization was regarding possible checking in the frontend ATOMIC_VAR_INIT and atomic_init, but outside the scope of the current patch. For example, the object and value for ATOMIC_VAR_INIT do need to be "initialization compatible" as mentioned in Section 6.13.11.1, and atomic_init does require that the "type C" of the value is the same as the corresponding atomic "type A". Also, the macro can only be used "to initialize atomic objects that are declared in program scope in the global address space." The place where these checks happen will determine how informative are the error messages generated.

Sameer.

On 2/17/2015 4:08 PM, Anastasia Stulova wrote:

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150226/ace44b96/attachment.html>