-
Notifications
You must be signed in to change notification settings - Fork 798
[SYCL] add clang diagnostic for illegal types of kernel free function arguments #19244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: sycl
Are you sure you want to change the base?
[SYCL] add clang diagnostic for illegal types of kernel free function arguments #19244
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the free function spec not allow such kernel arguments? At least regarding virtual functions AFAIK in SYCL 2020 such an object can be captured to the device code and is fine unless a virtual function is called.
See
https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:language.restrictions.kernels
The odr-use of polymorphic classes and classes with virtual inheritance is allowed. However, no virtual member functions are allowed to be called in a device function.
@Fznamznon kernel free function docs tell that: i.e. S is allowed only if it has non-virtual inheritance from allowed type which means virtual inheritance is not allowed, does not it? |
@dklochkov-emb , yeah, that vaguely reads that virtual bases are not allowed in kernel arguments. I mailed spec folks to double check. Two concerns though:
|
ac5c678
to
dedadc1
Compare
clang/lib/Sema/SemaSYCL.cpp
Outdated
@@ -1569,6 +1576,27 @@ class KernelObjVisitor { | |||
else if (ParamTy->isStructureOrClassType()) { | |||
if (KP_FOR_EACH(handleStructType, Param, ParamTy)) { | |||
CXXRecordDecl *RD = ParamTy->getAsCXXRecordDecl(); | |||
llvm::SmallVector<const CXXRecordDecl *, 8> WorkList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is not exactly what I meant about using visitors infrastructure. What I meant is that we should extend SyclKernelFieldChecker
class, so it checks bases. It has methods that accept CXXBaseSpecifier
AST node which represent base specifier. Each CXXBaseSpecifier
knows whether it is virtual. I think SyclKernelFieldChecker
should be already recursively visiting each struct it meets. We just need to diagnose from some of method of SyclKernelFieldChecker
that accepts CXXBaseSpecifier
. Because this is not even specific for free function kernels, you don't even need to invent a method specific for free function kernels and accepting CXXBaseSpecifier
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this one is resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in sycl/test-e2e/FreeFunctionKernels/virtual_methods.cpp LGTM.
@dpcpp-cfe-reviewers please, look at PR - @Fznamznon is on vacation until 28 July. |
@dpcpp-cfe-reviewers please, look at PR |
See #19244 (comment) |
This PR adds clang diagnostic message in case if:
Also, tests were added according to test plan