Repository navigation
[SYCL][2/2] Add enqueue function overloads taking kernel arguments as a std::span
#22971
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
Draft
mieshkiwrk
wants to merge
24
commits into
intel:sycl
Choose a base branch
from
mieshkiwrk:kernel-fastpath-span
base: sycl
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
3f95324
[SYCL] Add nd_launch overloads taking kernel arguments as a span
mieshkiwrk 293786f
[SYCL] Let raw_kernel_arg say that an argument is a pointer
mieshkiwrk 8bcaf7d
[SYCL] Pass an argument list given as a container to the span overload
mieshkiwrk 39ac830
[SYCL] Replace the argument sequence note with an Examples section
mieshkiwrk 9986af5
[SYCL] Take the kernel argument sequence as a std::span
mieshkiwrk c23f975
[SYCL] Apply clang-format to the argument sequence changes
mieshkiwrk 82aca3c
[SYCL] Rewrap the argument sequence dependency paragraph
mieshkiwrk 5502f83
[SYCL] Make the C++20 annotations reach the rendered specification
mieshkiwrk 9608d55
Update sycl/doc/extensions/experimental/sycl_ext_oneapi_raw_kernel_ar…
mieshkiwrk c01fd9a
[SYCL] Rewrap the applied suggestion for the pointer constructor
mieshkiwrk 89fb407
[SYCL] Use the namespace alias in both raw_kernel_arg examples
mieshkiwrk 5a2f8b8
[SYCL] Say that the byte constructor must not take a pointer argument
mieshkiwrk 34bf2f1
[SYCL] Drop the argument sequence paragraph from the Overview
mieshkiwrk 70e8177
[SYCL] Shorten the version 2 row of the enqueue functions extension
mieshkiwrk b51e275
[SYCL] Drop the Level Zero justification from the pointer argument note
mieshkiwrk bd98858
[SYCL] Move the pointer argument requirement to the Overview
mieshkiwrk a257649
[SYCL] Use the reviewer's wording for the pointer argument requirement
mieshkiwrk 4538a46
[SYCL] Limit the backend support status to the tested backends
mieshkiwrk 5c15db5
[SYCL] Rewrite the argument sequence example as a complete program
mieshkiwrk 551d09a
[SYCL] Constrain the parameter pack overloads instead of describing t…
mieshkiwrk ccb49bb
[SYCL] Keep the parameter pack literal in the rendered constraint
mieshkiwrk 5317dd6
[SYCL] Add std::span overloads to the remaining kernel object launch …
mieshkiwrk cff7c04
Review changes
mieshkiwrk a47e753
comment reduction
mieshkiwrk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Just adding this comment here as a convenient place ... There are several launch functions that take a pack of
Args, but you only added araw_kernel_argoverload for one set. The others are:single_tasktakingconst sycl::kernel& k, Args&&... args.parallel_fortakingsycl::range<Dimensions> r, const sycl::kernel& k, Args&&... argsparallel_fortakinglaunch_config<sycl::range<Dimensions>, Properties> c, const sycl::kernel& k, Args&& args...nd_launchtakinglaunch_config<sycl::nd_range<Dimensions>, Properties> c, const sycl::kernel& k, Args&& args...Can you add
raw_kernel_argoverloads for those too?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.
Would you prefer them in this PR or a separate one?
It is eight more overloads plus spec and tests, and they would go through the handler - the fast path from #22970 only covers
nd_launchwith annd_range.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'm OK either way. Maybe members of @intel/llvm-reviewers-runtime have a preference?
If you do make a separate PR, I assume you will prioritize it, so it comes soon? I'd like the APIs taking
std::spanto be consistent before we make our next release.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'll start looking into exact needed changes and let you know when it's ready about it's size.
For me it would be easier to keep it within this PR but it would get kinda big for review.
Let me update about the changes size and then we'll see how to handle that
Uh oh!
There was an error while loading. Please reload this page.
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.
Added them here in 5317dd6:
single_task,parallel_forwith a range, and bothlaunch_configforms, on the queue and on the handler, with spec entries and tests. They bind through the handler, since the handler-less path only coversnd_launchwith annd_range, and add no exported symbols; the extension stays at version 2 as it is not released yet.It is one big commit, let me know if you prefer as a separate PR.
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.
Friendly reminder bump
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.
this PR is huge already, I would suggest creating a follow-up PR for extra overloads
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've seen you did review already, not sure to split it now
Let me know if still needed then I'll do that