[OpenCL] Improve diagnostics for address spaces in template instantiation

[Anastasia]

Summary

When template instantiation leads to creation of invalid code patterns with address spaces compiler crashes with ICE.

This patch fixes the template instantiation to provide correct diagnostics instead.

[Anastasia]

Removed FIXME from the test case that is being fixed.

[rjmccall]

lib/Sema/TreeTransform.h

4241

When do you actually add the qualifier back?

Also, I don't think this is specific to either OpenCL or direct references to template type parameters; it has to be any dependent type.

[Anastasia]

lib/Sema/TreeTransform.h

4241

As far as I understand the qualifiers here are only used to rebuild the type. Therefore I assumed I don't need to restore the original. I am now thinking of moving this down into RebuildQualifiedType that has similar code for handling qualifiers.

However, after enabling it for C++ in general I am getting some issues with deduction of templates with pointers that have address spaces in pointees. So I am investigating this further whether it's the right approach at all.

[Anastasia]

• Changing qualifiers while rebuilding types in TreeTransform is incorrect -> prevent deducing address space instead.

[Anastasia]

lib/Sema/TreeTransform.h

4241

I think indeed this was wrong! Instead I now just prevent deduction of address spaces for dependent types. I think this is the only use case. So I don't think we need to do anything here any more.

[bader]

lib/Sema/TreeTransform.h

5285

"cann't" - typo?
cann't - > can't or cannot.

test/CodeGenOpenCLCXX/template-address-spaces.cl

28

I think it should comment should also be removed.

[Anastasia]

Corrected a typo in comments.

[Anastasia]

Removed commented code in the test.

[Anastasia]

lib/Sema/TreeTransform.h

5285

Thanks!

test/CodeGenOpenCLCXX/template-address-spaces.cl

28

Good spot!

[rjmccall]

LGTM.

lib/Sema/SemaDecl.cpp

if (getLangOpts().CPlusPlus) {
     if (DiagnoseUnexpandedParameterPack(D.getIdentifierLoc(), TInfo,
                                         UPPC_DataMemberType)) {
       D.setInvalidType();
       T = Context.IntTy;
       TInfo = Context.getTrivialTypeSourceInfo(T, Loc);
     }
   }
 
-  // TR 18037 does not allow fields to be declared with address spaces.
-  if (T.getQualifiers().hasAddressSpace() ||
-      T->isDependentAddressSpaceType() ||
-      T->getBaseElementTypeUnsafe()->isDependentAddressSpaceType()) {
-    Diag(Loc, diag::err_field_with_address_space);
-    D.setInvalidType();
-  }
-
-  // OpenCL v1.2 s6.9b,r & OpenCL v2.0 s6.12.5 - The following types cannot be
-  // used as structure or union field: image, sampler, event or block types.
-  if (LangOpts.OpenCL && (T->isEventT() || T->isImageType() ||
-                          T->isSamplerT() || T->isBlockPointerType())) {
-    Diag(Loc, diag::err_opencl_type_struct_or_union_field) << T;
-    D.setInvalidType();
-  }
-
   DiagnoseFunctionSpecifiers(D.getDeclSpec());
 
   if (D.getDeclSpec().isInlineSpecified())
     Diag(D.getDeclSpec().getInlineSpecLoc(), diag::err_inline_non_function)
         << getLangOpts().CPlusPlus17;
   if (DeclSpec::TSCS TSCS = D.getDeclSpec().getThreadStorageClassSpec())
     Diag(D.getDeclSpec().getThreadStorageClassSpecLoc(),
          diag::err_invalid_thread)
if (RequireCompleteType(Loc, EltTy, diag::err_field_incomplete)) {
       EltTy->isIncompleteType(&Def);
       if (Def && Def->isInvalidDecl()) {
         Record->setInvalidDecl();
         InvalidDecl = true;
       }
     }
   }
 
+  // TR 18037 does not allow fields to be declared with address space
+  if (T.getQualifiers().hasAddressSpace() || T->isDependentAddressSpaceType() ||
+      T->getBaseElementTypeUnsafe()->isDependentAddressSpaceType()) {
+    Diag(Loc, diag::err_field_with_address_space);
+    Record->setInvalidDecl();
+    InvalidDecl = true;
+  }
+
+  if (LangOpts.OpenCL) {
+    // OpenCL v1.2 s6.9b,r & OpenCL v2.0 s6.12.5 - The following types cannot be
+    // used as structure or union field: image, sampler, event or block types.
+    if (T->isEventT() || T->isImageType() || T->isSamplerT() ||
+        T->isBlockPointerType()) {
+      Diag(Loc, diag::err_opencl_type_struct_or_union_field) << T;
+      Record->setInvalidDecl();
+      InvalidDecl = true;
+    }
     // OpenCL v1.2 s6.9.c: bitfields are not supported.
-  if (BitWidth && getLangOpts().OpenCL) {
+    if (BitWidth) {
       Diag(Loc, diag::err_opencl_bitfields);
       InvalidDecl = true;
     }
+  }
 
   // Anonymous bit-fields cannot be cv-qualified (CWG 2229).
   if (!InvalidDecl && getLangOpts().CPlusPlus && !II && BitWidth &&
       T.hasQualifiers()) {
     InvalidDecl = true;
     Diag(Loc, diag::err_anon_bitfield_qualifiers);
   }
 

lib/Sema/TreeTransform.h

 }
 
 template<typename Derived>
 QualType
 TreeTransform<Derived>::TransformQualifiedType(TypeLocBuilder &TLB,
                                                QualifiedTypeLoc T) {
   Qualifiers Quals = T.getType().getLocalQualifiers();
 
+  // Template parameter types used as function parameters are deduced to
+  // private addr space in OpenCL just like regular function parameters.
+  // The instantiated template might be given different addr space,
+  // this is incorrect but we can only diagnose it once the type is created.
+  // Therefore, the addr space is ignored here so that the type creation
+  // succeeds.
+  if (SemaRef.getLangOpts().OpenCL && T.getType()->isTemplateTypeParmType())
+    Quals.removeAddressSpace();
+
   QualType Result = getDerived().TransformType(TLB, T.getUnqualifiedLoc());
   if (Result.isNull())
     return QualType();
 
   Result = getDerived().RebuildQualifiedType(Result, T.getBeginLoc(), Quals);
 
   // RebuildQualifiedType might have updated the type, but not in a way
   // that invalidates the TypeLoc. (There's no location information for
if (getDerived().TransformFunctionTypeParams(
         return QualType();
     }
   }
   else {
     ResultType = getDerived().TransformType(TLB, TL.getReturnLoc());
     if (ResultType.isNull())
       return QualType();
 
+    // Return type cann't be qualified with an address space.
+    if (ResultType.getAddressSpace() != LangAS::Default) {
+      SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),
+                   diag::err_attribute_address_function_type);
+      return QualType();
+    }
+
     if (getDerived().TransformFunctionTypeParams(
             TL.getBeginLoc(), TL.getParams(),
             TL.getTypePtr()->param_type_begin(),
             T->getExtParameterInfosOrNull(),
             ParamTypes, &ParamDecls, ExtParamInfos))
       return QualType();
   }
 

test/CodeGenOpenCLCXX/template-address-spaces.cl

 //CHECK: i32 @_ZN1SIiE3fooEv(%struct.S* %this)
 //CHECK: i32 addrspace(4)* @_ZN1SIPU3AS4iE3fooEv(%struct.S.0* %this)
 //CHECK: i32 addrspace(1)* @_ZN1SIPU3AS1iE3fooEv(%struct.S.1* %this)
 
 void bar(){
   S<int> sint;
   S<int*> sintptr;
   S<__global int*> sintptrgl;
-  // FIXME: Preserve AS in TreeTransform
-  //S<__global int> sintgl;
 
   sint.foo();
   sintptr.foo();
   sintptrgl.foo();
   //sintgl.foo();
 }

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}}
+};
+
+void bar() {
+  S<const __global int> sintgl; // expected-note{{in instantiation of template class 'S<const __global int>' requested here}}
+}