[OpenCL] Diagnose conflicting address spaces between template definition and its instantiation

[Anastasia]

Summary

Added new diagnostics when templates are instantiated with different address space from the one provided in definition.

This also prevents deducing generic address space in pointer type of templates to allow giving them concrete address space.

[rjmccall]

lib/Sema/SemaType.cpp

7235

I don't understand what you're trying to do here. Template arguments may need to be directly qualified with an address-space sometimes, and if you prevent that unconditionally you're going to break all sorts of things, like partially-specializing a template based on the presence of an address-space qualifier.

[Anastasia]

lib/Sema/SemaType.cpp

7235

I want to prevent deduction of address spaces for template arguments (when they are not specified explicitly).

Without this change, this example won't compile:

template <typename T>
void foo() {
  static __global T i;
}
foo<int>(); // error because int here is deduced to __private int (so i will have conflicting addr space quals)

But I think it's perfectly reasonable to compile this example because the addr space qual of i is specified to be __global.

Basically I am just trying to fix OpenCL C deduction rules that didn't account for the logic of templates.

[rjmccall]

Okay. What you're saying is that the current OpenCL C++ compiler is implicitly adding an address-space qualifier at the top level to explicit template arguments. I agree that it should not be doing that.

Can you explain why the changes to TreeTransform are required?

[Anastasia]

Can you explain why the changes to TreeTransform are required?

When address spaces of template parameter and argument are given explicitly they might mismatch. For example, foo1 (in the test) is instantiated with __local int but the template parameter of foo1 is __global T. Currently clang fails to compile this with ICE while rebuilding the type that has multiple inconsistent addr space qualifiers. My patch fixes it, by giving an error instead of attempting to rebuild the type, that the addr space qualifiers mismatch.

There is another example of the same issue - foo3, but with non-pointer type.

[rjmccall]

LGTM with minor revisions.

lib/Sema/SemaType.cpp

7206

I think a better comment here would be something like "Don't deduce address spaces for dependent types because they might end up instantiating to a type with a different address space qualifier."

7235

Okay. Please use braces instead of a ;.

[Anastasia]

Added last corrections before committing.

include/clang/Basic/DiagnosticSemaKinds.td

 def err_as_qualified_auto_decl : Error<
   "automatic variable qualified with an%select{| invalid}0 address space">;
 def err_arg_with_address_space : Error<
   "parameter may not be qualified with an address space">;
 def err_field_with_address_space : Error<
   "field may not be qualified with an address space">;
 def err_compound_literal_with_address_space : Error<
   "compound literal in function scope may not be qualified with an address space">;
+def err_address_space_mismatch_templ_inst : Error<
+  "conflicting address space qualifiers are provided between types %0 and %1">;
 def err_attr_objc_ownership_redundant : Error<
   "the type %0 is already explicitly ownership-qualified">;
 def err_invalid_nsnumber_type : Error<
   "%0 is not a valid literal type for NSNumber">;
 def err_objc_illegal_boxed_expression_type : Error<
   "illegal type %0 used in a boxed expression">;
 def err_objc_non_trivially_copyable_boxed_expression_type : Error<
   "non-trivially copyable type %0 cannot be used in a boxed expression">;

lib/Sema/SemaType.cpp

