Skip to content

[SYCL] Add range and nd_range CTAD support #772

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 1 commit into from
Nov 15, 2019
Merged

[SYCL] Add range and nd_range CTAD support #772

merged 1 commit into from
Nov 15, 2019

Conversation

rolandschulz
Copy link
Contributor

Also replace one enable_if with static_assert for better diagnostic.

keryell
keryell previously approved these changes Oct 30, 2019
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Thanks for starting implementing my bigger plan. :-)
triSYCL/triSYCL#218

sycl/test/basic_tests/range_ctad.cpp
int main() {
cl::sycl::range one_dim_range(64);
cl::sycl::range two_dim_range(64, 1);
cl::sycl::range three_dim_range(64, 1, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting to combine new-style CTAD with old-style explicit constructor call...

@keryell keryell mentioned this pull request Oct 30, 2019
Open
@rolandschulz
Copy link
Contributor Author

Thanks for starting implementing my bigger plan. :-)
triSYCL/triSYCL#218

Are there any other we should add? I looked through all and couldn't find any other.
The only other constructor I'm aware of where CTAD doesn't work is accessor. But for the accessor without handler argument it isn't obvious whether it should create a global placeholder accessor or a host accessor. And for the one with handler argument the get_accessor method seems a better choice.

@keryell
Copy link
Contributor

keryell commented Nov 2, 2019

Are there any other we should add? I looked through all and couldn't find any other.
The only other constructor I'm aware of where CTAD doesn't work is accessor. But for the accessor without handler argument it isn't obvious whether it should create a global placeholder accessor or a host accessor. And for the one with handler argument the get_accessor method seems a better choice.

Yes probably for the accessors get_accessor() is the way to go since we have auto.
The most obvious & useful case is inferring buffer type and dimensions with CTAD.

@jbrodman jbrodman self-requested a review November 11, 2019 14:54
jbrodman
jbrodman previously approved these changes Nov 11, 2019
View reviewed changes
Copy link
Contributor

@jbrodman jbrodman left a comment

Choose a reason for hiding this comment

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

LGTM

@romanovvlad
Copy link
Contributor

@rolandschulz Please, rebase your changes.

@rolandschulz
[SYCL] Add range, id, and nd_range CTAD support
ef512f8 
Also replace one enable_if with static_assert
for better diagnostic.

Signed-off-by: Roland Schulz <[email protected]>
@jbrodman jbrodman requested a review from romanovvlad November 14, 2019 20:25
@romanovvlad romanovvlad merged commit 644013e into intel:sycl Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader approved these changes

@keryell keryell keryell left review comments

@jbrodman jbrodman jbrodman left review comments

@romanovvlad romanovvlad Awaiting requested review from romanovvlad

Assignees

@romanovvlad romanovvlad

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rolandschulz @keryell @romanovvlad @bader @jbrodman