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

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

> 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:

I don't have enough experience with lazy template instantiation to give meaningful advice here.  Though I'm not too worried about performance penalties for the example you're giving, as I'd expect most realistic use cases will require the full instantiation of a templated type sooner or later in the source program anyway (e.g. near the calling point).

> 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?

That sounds reasonable to me; clang currently hard-rejects a valid source due to an over-conservative diagnostic which isn't a great user experience.

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

I think that makes sense, and should be fine to leave as a followup.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:8856
+     if (CXXRec) {
+       if (!CXXRec->hasDefinition())
+         CXXRec = CXXRec->getTemplateInstantiationPattern();
----------------
A comment explaining what we're trying to do here would be nice.


================
Comment at: clang/test/SemaOpenCLCXX/invalid-kernel.clcpp:99
+struct Outer {
+struct Inner{
+int i;
----------------
Indenting would help readability.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134445/new/

https://reviews.llvm.org/D134445