[OpenCL] Add Sema checks for atomics and implicit declaration

[pxli168]

Summary

Add Sema checks for atomics and implicit declaration
This patch is partitioned from http://reviews.llvm.org/D16047

[Anastasia]

lib/Sema/SemaDecl.cpp

3900

I am still not convinced about this change? Could you give reference to spec or an example? I don't understand why you are trying to change default C behavior.

7283

Why this code?

test/Parser/opencl-atomics-cl20.cl

71

Please remove the macro here as it doesn't have any functionality.

This message seems wrong. Why is it restricted to variables in global address space?

We have similar diagnostic err_atomic_init_constant. Could you reuse it, may be with help of %select?

[Anastasia]

lib/Sema/SemaDecl.cpp

11504

Could you add spec reference please!

[pxli168]

It seems this patch is useless.
The spec does not tell about implicit declaration of function, but now clang with -triple spir will output err if there is implicit declaration of function.
I have read about spir and opencl spec but could not find anything talk about that.
If this is a clang bug then this patch is useless.

What do you think?

lib/Sema/SemaDecl.cpp

3900

Just removed.

7283

Removed.

[Anastasia]

I agree there seems to be nothing specifically on this topic in OpenCL spec. However, I wouldn't modify Clang and rely on its default behavior:

1. In C99 gives a warning
2. For some targets set up in a special way (i.e. SPIR) gives an error

lib/Sema/SemaInit.cpp

6156

Actually I don't see any statement in spec that forbids initialization of atomic variables inside the functions. It just says that ATOMIC_VAR_INIT can only be used with global variables, but you are not checking that here.

[pxli168]

It now gives a warning for both C99 and OpenCL.
But for target SPIR it gives an err only because it treat unprototyped function as varidic function:

/// \brief Checks whether the given calling convention supports variadic
/// calls. Unprototyped calls also use the variadic call rules.
inline bool sthiupportsVariadicCall(CallingConv CC) {...}

and in OpenCL s6.9.e it says

e. Variadic macros and functions with the exception of printf and enqueue_kernel are not supported.

If SPIR is followed this rule, then OpenCL should also give a err. But OpenCL gives only a warning.
I could not find where the following referenced

Unprototyped calls also use the variadic call rules.

What do you think? Should we asked about if implicit declaration of function is allowed in OpenCL, and should we treat these function as variadic functions?

[Anastasia]

Yes, I think it deserves clarification. Could you submit a bug to Khronos then?

[pxli168]

https://cvs.khronos.org/bugzilla/show_bug.cgi?id=15603
Bug reported.

[pxli168]

Hi Anastasia,
https://cvs.khronos.org/bugzilla/show_bug.cgi?id=15603
Now we got clarify from khronos. I will continue on this patch.
BTW, https://cvs.khronos.org/bugzilla/show_bug.cgi?id=15599 is also fixed in the same revision.
Thanks
Xiuli

[Anastasia]

@pxli168, do you still plan to update this? I think the implicit declarations would be quite useful to have.

[pxli168]

Yes, I found although the khronos have make a clarify with implicit declarations but they sames to be useful with some program link. And I found some test case about link could not pass by this reason.
But how should we handle the new added spec?

[Anastasia]

In D17438#443422, @pxli168 wrote:

Yes, I found although the khronos have make a clarify with implicit declarations but they sames to be useful with some program link. And I found some test case about link could not pass by this reason.
But how should we handle the new added spec?

Could you add the tests that don't pass to the Khronos bug? I will try to discuss it on the next call.

Thanks!

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_main_redefined : Warning<"variable named 'main' with external linkage "
     "has undefined behavior">, InGroup<Main>;
 def ext_main_used : Extension<
   "ISO C++ does not allow 'main' to be used by a program">, InGroup<Main>;
 
 /// parser diagnostics
 def ext_no_declarators : ExtWarn<"declaration does not declare anything">,
   InGroup<MissingDeclarations>;