if ( // Do not deduce addr space for function return type and function type,
       (D.getContext() == DeclaratorContext::MemberContext && !IsPointee) ||
       // Do not deduce addr space for types used to define a typedef and the
       // typedef itself, except the pointee type of a pointer type which is used
       // to define the typedef.
       (D.getDeclSpec().getStorageClassSpec() == DeclSpec::SCS_typedef &&
        !IsPointee) ||
       // Do not deduce addr space of the void type, e.g. in f(void), otherwise
       // it will fail some sema check.
-      (T->isVoidType() && !IsPointee))
+      (T->isVoidType() && !IsPointee) ||
+      // Do not deduce address spaces for dependent types because they might end
+      // up instantiating to a type with an explicit address space qualifier.
+      T->isDependentType())
     return;
 
   LangAS ImpAddr = LangAS::Default;
   // Put OpenCL automatic variable in private address space.
   // OpenCL v1.2 s6.5:
   // The default address space name for arguments to a function in a
   // program, or local variables of a function is __private. All function
   // arguments shall be in the __private address space.
   if (State.getSema().getLangOpts().OpenCLVersion <= 120 &&
       !State.getSema().getLangOpts().OpenCLCPlusPlus) {
     ImpAddr = LangAS::opencl_private;
   } else {
     // If address space is not set, OpenCL 2.0 defines non private default
     // address spaces for some cases:
     // OpenCL 2.0, section 6.5:
     // The address space for a variable at program scope or a static variable
     // inside a function can either be __global or __constant, but defaults to
     // __global if not specified.
     // (...)
     // Pointers that are declared without pointing to a named address space
     // point to the generic address space.
     if (IsPointee) {
       ImpAddr = LangAS::opencl_generic;
     } else {
-      if (D.getContext() == DeclaratorContext::TemplateArgContext ||
-          T->isDependentType()) {
-        // Do not deduce address space for non-pointee type in templates.
+      if (D.getContext() == DeclaratorContext::TemplateArgContext) {
+        // Do not deduce address space for non-pointee type in template arg.
       } else if (D.getContext() == DeclaratorContext::FileContext) {
         ImpAddr = LangAS::opencl_global;
       } else {
         if (D.getDeclSpec().getStorageClassSpec() == DeclSpec::SCS_static ||
             D.getDeclSpec().getStorageClassSpec() == DeclSpec::SCS_extern) {
           ImpAddr = LangAS::opencl_global;
         } else {
           ImpAddr = LangAS::opencl_private;
         }
       }

lib/Sema/TreeTransform.h

 #define ABSTRACT_STMT(Stmt)
 #include "clang/AST/StmtNodes.inc"
 
 #define OPENMP_CLAUSE(Name, Class)                        \
   LLVM_ATTRIBUTE_NOINLINE \
   OMPClause *Transform ## Class(Class *S);
 #include "clang/Basic/OpenMPKinds.def"
 
-  /// Build a new qualified type given its unqualified type and type
-  /// qualifiers.
+  /// Build a new qualified type given its unqualified type and type location.
   ///
   /// By default, this routine adds type qualifiers only to types that can
   /// have qualifiers, and silently suppresses those qualifiers that are not
   /// permitted. Subclasses may override this routine to provide different
   /// behavior.
-  QualType RebuildQualifiedType(QualType T, SourceLocation Loc,
-                                Qualifiers Quals);
+  QualType RebuildQualifiedType(QualType T, QualifiedTypeLoc TL);
 
   /// Build a new pointer type given its pointee type.
   ///
   /// By default, performs semantic analysis when building the pointer type.
   /// Subclasses may override this routine to provide different behavior.
   QualType RebuildPointerType(QualType PointeeType, SourceLocation Sigil);
 
   /// Build a new block pointer type given its pointee type.
TreeTransform<Derived>::TransformTypeWithDeducedTST(TypeSourceInfo *DI) {
   auto DNTL = TL.castAs<DependentNameTypeLoc>();
 
   QualType Result = getDerived().TransformDependentNameType(
       TLB, DNTL, /*DeducedTSTContext*/true);
   if (Result.isNull())
     return nullptr;
 
   if (QTL) {
-    Result = getDerived().RebuildQualifiedType(
-        Result, QTL.getBeginLoc(), QTL.getType().getLocalQualifiers());
+    Result = getDerived().RebuildQualifiedType(Result, QTL);
+    if (Result.isNull())
+      return nullptr;
     TLB.TypeWasModifiedSafely(Result);
   }
 
   return TLB.getTypeSourceInfo(SemaRef.Context, Result);
 }
 
 template<typename Derived>
 QualType
 TreeTransform<Derived>::TransformQualifiedType(TypeLocBuilder &TLB,
                                                QualifiedTypeLoc T) {
   Qualifiers Quals = T.getType().getLocalQualifiers();
 
   QualType Result = getDerived().TransformType(TLB, T.getUnqualifiedLoc());
   if (Result.isNull())
     return QualType();
 
-  Result = getDerived().RebuildQualifiedType(Result, T.getBeginLoc(), Quals);
+  Result = getDerived().RebuildQualifiedType(Result, T);
+
+  if (Result.isNull())
+    return QualType();
 
   // RebuildQualifiedType might have updated the type, but not in a way
   // that invalidates the TypeLoc. (There's no location information for
   // qualifiers.)
   TLB.TypeWasModifiedSafely(Result);
 
   return Result;
 }
 
-template<typename Derived>
+template <typename Derived>
 QualType TreeTransform<Derived>::RebuildQualifiedType(QualType T,
-                                                      SourceLocation Loc,
-                                                      Qualifiers Quals) {
+                                                      QualifiedTypeLoc TL) {
+
+  SourceLocation Loc = TL.getBeginLoc();
+  Qualifiers Quals = TL.getType().getLocalQualifiers();
+
+  if (((T.getAddressSpace() != LangAS::Default &&
+        Quals.getAddressSpace() != LangAS::Default)) &&
+      T.getAddressSpace() != Quals.getAddressSpace()) {
+    SemaRef.Diag(Loc, diag::err_address_space_mismatch_templ_inst)
+        << TL.getType() << T;
+    return QualType();
+  }
+
   // C++ [dcl.fct]p7:
   //   [When] adding cv-qualifications on top of the function type [...] the
   //   cv-qualifiers are ignored.
   if (T->isFunctionType())
     return T;
 
   // C++ [dcl.ref]p1:
   //   when the cv-qualifiers are introduced through the use of a typedef-name

test/SemaOpenCLCXX/address-space-templates.cl

 //RUN: %clang_cc1 %s -cl-std=c++ -pedantic -verify -fsyntax-only
 
 template <typename T>
 struct S {
   T a;        // expected-error{{field may not be qualified with an address space}}
   T f1();     // expected-error{{function type may not be qualified with an address space}}
   void f2(T); // expected-error{{parameter may not be qualified with an address space}}
 };
 
+template <typename T>
+T foo1(__global T *i) { // expected-note{{candidate template ignored: substitution failure [with T = __local int]: conflicting address space qualifiers are provided between types '__global T' and '__local int'}}
+  return *i;
+}
+
+template <typename T>
+T *foo2(T *i) {
+  return i;
+}
+
+template <typename T>
+void foo3() {
+  __private T ii; // expected-error{{conflicting address space qualifiers are provided between types 'T' and '__global int'}}
+}
+
 void bar() {
   S<const __global int> sintgl; // expected-note{{in instantiation of template class 'S<const __global int>' requested here}}
+
+  foo1<__local int>(1); // expected-error{{no matching function for call to 'foo1'}}
+  foo2<__global int>(0);
+  foo3<__global int>(); // expected-note{{in instantiation of function template specialization 'foo3<__global int>' requested here}}
 }