Skip to content

[SYCL] Added missing SYCL 2020 USM features #3897

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
Jun 28, 2021
Merged

[SYCL] Added missing SYCL 2020 USM features #3897

merged 2 commits into from
Jun 28, 2021

Conversation

dnmokhov
Copy link
Contributor

@dnmokhov dnmokhov commented Jun 8, 2021

  • USM atomic allocation aspects (+ tests)
  • handler::mem_advise
  • handler::copy and queue::copy
  • queue class: overloads of fill, memset, memcpy, copy, mem_advise, prefetch with dependency event arguments
  • optional property list argument in USM allocation functions and usm_allocator constructors (+ tests)

Tests: intel/llvm-test-suite#342

- USM atomic allocation aspects (+ tests)
- handler::mem_advise
- handler::copy and queue::copy
- queue class: overloads of fill, memset, memcpy, copy, mem_advise, prefetch with dependency event arguments
- optional property list argument in USM allocation functions and usm_allocator constructors (+ tests)
@dnmokhov
Copy link
Contributor Author

dnmokhov commented Jun 8, 2021

/summary:run

@dnmokhov dnmokhov marked this pull request as ready for review June 9, 2021 18:39
@dnmokhov dnmokhov requested a review from a team as a code owner June 9, 2021 18:39
cperkinsintel
cperkinsintel previously approved these changes Jun 9, 2021
Copy link
Contributor

@cperkinsintel cperkinsintel left a comment

Choose a reason for hiding this comment

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

other than the question, LGTM.

@dm-vodopyanov dm-vodopyanov self-requested a review June 15, 2021 17:34
@dnmokhov
Copy link
Contributor Author

Pinging the reviewers: @intel/llvm-reviewers-runtime , @s-kanaev, @jbrodman, @dm-vodopyanov.

@vladimirlaz
Copy link
Contributor

@dnmokhov could you add tests for newly added functionality to https://github.com/intel/llvm-test-suite/
If it is in place please add link to the PR description.

Copy link
Contributor

@cperkinsintel cperkinsintel left a comment

Choose a reason for hiding this comment

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

LGTM

@bader bader requested a review from s-kanaev June 27, 2021 20:08
Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

LGTM

@bader bader merged commit 1df6873 into intel:sycl Jun 28, 2021
@gmlueck
Copy link
Contributor

gmlueck commented Jul 1, 2021

I noticed that this change was just merged.

Coincidentally, the language evolution group just identified a spec bug here. The USM copy member functions take the src and dst parameters in the wrong order. The order should be (src, dst), which matches the standard C++ copy function and matches the existing SYCL copy functions that take accessor parameters.

Can we change the implementation of these USM copy member functions?

@cperkinsintel
Copy link
Contributor

cperkinsintel commented Jul 1, 2021

@gmlueck - is that right? memcpy is definitely dest pointer argument first, followed by source. I thought std::copy took iterators, not pointers. The sycl::memcpy and sycl::copy methods in this PR both take pointers, no? Perhaps it's not the order of the arguments the committee should discuss, but the naming of the sycl::copy function?

@gmlueck
Copy link
Contributor

gmlueck commented Jul 1, 2021

We agree that std::memcpy is (dst, src), so we do not think the USM memcpy functions should change. They should remain as they are with (dst, src) ordering.

The USM copy functions are the ones we're worried about, which also have the parameter order (dst, src). Our primary concern is that this does not match the parameter order for the handler::copy() member functions that take an accessor parameter. Those functions have (src, dst) ordering. We think it is particularly bad to have two overloads of the handler::copy() function, where one takes (src, dst) and the other takes (dst, src).

We also think that the (src, dst) ordering is consistent with std::copy(). You are correct that std::copy() takes iterators (not pointers) as parameters. However, the order of the iterators is "src" first and then "dst".

bader pushed a commit that referenced this pull request Jul 5, 2021
As noted in #3897 (comment),
a spec bug was identified. The USM `copy` member functions take the `src`
and `dst` parameters in the wrong order. The order should be `(src, dst)`,
which matches the standard C++ `copy` function and matches the existing
SYCL `copy` functions that take accessor parameters.
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.

8 participants