[SYCL] Force-emit more member functions into device code #13985
Conversation
We only emit unused member functions if they have been explicitly annotated with `sycl_device` attribute (through `SYCL_EXTERNAL` macro). SYCL extension for virtual functions introduces an alternative markup for specifying which function and that markup is SYCL compile-time properties that we turn into attributes implicitly under the hood. Essentially, we now have a situation where an implicit `sycl_device` attribute on a member function should be treated as an explicit one, because it could be a result of SYCL compile-time property being applied to that method.
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
clang/lib/AST/ASTContext.cpp
Outdated
| @@ -12037,8 +12037,12 @@ bool ASTContext::DeclMustBeEmitted(const Decl *D) { | |||
| if (LangOpts.SYCLIsDevice && isa<CXXMethodDecl>(D)) { | |||
| if (auto *A = D->getAttr<SYCLDeviceIndirectlyCallableAttr>()) | |||
| return !A->isImplicit(); | |||
| // Implicit 'sycl_device' attribute is treated as explicit one if method | |||
| // is also annotated with 'add_ir_attributes_function' attribute, because | |||
| // the latter can work as an alias for SYCL_EXTERNAL. | |||
| if (auto *A = D->getAttr<SYCLDeviceAttr>()) | |||
There was a problem hiding this comment.
Do you know why we have the Implicit check? Why don't we emit functions irrespective of whether the attribute is implicit or explicit?
There was a problem hiding this comment.
Do you know why we have the Implicit check? Why don't we emit functions irrespective of whether the attribute is implicit or explicit?
No, I don't. git blame just goes years back to some refactoring commit and there is not much info about why we specifically look for an explicit attribute (3baec18).
Looking at the clang codebase, there are only 3 places where we create implicit sycl_device attribute:
SemaLookup.cppwhen we generate declarations for SPIR-V friendly IR built-insSemaDeclAttr.cppwhen we handle legacy indirectly callable attribute we haveSemaDeclAttr.cppwhen we handleadd_ir_function_attributesattribute (my recent patch)
Let me try and just drop requirement for the attribute to be explicit and see if anything breaks.
There was a problem hiding this comment.
Both check-clang and check-sycl pass if I remove !A->isImplicit() check for sycl_device attribute. I still have no idea why it was introduced in the first place. Perhaps it is some kind of legacy? Before 3baec18 we used to add the attribute everywhere alongside call graph (implicitly) to indicate which functions should be emitted and which are not.
Considering that now there is only a few places where we implicitly add the attribute and only a couple of them is relevant to CXXMethodDecls, what do you think about just dropping the requirement for the attribute to be explicit?
There was a problem hiding this comment.
I think it makes sense dropping the isImplicit check. In the cases mentioned above it feels like we actually want the functions/builtins to be emitted? I don't know what the purpose of adding this attribute is, if we don't want to emit the function.
There was a problem hiding this comment.
Dropped the check in 5bcd485
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
|
By some reason, we used to only emit unused member functions if they are explicitly annotated with `sycl_device` attribute (through `SYCL_EXTERNAL` macro). This logic was introduced in 3baec18 and there is no clear indication as to why exactly we have a check that the attribute is explicit. SYCL extension for virtual functions introduces an alternative markup for specifying which function and that markup is SYCL compile-time properties that we turn into attributes implicitly under the hood. Essentially, we now have a situation where an implicit `sycl_device` attribute on a member function should be treated as an explicit one, because it could be a result of SYCL compile-time property being applied to that method. Considering our current codebase, it seems like we intend to have member function to be emitted in all cases where `sycl_device` is being implicitly added and therefore this patch removes the requirement for the attribute to be explicit.
We only emit unused member functions if they have been explicitly annotated with
sycl_deviceattribute (throughSYCL_EXTERNALmacro).SYCL extension for virtual functions introduces an alternative markup for specifying which function and that markup is SYCL compile-time properties that we turn into attributes implicitly under the hood.
Essentially, we now have a situation where an implicit
sycl_deviceattribute on a member function should be treated as an explicit one, because it could be a result of SYCL compile-time property being applied to that method.