Skip to content

[SYCL] Allow USM ptrs in exclusive and inclusive scans #4310

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

Merged
merged 2 commits into from
Aug 15, 2021

Conversation

Michoumichmich
Copy link
Contributor

The group algorithms are currently accepting only multi_ptrs while the SYCL spec (4.17.4) states that data ranges can be described using pointers, iterators or instances of the multi_ptr class. The spec allows to introduce additional restrictions, but now that this implementation supports USM should we still keep enforcing the use of multi_ptr ? The doc has no reference to the use of multi_ptr.

@Michoumichmich Michoumichmich requested a review from a team as a code owner August 10, 2021 17:37
@dm-vodopyanov
Copy link
Contributor

now that this implementation supports USM should we still keep enforcing the use of multi_ptr ? The doc has no reference to the use of multi_ptr.

@Pennycook , can you please comment?

@Pennycook
Copy link
Contributor

@Pennycook , can you please comment?

Functionality LGTM. Our implementation of joint_reduce already seems to accept pointers that aren't multi_ptrs, so it makes sense to extend that to exclusive_scan and inclusive_scan.

Copy link
Contributor

@vladimirlaz vladimirlaz left a comment

Choose a reason for hiding this comment

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

@Pennycook , can you please comment?

Functionality LGTM. Our implementation of joint_reduce already seems to accept pointers that aren't multi_ptrs, so it makes sense to extend that to exclusive_scan and inclusive_scan.

should we "deduce" if the raw pointer is in global or local AS (for future performance optimizations)? Or this functionality assumes glocab AS?

@Pennycook
Copy link
Contributor

should we "deduce" if the raw pointer is in global or local AS (for future performance optimizations)? Or this functionality assumes glocab AS?

Good question. I think we should explore this in a separate PR, because my hope is that this won't be necessary.

The algorithms can work with any address space that all work-items in the group can see. (We don't currently have a group that represents a single work-item, but if we introduced that degenerate case, even private memory would make sense I suppose). Unlike the sub-group loads/stores, we never have to pass the pointer directly to a SPIR-V built-in -- if you take a peek inside the implementation, joint_reduce is basically just a loop around reduce_over_group, with all the pointer dereferences happening in the headers.

So my expectation is:

  • If the user passes a multi_ptr, the compiler will propagate the address space to the point of dereference
  • If the user passes a generic raw pointer and the compiler can deduce the address space at compile-time, it will propagate the address space to the point of dereference
  • If the user passes a generic raw pointer and the compiler can't deduce the address space at compile-time, it will generate a run-time check and hoist that check outside of the loop (because I think it's safe to assume that incrementing a global pointer gives a global pointer)

I think we should only worry about adding our own run-time checks if the compiler is failing with the hoisting, and if we're certain that the compiler will then optimize away the run-time checks in the other cases.

@vladimirlaz
Copy link
Contributor

I think we should only worry about adding our own run-time checks if the compiler is failing with the hoisting, and if we're certain that the compiler will then optimize away the run-time checks in the other cases.

Thank you for the detailed explanation. It is clear now.

@bader bader merged commit 2de0f92 into intel:sycl Aug 15, 2021
@Michoumichmich Michoumichmich deleted the Removing_multiptr_group_algorithms branch August 17, 2021 14:59
@Michoumichmich Michoumichmich restored the Removing_multiptr_group_algorithms branch September 10, 2021 10:52
@Michoumichmich Michoumichmich deleted the Removing_multiptr_group_algorithms branch October 18, 2021 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants