Skip to content

[SYCL] Allocate SubmissionInfo completely on stack #18314

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
May 8, 2025
Merged

[SYCL] Allocate SubmissionInfo completely on stack #18314

merged 18 commits into from
May 8, 2025

Conversation

Alexandr-Konovalov
Copy link
Contributor

@Alexandr-Konovalov Alexandr-Konovalov commented May 5, 2025 •
edited
Loading

Move SubmissionInfo to versioned public header and allocate it completely on stack.

@Alexandr-Konovalov
[SYCL] Allocate SubmissionInfo complitely on stack
3394a1d 
Move SubmissionInfo to versioned public header and allocate it
complitely on stack.

Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
@Alexandr-Konovalov
Code formatting.
2ccf7e8 
Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
@Alexandr-Konovalov
Code formatting.
9d553e0 
Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
@Alexandr-Konovalov
Fix build under Windows.
0af903f 
Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
@Alexandr-Konovalov
d1->v1, add old code to __INTEL_PREVIEW_BREAKING_CHANGES.
2010156 
Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
@vinser52 vinser52 requested a review from steffenlarsen May 7, 2025 07:47
@Alexandr-Konovalov Alexandr-Konovalov marked this pull request as ready for review May 7, 2025 08:45
@Alexandr-Konovalov Alexandr-Konovalov requested a review from a team as a code owner May 7, 2025 08:45
@steffenlarsen steffenlarsen changed the title [SYCL] Allocate SubmissionInfo complitely on stack [SYCL] Allocate SubmissionInfo completely on stack May 7, 2025
@Alexandr-Konovalov
Add comment.
294b9b1 
Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
@vinser52 vinser52 requested a review from sergey-semenov May 7, 2025 14:49
@Alexandr-Konovalov
Merge branch 'sycl' into Alexandr-Konovalov/SubmissionInfo
62995ce 
Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
steffenlarsen
steffenlarsen reviewed May 8, 2025
View reviewed changes
@Alexandr-Konovalov @steffenlarsen
Fix alignment.
496105f 
Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>
@Alexandr-Konovalov
Improve comment.
b5fd0b5 
Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
steffenlarsen
steffenlarsen approved these changes May 8, 2025
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM!

sergey-semenov
sergey-semenov approved these changes May 8, 2025
View reviewed changes
@Alexandr-Konovalov @sergey-semenov
Update sycl/include/sycl/detail/optional.hpp
c174746 
Co-authored-by: Sergey Semenov <sergey.semenov@intel.com>
@Alexandr-Konovalov
Get rid of SubmissionInfoImpl.
46c7b28 
Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
@Alexandr-Konovalov
Use has_value() instead of cast to bool.
910d142 
Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
@Alexandr-Konovalov
Code formatting.
ac58752 
Signed-off-by: Alexandr Konovalov <alexandr.konovalov@intel.com>
@vinser52 vinser52 requested a review from sergey-semenov May 8, 2025 15:22
@Alexandr-Konovalov
Copy link
Contributor Author

ThreadSanitizer/check_device_global.cpp failure seems unrelated to the PR, see #18349 .

@vinser52
Copy link
Contributor

vinser52 commented May 8, 2025

@intel/llvm-gatekeepers the PR is ready to merge

@sarnex sarnex merged commit 75aa60c into intel:sycl May 8, 2025
18 of 19 checks passed
aelovikov-intel
aelovikov-intel reviewed May 8, 2025
View reviewed changes
Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

I also doubt that this approach would work well for non-NFC ABI-breaking changes (e.g. adding new fields or removing old ones). Providing old versions without those new fields could be troublesome, unless all of them are actually optional.

sycl/include/sycl/queue.hpp
@@ -95,6 +96,60 @@ class __SYCL_EXPORT SubmissionInfo {
private:
std::shared_ptr<SubmissionInfoImpl> impl = nullptr;
};
#endif

namespace v1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This way one particular data structure claims entire sycl::detail::vN namespace, so that we can't use the same approach for other APIs (that's the same reason why I think our sycl::_v1 turned out to be rather useless). IMO, it would be better to have detail::submission_info::_v1::SubmissionInfo or something like that. Maybe make submission_info inline itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aelovikov-intel aelovikov-intel aelovikov-intel left review comments

@vinser52 vinser52 vinser52 approved these changes

@sergey-semenov sergey-semenov sergey-semenov approved these changes

@steffenlarsen steffenlarsen steffenlarsen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Alexandr-Konovalov @vinser52 @sergey-semenov @aelovikov-intel @steffenlarsen @sarnex