This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by olestrohm on Apr 20 2021, 8:28 AM.

Details

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

Diff Detail

Event Timeline

olestrohm created this revision.Apr 20 2021, 8:28 AM
olestrohm requested review of this revision.Apr 20 2021, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2021, 8:28 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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
6825

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 updated this revision to Diff 339175.Apr 21 2021, 4:09 AM

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 updated this revision to Diff 339197.Apr 21 2021, 5:31 AM

Ran git-clang-format.

Anastasia accepted this revision.Apr 21 2021, 6:34 AM

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

This revision is now accepted and ready to land.Apr 21 2021, 6:34 AM
olestrohm updated this revision to Diff 339251.Apr 21 2021, 8:38 AM

Renamed test as suggested.

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.

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.

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 updated this revision to Diff 339976.Apr 23 2021, 4:24 AM

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 accepted this revision.Apr 23 2021, 7:56 AM

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.