+def err_no_declarators : Error<"declaration does not declare anything">;
 def ext_typedef_without_a_name : ExtWarn<"typedef requires a name">,
   InGroup<MissingDeclarations>;
 def err_typedef_not_identifier : Error<"typedef name must be an identifier">;
 def err_typedef_changes_linkage : Error<"unsupported: typedef changes linkage"
   " of anonymous type, but linkage was already computed">;
 def note_typedef_changes_linkage : Note<"use a tag name here to establish "
   "linkage prior to definition">;
 def err_statically_allocated_object : Error<
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_implicit_vector_conversion : Error<
   "implicit conversions between vector types (%0 and %1) are not permitted">;
+def err_opencl_implicit_function_decl : Error<
+  "implicit declaration of function %0 is invalid in OpenCL">;
 def err_opencl_dereferencing : Error<
   "dereferencing pointer of type %0 is not allowed in OpenCL">;
 def err_opencl_pointer_to_image : Error<
   "pointer to image is invalid in OpenCL">;
 def err_opencl_type_can_only_be_used_as_function_parameter : Error <
   "%0 can only be used as a function parameter">;
+def err_opencl_atomic_init_addressspace : Error<
+  "initialization of atomic variables is restricted to variables in global address space in opencl">;
 def err_opencl_block_proto_variadic : Error<
   "invalid block prototype, variadic arguments are not allowed in opencl">;
 def err_opencl_invalid_block_array : Error<
   "array of block is invalid in OpenCL">;
 def err_opencl_ternary_with_block : Error<
   "blocks cannot be used as expressions in ternary expressions in opencl">;
 
 // OpenCL v2.0 s6.13.6 -- Builtin Pipe Functions

lib/Sema/SemaDecl.cpp

