Skip to content

[SYCL] Add support for structs containing special types as free function kernel parameters #18692

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

Open
wants to merge 57 commits into
base: sycl
Choose a base branch
from

Conversation

lbushi25
Copy link
Contributor

@lbushi25 lbushi25 commented May 28, 2025

Add support for structs containing special types as free function kernel parameters.

@lbushi25 lbushi25 changed the title [SYCL] Struct contains special types [SYCL] Add support for structs containing special types as free function kernel parameters May 28, 2025
@lbushi25
Copy link
Contributor Author

lbushi25 commented Aug 7, 2025

@intel/dpcpp-cfe-reviewers friendly ping for review

Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that a lot of additional info is gathered and stored which doesn't happen in SYCL2020 mode. I think we lack some additional modifications to visitors mechanism for free function kernels and we should extend that instead of storing additional data which is not stored for SYCL 2020 kernels.
I'm also fairly concerned with how much additional C++ code will be added to the integration header. C++ code generation and the whole integration header is proven to be heavily bug-prone. I'm not really sure generating the whole set_arg function is worth the complication and the maintenance burden. I think for SYCL RT it should be enough to know which parameter of generated spir_kernel function corresponds to which top-level parameter of the original free function kernel signature.
cc @tahonermann , @AlexeySachkov and @intel/llvm-reviewers-runtime for opinions. I'm also slightly concerned that the PR doesn't have passing E2E tests added for the case.

// A map that keeps track of all structs encountered with
// special types inside. Relevant for free function kernels only.
// The key stores the struct as a declaration and the value stores the struct
// as a QualType just to make it easier to use it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is that hard to obtain a type from a RecordDecl so we need to store it separately. Note there can be lots of kernels and lots of struct parameters within the same translation unit.

// TODO manipulate struct depth once special types are supported for free
// function kernels.
// ++StructFieldDepth;
++StructFieldDepth;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these used for free function kernels now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not. Will remove them.

@@ -4368,16 +4348,18 @@ class SyclKernelBodyCreator : public SyclKernelFieldHandler {
class FreeFunctionKernelBodyCreator : public SyclKernelFieldHandler {
SyclKernelDeclCreator &DeclCreator;
llvm::SmallVector<Stmt *, 16> BodyStmts;
// Keep track of the structs we have encountered on our way to a special type.
// They will be needed to properly generate the __init call. Note that the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are these kept for SYCL 2020 kernels? How free function kernels are different from SYCL 2020 so we need this additional tracking?

Copy link
Contributor Author

@lbushi25 lbushi25 Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SyclKernelBodyCreator uses the MemberExprBases vector which serves a similar purpose. I think there is slightly less code this way for example MemberExprBases requires a bit more work to maintain properly as we enter and leave structs. Also FreeFunctionKernelBodyCreator does not inherit from SyclKernelBodyCreator so we are not wasting information. I just chose a slightly different way of doing things.

FwdDeclEmitter.Visit(
member->getType().getDesugaredType(S.getASTContext()));
offsets.emplace_back(std::make_pair(
member, S.getASTContext().getFieldOffset(member) / 8));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For SYCL 2020 kernels we calculate offsets in SyclKernelIntHeaderCreator. Why can't we do that for free function kernels.

Comment on lines +302 to +312
[[__sycl_detail__::add_ir_attributes_function("sycl-single-task-kernel", 0)]]
void ff_25(AccessorAndLocalAccessor arg1) {
}

[[__sycl_detail__::add_ir_attributes_function("sycl-single-task-kernel", 0)]]
void ff_26(AccessorAndLocalAccessor arg1, SecondLevelAccessor arg2) {
}

[[__sycl_detail__::add_ir_attributes_function("sycl-single-task-kernel", 0)]]
void ff_27(IntAndAccessor arg1, AccessorAndInt) {
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need more testing here. We need template structs, typedefed structs, namespaces, default template parameters and combinations of these.

@lbushi25
Copy link
Contributor Author

lbushi25 commented Aug 8, 2025

I'm concerned that a lot of additional info is gathered and stored which doesn't happen in SYCL2020 mode. I think we lack some additional modifications to visitors mechanism for free function kernels and we should extend that instead of storing additional data which is not stored for SYCL 2020 kernels. I'm also fairly concerned with how much additional C++ code will be added to the integration header. C++ code generation and the whole integration header is proven to be heavily bug-prone. I'm not really sure generating the whole set_arg function is worth the complication and the maintenance burden. I think for SYCL RT it should be enough to know which parameter of generated spir_kernel function corresponds to which top-level parameter of the original free function kernel signature. cc @tahonermann , @AlexeySachkov and @intel/llvm-reviewers-runtime for opinions. I'm also slightly concerned that the PR doesn't have passing E2E tests added for the case.

Concerning the last statement, there is an E2E test added previously and I just removed the XFAIL's in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants