[SYCL] Improve kernel name diagnostic #45
Conversation
In case of conflicting SYCL kernel name, the compiler reported the location of the initial kernel but without any context. This lead to a poor diagnostic when used with template as the user had no information on how the initial kernel was instantiated. A second issue can arise with SFINAE as the name validation was done during overload resolution when the attribute is used on a templated function. This lead to the attribute interfering with the overload resolution. We now record the template instantiation trace when a SYCL entry point is registered. In case of conflicting name, the diagnostic is issue when emitting the SYCL kernel CallStmt and the trace is looked up to provide the full context.
| // this doesn't trigger any error. | ||
| sycl_entry_point<kernel_name>([]{}, l); |
There was a problem hiding this comment.
Would an error have been issued for this case prior to your changes? I can't test right now, but I suspect the answer is no. This call is an exact match for the second overload, so there isn't any reason for the first overload to have been selected anyway. I think the test cases above suffice to validate the SFINAE concerns since they verify that a "no matching function for call to ..." error is not issued.
There was a problem hiding this comment.
Would an error have been issued for this case prior to your changes?
Yes
so if I have these 2 calls:
int i = 42;
long l = 42;
sycl_entry_point<kernel_name>([]{}, i);
sycl_entry_point<kernel_name>([]{}, l);the compiler produces on the current target branch
error: 'sycl_kernel_entry_point' kernel name argument conflicts with a previous declaration
8 | void sycl_entry_point(K ker, int) {
| ^
note: in instantiation of function template specialization 'sycl_entry_point<kernel_name, (lambda at sycl-kernel-entry-point-attr-conflict.cpp:24:33)>' requested here
24 | sycl_entry_point<kernel_name>([]{}, l);
| ^
note: previous declaration is here
8 | void sycl_entry_point(K ker, int) {
| ^
1 error generated.
But no error should be generated as the second is to a function without the attribute.
There was a problem hiding this comment.
Oh, wow. I guess that implies that CheckSYCLEntryPointFunctionDecl() is getting called during overload resolution? Even for a previously instantiated declaration?
There was a problem hiding this comment.
yes, the caller of CheckSYCLEntryPointFunctionDecl is also used to check if template parameter substitution yield a valid decl and, in turn, allows to select viable functions for the overload resolution.
Even for a previously instantiated declaration?
I don't think so as the lookup should retrieve the decl rather than create a new one, but we probably should test that :).
Improve testing Refactor diagnostics of sycl_entry_point - move diagnostics into attr handler when possible - delay diagnostic if we are in a template instantiation context
…ate the sycl kernel call stmt
|
|
||
| diagnoseUnavailableAlignedAllocation(*cast<FunctionDecl>(D), Loc); | ||
| } | ||
| SYCL().EmitDelayedKernelEntryPointDiagnostics(D->getCanonicalDecl()); |
There was a problem hiding this comment.
I experimented a little. I wonder if we can simply call CheckSYCLEntryPointFunctionDecl() here without having to collect partial diagnostics. Take a look at https://godbolt.org/z/T1Pz5T6qf (and enjoy the implementation divergence while you're there!) The diagnostic issued for use of a deleted function isn't issued until the use occurs, but still includes the instantiation stack; see the code starting at line 271 below.
2 participants
In case of conflicting SYCL kernel name, the compiler reported the location of the initial kernel but without any context. This lead to a poor diagnostic when used with template as the user had no information on how the initial kernel was instantiated.
A second issue can arise with SFINAE as the name validation was done during overload resolution when the attribute is used on a templated function. This lead to the attribute interfering with the overload resolution.
We now record the template instantiation trace when a SYCL entry point is registered. In case of conflicting name, the diagnostic is issue when emitting the SYCL kernel CallStmt and the trace is looked up to provide the full context.