Skip to content

[SYCL][DOC] USM Proposal Update #395

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 3 commits into from
Aug 9, 2019
Merged

[SYCL][DOC] USM Proposal Update #395

merged 3 commits into from
Aug 9, 2019

Conversation

jbrodman
Copy link
Contributor

Rev proposal based on feedback.

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.

Great improvement!

};

class queue {
...
public:
...
event sycl_memset(void* ptr, int value, size_t count);
event memset(void* ptr, int value, size_t count);
Copy link
Contributor

Choose a reason for hiding this comment

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

fill?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be contiguous byte-based like std::memset.

Iterators don't have that requirement, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

void * is not an iterator anyway.
But of course if you prefer you can use memset.

[source,cpp]
----
class handler {
...
public:
...
event sycl_memset(void* ptr, int value, size_t count);
event memset(void* ptr, int value, size_t count);
Copy link
Contributor

Choose a reason for hiding this comment

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

fill like std::fill?

[source,cpp]
----
class handler {
...
public:
...
event sycl_memcpy(void* dest, const void* src, size_t count);
event memcpy(void* dest, const void* src, size_t count);
Copy link
Contributor

Choose a reason for hiding this comment

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

copy_n like std::copy_n?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be contiguous byte copy like std::memcpy.

Iterators don't have that requirement, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I don't like adding a flavor of handler::copy for this, b/c those are based on objects that have their size baked in, hence good ol' std::memcpy style operations.

Note - we can't use std::memcpy with device pointers b/c we need to handle them specially.

Copy link
Contributor

Choose a reason for hiding this comment

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

As you want.
This was just to propose some C++-like naming.
But C-like naming is fine.
In both case anyway it not exactly the same as either the C or C++ equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. The good news is that for shared/host allocations, regular std::fill/copy_n should work!

};

class queue {
...
public:
...
event sycl_memcpy(void* dest, const void* src, size_t count);
event memcpy(void* dest, const void* src, size_t count);
Copy link
Contributor

Choose a reason for hiding this comment

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

copy_n like std::copy_n? And if so, adapting the order of the arguments...

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

LGTM.
One comment.

bader
bader previously approved these changes Aug 8, 2019
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

@keryell, does it look good to you?

@bader
Copy link
Contributor

bader commented Aug 8, 2019

http://ec2-52-15-243-124.us-east-2.compute.amazonaws.com:8010/#/builders/18/builds/16

@jbrodman, could you merge sycl branch to your branch, please?
We recently added Windows systems to CI and it seems it relies on some recent changes in the sycl branch.
@vladimirlaz, am I right?

@jbrodman
Copy link
Contributor Author

jbrodman commented Aug 8, 2019

Done.

bader
bader previously approved these changes Aug 8, 2019
@bader
Copy link
Contributor

bader commented Aug 8, 2019

okay. It didn't help.
@vladimirlaz, @DoyleLi, @tfzhu could you fix sycl-win-x64-pr ASAP.

@DoyleLi
Copy link
Contributor

DoyleLi commented Aug 9, 2019

okay. It didn't help.
@vladimirlaz, @DoyleLi, @tfzhu could you fix sycl-win-x64-pr ASAP.

Hi Alexey, Vladimir has already merged the code for windows build in the PR:

#463
[SYCL] Enable build and LIT tests on windows

You can update your code to make win-x64-pr pass

@bader
Copy link
Contributor

bader commented Aug 9, 2019

okay. It didn't help.
@vladimirlaz, @DoyleLi, @tfzhu could you fix sycl-win-x64-pr ASAP.

Hi Alexey, Vladimir has already merged the code for windows build in the PR:

#463
[SYCL] Enable build and LIT tests on windows

You can update your code to make win-x64-pr pass

@DoyleLi, could you clarify what do you mean by "update your code"?
I think James merged this patch to this PR. Am I wrong?

@DoyleLi
Copy link
Contributor

DoyleLi commented Aug 9, 2019

okay. It didn't help.
@vladimirlaz, @DoyleLi, @tfzhu could you fix sycl-win-x64-pr ASAP.

Hi Alexey, Vladimir has already merged the code for windows build in the PR:
#463
[SYCL] Enable build and LIT tests on windows
You can update your code to make win-x64-pr pass

@DoyleLi, could you clarify what do you mean by "update your code"?
I think James merged this patch to this PR. Am I wrong?

I mean you can update the files under the folder "buildbot" in your own branch from "#463":

buildbot/compile.py
buildbot/configure.py
buildbot/dependency.py

@bader
Copy link
Contributor

bader commented Aug 9, 2019

I think James merged this patch to this PR. Am I wrong?

I was wrong.
@jbrodman, could you merge #463 to you branch, please?
Your branch is currently "89 commits behind intel:sycl."

Signed-off-by: James Brodman <[email protected]>
Signed-off-by: James Brodman <[email protected]>
@jbrodman
Copy link
Contributor Author

jbrodman commented Aug 9, 2019

Updated.

@bader bader merged commit 603c1f6 into intel:sycl Aug 9, 2019
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.

4 participants