Skip to content

[SYCL] Restore the nd_range default constructor #22949

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

Merged
KornevNikita merged 3 commits into from
Aug 25, 2026
+14 −0
Merged

[SYCL] Restore the nd_range default constructor #22949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
748a67a
[SYCL] Restore the nd_range default constructor
arnavprabhu Aug 15, 2026
7c19c4d
[SYCL] Address nd_range review feedback
arnavprabhu Aug 17, 2026
a0aa87b
add-guards
KornevNikita Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions sycl/include/sycl/nd_range.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ template <int Dimensions = 1> class nd_range {
nd_range(nd_range<Dimensions> &&rhs) = default;
nd_range<Dimensions> &operator=(const nd_range<Dimensions> &rhs) = default;
nd_range<Dimensions> &operator=(nd_range<Dimensions> &&rhs) = default;

#ifndef __INTEL_PREVIEW_BREAKING_CHANGES

@dvrogozh dvrogozh Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On pytorch side we can't accept this solution in the release version of the compiler, neither closed source DPC++ nor open source dpclang. We need this resolved before the next release. Can you educate me what exactly is an issue to step out from the spec on this matter and continue to have nd_range default compiler defined?

@KornevNikita KornevNikita Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dvrogozh you mean you can't accept this macro? By the next release you mean 7.3.0? Because 7.2.0 will contain this ctor. I believe we'll resolve this by 7.3.0, this is just to unblock folks that need this right now.

Can you educate me what exactly is an issue to step out from the spec on this matter and continue to have nd_range default compiler defined?

@gmlueck do you think we can restore this right now without any guards? Note - we had it for a long time, but I removed it recently, so it won't be like adding a new API but rather restoring the existed one.

@dvrogozh dvrogozh Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pytorch stopped using preview macro long time ago and it was an effort dedicated to that due to Pytorch being production environment. And we don't want to introduce it again.

this is just to unblock folks that need this right now.

This solution is OK as intermediate engineering solution, but that's showstopper from my perspective for the next release which seems to be 7.3.0 per your comment.

@KornevNikita KornevNikita Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This ctor is added under "if not defined INTEL_PREVIEW...", so it'll be available even in 7.3.0, but might be removed prior to 8.0.0. I believe we'll add it to SYCL 2020 or create a oneapi extension by this time.

Does it work for you?

@dvrogozh dvrogozh Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh, ok, I misread the condition looking into the PR early morning. Then reformulating the concern, this PR is fine right now but gives a risk to the next major release 8.0.0 if you will decide to drop this ctor.

@gmlueck gmlueck Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not opposed to this change, but it might make sense to wait until later this week if there is no hurry. I'm hoping to bring KhronosGroup/SYCL-Docs#1043 up for discussing on Thursday of this week. If the Khronos WG decides to add the default constructor as an "errata" fix, then we won't need this PR at all. We can just keep the default constructor as it is now.

@dvrogozh dvrogozh Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Without this change intel/llvm pytorch CI is blocked. Longer it's broken more risk for other issues to appear unnoticed contributing to the triage bucket.

@gmlueck gmlueck Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe I'm reading this PR wrong. I thought it was just removing the default constructor in -fpreview-breaking-changes mode? There's no change at all in the regular mode, right?

@dvrogozh, I think you said above that PyTorch doesn't use the -fpreview-breaking-changes mode. If my understanding about this PR is correct, I don't think it will have any effect on PyTorch at all.

@dvrogozh dvrogozh Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am talking about pytorch CI defined in this intel/llvm repository. You have a workflow which validates most recent builds of the dpclang compiler with the pytorch. See https://github.com/intel/llvm/blob/sycl/.github/workflows/sycl-pytorch-build-and-test.yml.

@gmlueck gmlueck Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand now. Sorry, I didn't read the PR description closely enough. I see now that this is undoing the recent change in #22908, which removed that default constructor.

Feel free to merge this PR now if you want. If the WG decides later to add the default constructor, we can make another PR to remove the #ifndef.

nd_range() = default;
#endif // __INTEL_PREVIEW_BREAKING_CHANGES

~nd_range() = default;

// Common hidden friend functions for by-value semantics
Expand Down
9 changes: 9 additions & 0 deletions sycl/test/basic_tests/nd_range.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,13 @@ int main() {
assert(three_dim_nd_range.get_group_range() == sycl::range<3>(2, 2, 2));
assert(three_dim_nd_range.get_offset() == sycl::id<3>(0, 0, 0));
cout << "three_dim_nd_range passed " << endl;

#ifndef __INTEL_PREVIEW_BREAKING_CHANGES
// Check that a default-constructed nd_range has zeroed ranges and offset.
sycl::nd_range<3> default_nd_range;
assert(default_nd_range.get_global_range() == sycl::range<3>(0, 0, 0));
assert(default_nd_range.get_local_range() == sycl::range<3>(0, 0, 0));
assert(default_nd_range.get_offset() == sycl::id<3>(0, 0, 0));
cout << "default nd_range passed " << endl;
#endif // __INTEL_PREVIEW_BREAKING_CHANGES
}
Loading