Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[SYCL] Add SYCL Module splitting. #1
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
[SYCL] Add SYCL Module splitting. #1
Changes from 5 commits
ae29f74
63d93ed
a773280
5e176d6
6a943c3
3171ebe
c7297ac
a12d0f7
d38dc63
bf3a7d4
1db04f1
051c0a9
93db9b2
a4e71e2
96d36a4
1f2039e
59e29a5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 feel like we may need to elaborate on what "entry point" means here.
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.
One related comment. Do we want to treat only SYCL kernels as 'entry points' or do we want to consider function with SYCL_EXTERNAL attribute also as 'entry points'? I think the former option is more viable from upstreaming POV.
Thanks
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.
Treating
SYCL_EXTERNAL
as entry points comes from "extra" features, like interoperability with other languages (like linking to a kernel written in ISPC which calls a function written in SYCL), or support for shared libraries/dynamic linking.For our initial patch, I think that we can most certainly simplify things down to only considering kernels as entry points
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.
Here is my take.
This functionality takes as input a fully linked SYCL device module with a set of SYCL device kernels and performs splitting to generate several fully-contained device modules. Each of the newly formed module contains a sub-set of the original set of SYCL device kernels along with a union of all the functions from each of their respective call graphs. Here, call graph of a SYCL kernel is the set of all functions reachable from that kernel.
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.
Should we be more consistent about by-value vs lvalue-ref vs rvalue-ref to accept more complex arguments like
EntryPointSet
? Note that it is aSetVector
and we are making a copy here in those constuctors.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.
All
EntryPointSet
arguments are expected to be constructed bymove
constructors.SetVector
is a container that stores content in the heap and it's move constructors are "cheap".In our repository, they have been passed over by
rvalue
references which is unconventional in C++. This observation has been inspired by tips like the following: https://abseil.io/tips/117. However, there is no direct guidance for function's arguments.I see now that we have a
const Properties &
argument that is being copied right away. I think we could apply the same principle for this argument as following:EntryPointGroup(StringRef GroupId, EntryPointSet Functions, Properties Props) : GroupId(GroupId), Functions(std::move(Functions)), Props(std::move(Props)) {}
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.
Please consider adding a descriptive comment. Thanks
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.
Considering that it is a simple string pair, do we really need to have this custom struct at all?
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.
We are going to add Properties into this structure in follow-up patches.
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.
Some separator kind of comment would be welcome here, I think to signify that there is a code which is related to the splitting algorithm itself and there is a code which is related to a tooling we have (that is in turn mostly used for testing)