[PATCH] D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: svenvh, olestrohm, rjmccall.
Herald added subscribers: Naghasan, ebevhan, yaxunl.
Herald added a project: All.
Anastasia requested review of this revision.

It seems relying on `isStandardLayoutType` helpers is fragile when kernel arguments are references/pointers to templated types.

Clang uses lazy template instantiation in this case and therefore types are not fully instantiated when those are used.

Example:

  template<typename T>
  struct outer{
  public:
      struct inner{
          int size;
      };
  };
  void foo(outer<int>::inner& p)              
  {        
  } 

will have the following AST when p is a reference:

  | `-ClassTemplateSpecializationDecl <line:1:1, line:7:1> line:2:8 struct outer definition
  |   |-DefinitionData pass_in_registers empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
  |   | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
  |   | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  |   | |-MoveConstructor exists simple trivial needs_implicit
  |   | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
  |   | |-MoveAssignment exists simple trivial needs_implicit
  |   | `-Destructor simple irrelevant trivial needs_implicit
  |   |-TemplateArgument type 'int'
  |   | `-BuiltinType 'int'
  |   |-CXXRecordDecl <col:1, col:8> col:8 implicit struct outer
  |   |-AccessSpecDecl <line:3:1, col:7> col:1 public
  |   `-CXXRecordDecl <line:4:5, col:12> col:12 referenced struct inner

and when it is not a reference

  | `-ClassTemplateSpecializationDecl <line:1:1, line:7:1> line:2:8 struct outer definition
  |   |-DefinitionData pass_in_registers empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
  |   | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
  |   | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  |   | |-MoveConstructor exists simple trivial needs_implicit
  |   | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
  |   | |-MoveAssignment exists simple trivial needs_implicit
  |   | `-Destructor simple irrelevant trivial needs_implicit
  |   |-TemplateArgument type 'int'
  |   | `-BuiltinType 'int'
  |   |-CXXRecordDecl <col:1, col:8> col:8 implicit struct outer
  |   |-AccessSpecDecl <line:3:1, col:7> col:1 public
  |   `-CXXRecordDecl <line:4:5, line:6:5> line:4:12 referenced struct inner definition
  |     |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
  |     | |-DefaultConstructor exists trivial needs_implicit
  |     | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  |     | |-MoveConstructor exists simple trivial needs_implicit
  |     | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
  |     | |-MoveAssignment exists simple trivial needs_implicit
  |     | `-Destructor simple irrelevant trivial needs_implicit
  |     |-CXXRecordDecl <col:5, col:12> col:12 implicit struct inner
  |     `-FieldDecl <line:5:9, col:13> col:13 size 'int'

In the first case for reference type `inner` is incomplete while if we use a non-reference type the definition of template specialization is complete.

I have attempted to workaround this issue for the reported test cases, however it still doesn't quite work when the type is created from the template parameter (see FIXME test case in the patch). I presume we want to allow this? If so we might need to disable lazy template instantiation in this case. My guess is the only issue this this is that we will have performance penalty for the code of this format:

  template<class T> struct Dummy {
  T i;
  };
  
  extern kernel void foo(Dummy<int>& i);

which doesn't technically need the full instantiation.

Also I have discovered that in OpenCL C we have allowed passing kernel parameters with pointers to forward declared structs, but in C++ for OpenCL that no longer compiles. Not sure whether we should keep this restriction but if not it might interfere with the idea of always instantiating the structs definitions fully and even providing the safe diagnostics. So another approach we could take is to change this diagnostics into warnings and then if we can't fully detect the type provide the messaging that we can't detect whether the type is safe...

Another aspect to consider is that we have an extension that allows disabling all of this safe checks completely: https://clang.llvm.org/docs/LanguageExtensions.html#cl-clang-non-portable-kernel-param-types

While this matter might need more thoughts and investigations I wonder whether it makes sense to commit this fix for the time being since it's fixing the reported test case at least?


https://reviews.llvm.org/D134445

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaOpenCLCXX/invalid-kernel.clcpp


Index: clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
===================================================================
--- clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
+++ clang/test/SemaOpenCLCXX/invalid-kernel.clcpp
@@ -93,3 +93,24 @@
 #ifndef UNSAFEKERNELPARAMETER
 //expected-error at -2{{'__global Trivial &__private' cannot be used as the type of a kernel parameter}}
 #endif
+
+// Nested types and templates
+struct Outer {
+struct Inner{
+int i;
+};
+};
+template<class T>
+struct OuterTempl {
+struct Inner{
+int i;
+};
+};
+
+// FIXME: Use of templates doesn't work due to lazy instantiation of reference types. 
+//template<class T>
+//struct Templ {
+//T i;
+//};
+
+extern kernel void nested(constant Outer::Inner& r1, constant OuterTempl<int>::Inner& r2/*, constant Templ<int>& r3*/);
Index: clang/lib/Sema/SemaDecl.cpp
===================================================================
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -8849,10 +8849,19 @@
     // reference if an implementation supports them in kernel parameters.
     if (S.getLangOpts().OpenCLCPlusPlus &&
         !S.getOpenCLOptions().isAvailableOption(
-            "__cl_clang_non_portable_kernel_param_types", S.getLangOpts()) &&
-        !PointeeType->isAtomicType() && !PointeeType->isVoidType() &&
-        !PointeeType->isStandardLayoutType())
+            "__cl_clang_non_portable_kernel_param_types", S.getLangOpts())) {
+     auto CXXRec = PointeeType.getCanonicalType()->getAsCXXRecordDecl();
+     bool IsStandardLayoutType = true;
+     if (CXXRec) {
+       if (!CXXRec->hasDefinition())
+         CXXRec = CXXRec->getTemplateInstantiationPattern();
+       if (!CXXRec || !CXXRec->hasDefinition() || !CXXRec->isStandardLayout())
+         IsStandardLayoutType = false;
+     }
+     if (!PointeeType->isAtomicType() && !PointeeType->isVoidType() &&
+        !IsStandardLayoutType)
       return InvalidKernelParam;
+    }
 
     return PtrKernelParam;
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D134445.462178.patch
Type: text/x-patch
Size: 1977 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220922/fa8ccfdf/attachment-0001.bin>