Sema::ParsedFreeStandingDeclSpec(Scope *S, AccessSpecifier AS, DeclSpec &DS,
   //   A declaration [...] shall declare at least a declarator [...], a tag,
   //   or the members of an enumeration.
   // C++ [dcl.dcl]p3:
   //   [If there are no declarators], and except for the declaration of an
   //   unnamed bit-field, the decl-specifier-seq shall introduce one or more
   //   names into the program, or shall redeclare a name introduced by a
   //   previous declaration.
   if (!DeclaresAnything) {
+    // OpenCL C doesn't support bit-field, so declaration with no declarator
+    // has no use.
+    if (getLangOpts().OpenCL) {
+      Diag(DS.getLocStart(), diag::err_no_declarators) << DS.getSourceRange();
+      return 0;
+    }
     // In C, we allow this as a (popular) extension / bug. Don't bother
     // producing further diagnostics for redundant qualifiers after this.
     Diag(DS.getLocStart(), diag::ext_no_declarators) << DS.getSourceRange();
     return TagD;
   }
 
   // C++ [dcl.stc]p1:
   //   If a storage-class-specifier appears in a decl-specifier-seq, [...] the
enum OpenCLParamType {
   RecordKernelParam
 };
 
 static OpenCLParamType getOpenCLKernelParameterType(QualType PT) {
   if (PT->isPointerType()) {
     QualType PointeeType = PT->getPointeeType();
     if (PointeeType->isPointerType())
       return PtrPtrKernelParam;
-    return PointeeType.getAddressSpace() == 0 ? PrivatePtrKernelParam
-                                              : PtrKernelParam;
+    // Now generice address space is added, we need to handle like this
+    unsigned addrSpace = PointeeType.getAddressSpace();
+    return (addrSpace != LangAS::opencl_global &&
+            addrSpace != LangAS::opencl_constant &&
+            addrSpace != LangAS::opencl_local)
+               ? PrivatePtrKernelParam
   }
 
   // TODO: Forbid the other integer types (size_t, ptrdiff_t...) when they can
   // be used as builtin types.
 
   if (PT->isImageType())
     return PtrKernelParam;
 
   if (PT->isBooleanType())
     return InvalidKernelParam;
 
   if (PT->isEventT())
     return InvalidKernelParam;
 
   if (PT->isHalfType())
     return InvalidKernelParam;
 
+  if (PT->isReserveIDT())
+    return InvalidKernelParam;
+
   if (PT->isRecordType())
     return RecordKernelParam;
 
   return ValidKernelParam;
 }
 
 static void checkIsValidOpenCLKernelParameter(
   Sema &S,
if (NamedDecl *ExternCPrev = findLocallyScopedExternCDecl(&II)) {
     Diag(ExternCPrev->getLocation(), diag::note_previous_declaration);
     return ExternCPrev;
   }
 
   // Extension in C99.  Legal in C90, but warn about it.
   unsigned diag_id;
   if (II.getName().startswith("__builtin_"))
     diag_id = diag::warn_builtin_unknown;
+  else if (getLangOpts().OpenCL)
+    // OpenCL function need to be called with prototype, so we don't allow
+    // implicit function declarations in OpenCL
+    diag_id = diag::err_opencl_implicit_function_decl;
   else if (getLangOpts().C99)
     diag_id = diag::ext_implicit_function_decl;
   else
     diag_id = diag::warn_implicit_function_decl;
   Diag(Loc, diag_id) << &II;
 
   // Because typo correction is expensive, only do it if the implicit
   // function declaration is going to be treated as an 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 - The ATOMIC_VAR_INIT macro expands to a token
+  // sequence suitable for initializing an atomic object of a type that is
+  // initialization-compatible with value. An atomic object with automatic
+  // storage duration that is not explicitly initialized using ATOMIC_VAR_INIT
+  // is initially in an indeterminate state; however, the default (zero)
+  // initialization for objects with static storage duration is guaranteed to
+  // produce a valid state.
+  // #define ATOMIC_VAR_INIT(C value)
+  // This macro can only be used to initialize atomic objects that are declared
+  // in program scope in the global address space.
+  // Examples:
+  // global atomic_int guide = ATOMIC_VAR_INIT(42);
+  if (S.getLangOpts().OpenCL && S.getLangOpts().OpenCLVersion >= 200 &&
+      Entity.getType()->isAtomicType()) {
+    Qualifiers TyQualifiers = Entity.getType().getQualifiers();
+    bool HasGlobalAS = TyQualifiers.hasAddressSpace() &&
+                       TyQualifiers.getAddressSpace() == LangAS::opencl_global;
+    if (!HasGlobalAS && Entity.getKind() == InitializedEntity::EK_Variable &&
+        Args.size() > 0) {
+      Expr *Init = Args[0];
+      S.Diag(Init->getLocStart(), diag::err_opencl_atomic_init_addressspace)
+          << SourceRange(Entity.getDecl()->getLocStart(), Init->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)) {
     Expr *Init = Args[0];
     Expr::LValueClassification Kind = Init->ClassifyLValue(S.Context);
     if (Kind == Expr::LV_ClassTemporary || Kind == Expr::LV_ArrayTemporary)

test/Parser/opencl-atomics-cl20.cl

 // expected-error-re@-30 {{use of type 'atomic_uintptr_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be enabled}}
 // expected-error-re@-30 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
 // expected-error-re@-31 {{use of type 'atomic_size_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be enabled}}
 // expected-error-re@-31 {{use of type 'atomic_ptrdiff_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_base_atomics extension to be enabled}}
 // expected-error-re@-32 {{use of type 'atomic_ptrdiff_t' (aka '_Atomic({{.+}})') requires cl_khr_int64_extended_atomics extension to be enabled}}
 #endif
 
 #ifdef CL20
+#define ATOMIC_VAR_INIT
 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 {{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')}}
 }
+void atomic_init_test() {
+    atomic_int guide = ATOMIC_VAR_INIT(42); // expected-error {{initialization of atomic variables is restricted to variables in global address space in opencl}}
+}
 #endif

test/SemaOpenCL/invalid-decl.cl

+// RUN: %clang_cc1 -verify -cl-std=CL2.0 %s
+int; // expected-error {{declaration does not declare anything}}
+
+void test1(){
+  myfun(); // expected-error {{implicit declaration of function 'myfun' is invalid in OpenCL}}
+}