[SYCL] Correct CFE behavior with named lambdas in unnamed-lambda mode #3990
Conversation
The library is responsible to replace a non-named kernel's name with the type of the lambda. However, it still allows the user to specify the name if they'd like, and uses that as the name instead. The current implementation ALWAYS marked the kernel functor as the name of the kernel, but this isn't correct, it isn't the name of the kernel unless the library makes it so. This ends up causing user-named kernels that have their names changed by the functor they pass in. We can't mark the first template parameter, since it might be a user provided name, and we can't mark the functor since the user might have provided a name. However, if they are both the same (after unqualification and canonicalization) that means that the library has done such a replacement, and thus needs to be marked.
erichkeane
requested review from
AaronBallman,
elizabethandrews and
premanandrao
as code owners
Co-authored-by: premanandrao <[email protected]>
A version of intel#3971 set up so that hopefully CI can tell us if this approach is acceptable.
Moved call to `__builtin_sycl_mark_kernel_name` into `get_kernel_name_t` for both named and unnamed lambda paths. Added diagnostic (through `static_assert`) which disallows using unnamed lambdas without specifying corresponding compiler flag. Some tests were updated because they use unnamed lambdas without specifying corresponding compiler option. TODO: we might want to update tests to increase coverage, i.e. to launch some tests with both named and unnamed lambdas and also add a test for diagnostic message about unnamed lambda without compiler flag.
erichkeane
requested review from
bader,
DenisBakhvalov,
kbobrovs and
pvchupin
as code owners
|
@v-klochkov : Pushed b1df7ea to test my hypothesis. |
@erichkeane - thank you for fixing that. The fix though needs some additional changes as it does not changes the class __sycl_reduction_main_kernel, which now has <typename, bool, bool, typename> and the proposed fix added an additional typename into that template-arg-list making it <typename, typename, bool, bool, typename>. |
Ah, thanks! I didn't really get it building last night apparently, but I'll work to correct it today. |
|
Whew! This finally got through testing :) Please review it if you can! |
|
/summary:run |
clang/lib/Sema/SemaSYCL.cpp
Outdated
| QualType FunctorTy = GetSYCLKernelObjectType(FD); | ||
| QualType TmplArgTy = | ||
| calculateKernelNameType(SemaRef.Context, FD).getUnqualifiedType(); |
There was a problem hiding this comment.
It's a little bit odd that one of these functions returns an unqualified type while the other one returns a possibly qualified type. Should calculateKernelNameType() always return an unqualified type instead? Or are there times when that qualification is important for the kernel name?
There was a problem hiding this comment.
Also, can you double-check that we have tests for qualified kernel names and that we get the proper instantiations out of them?
There was a problem hiding this comment.
I think actually the unqualification here is wrong, we only want it in unqualified mode since we want to differentiate between:
kernel<Foo>(...);
and
kernel<const Foo>(...);
So I'll remove the unqualification here. As far as tests, I don't see any CFE tests, but I'm sure there has got to be one in at least one of the test suites SOMEWHERE.
There was a problem hiding this comment.
We've thought that in the past :-D Can you make sure we have some test coverage for the case you mention? That seems like something we'd want to explicitly document as not naming the same kernel.
|
/summary:run |
|
The precommit test failure smells unrelated, but it'd be nice to rerun the tests to make sure it goes back to green. |
In unnamed-lambda mode, we currently treat ALL kernels as using the
unnamed-lambda feature to the language, which is wrong. Named lambdas
should still work like they did in SYCL2017, and the unnamed lambda functionality
only applies when the user omits the name.
The library is responsible to replace a non-named kernel's name with the
type of the lambda. However, it still allows the user to specify the
name if they'd like, and uses that as the name instead.
Because of this, we can identify the unnamed lambda by when the first
template argument and the functor are the same unqualified type, meaning
the library has done the above replacement.
There are a couple of features in this patch that all need to work together/
be merged together since they all break eachother.
First is marking of the kernel functors. The current implementation ALWAYS
marked the kernel functor as the name
of the kernel, but this isn't correct, it isn't the name of the kernel
unless the library makes it so. This ends up causing user-named kernels
that have their names changed by the functor they pass in.
Second is the 'default' value of LangOpts.SYCLUnnamedLambdas. When not
specified, we should be in unnamed-lambda mode if we are in SYCL2020. This
patch also corrects that, PLUS adds an -fno flag for the same thing. This also
required fixing up how -sycl-std is handled (since it isn't quite a lang-standard,
and can't be treated directly as a lang-arg, since others can depend on it).
Third is SemaSYCL Sema checking. We need to still check bad kernel names when
in unnamed-lambda mode if the kernel name is user provided.
Forth is the Integration header. There is an 'unnamed lambda' format for it and a
'normal' format for it. We need to make sure we generate the correct one for each
type of functor.