[C++4OpenCL] Add missing OpenCL specific diagnostics in templates

[olestrohm]

Summary

For templates diagnosis must be done after specialization instead of during parsing.

I've left the check during parsing as well, because it provides slightly better error messages, though some are lost now that NewVD is set as invalid, instead of just setting D invalid.

This fixes: https://bugs.llvm.org/show_bug.cgi?id=48887

[Anastasia]

I've left the check during parsing as well, because it provides slightly better error messages, though some are lost now that NewVD is set as invalid, instead of just setting D invalid.

Yes, when there are multiple errors we don't provide the diagnostics consistently so I think it's fine.

clang/lib/Sema/SemaDecl.cpp

6827

I find this function a bit confusing I would just move its body inline where it is currently called.

Although actually could this code be moved to where we do similar check for static or extern
https://clang.llvm.org/doxygen/SemaDecl_8cpp_source.html#l07911

You would then need to change this slightly i.e. to call VarDecl::getTSCSpec instead.

clang/test/SemaOpenCLCXX/template-diagnostics.clcpp

27 ↗

(On Diff #338876)

Let's move this into the OpenCL C test for clk_event_t diagnostic test/SemaOpenCL/clk_event_t.cl. I would add a FIXME comment next to it stating that it is not clear whether this should be diagnosed or not.

[olestrohm]

Inlined the thread_local check and moved static clk_event_t into the appropriate test.
I did not move the thread_local check since NewVD had the wrong value for TSCS, so D was required.

[olestrohm]

Ran git-clang-format.

[Anastasia]

LGTM, let's just rename template-diagnostics.clcpp into something like template-opencl-types.clcpp.

[olestrohm]

Renamed test as suggested.

[svenvh]

I've left the check during parsing as well, because it provides slightly better error messages

Do you have an example of what error messages get worse if you remove the parsing check? Just wondering if there is an easy way to fix that and avoid having the check twice.

[olestrohm]

The only ones that change (in the test cases at least) are as follows:
Here Old is with the current change, and New is with the call to diagnoseOpenCLTypes at parsing removed.

In event_t.cl, event_t glb_evt; in program scope has this difference:

Old:
  the '__private event_t' type cannot be used to declare a program scope variable
New:
  program scope variable must reside in constant address space

In template-opencl-types.clcpp, __local event_t e; inside a function has this difference:

Old:
  the event_t type can only be used with __private address space qualifier
New:
  non-kernel function variable cannot be declared in local address space

The first on explains that event_t can't be used in program scope at all, which is better I think.
However the second error actually gets slightly better.

I think these errors are primarily emitted between the two checks, so I don't know if it's easy to put the check somewhere that gives better results.

[svenvh]

In D100860#2709057, @olestrohm wrote:

The first on explains that event_t can't be used in program scope at all, which is better I think.

Agreed, removing the parser check gives a worse diagnostic here.

The underlying problem might actually be that the diagnostic for program scope variables is firing too early. I tried to address a similar issue with D53705, which got stalled unfortunately. Perhaps we should revisit that and try to see if postponing the program scope variable diagnostic allows us to remove the parser check without getting a worse diagnostic.

[olestrohm]

I noticed that I could move the new check above the diagnostic for program scope variables, and the check during parsing can then be removed, while maintaining good diagnostics.

This does change the diagnostic for event_t mentioned above, however it's not a bad diagnostic, and actually matches what happens in non-kernel functions, so I think it's okay.
I would prefer the previous diagnostic for event_t, but that originates in the other branch of the same if statement as the program scope diagnostic, so it's not easy to separate them out and then insert the new check between them.

[svenvh]

LGTM, thanks!

Please make sure that the commit message corresponds to the new shape of your patch: you might want to update the description of this review.