Skip to content
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

Specify add_free behaviour on invalid usage #27

Closed
wants to merge 1 commit into from
Closed

Specify add_free behaviour on invalid usage #27

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions sycl/doc/extensions/proposed/sycl_ext_oneapi_graph.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ Table 7. Member functions of the `command_graph` class (memory operations).
using namespace ext::oneapi::experimental;
node add_malloc_device(void *&data, size_t numBytes, const std::vector<node>& dep = {});
----
|Adding a node that encapsulates a memory allocation operation.
|Adds a node that encapsulates a USM device memory allocation operation.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding is that this is equivalent to a USM device allocation, saying this explicitly means that a reading can just consult that section of the SYCL spec for valid usage of the pointer returned by data. However if add_malloc_device should be used differently, we should specify that expectation here instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the usage of the pointers returned should be the same as a regular USM allocation, we could note that specifically here I suppose just to be extra clear.


Preconditions:

Expand Down Expand Up @@ -562,7 +562,10 @@ Exceptions:
using namespace ext::oneapi::experimental;
node add_free(void *data, const std::vector<node>& dep = {});
----
|Adding a node that encapsulates a memory free operation.
|Adds a node that encapsulates a memory free operation. The memory pointed to
by `data` must be a valid address that has been allocated using one of the USM
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The phrase

must be a valid address that has been allocated using one of the USM allocation routines

means that a user could call add_free using an address that was from a USM allocation function that is not add_malloc_device, but sycl::malloc_[device,host,shared]. I'd like to double check if this is behaviour we want. If not, this language can be changed to "must be a valid address that has been allocated from add_malloc_device"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think letting the graph free an allocation it didn't create is very error prone. IMO we should either mandate that it must have been allocated as part of the graph and error if not, or remove allocation/freeing altogether from graphs.

allocation routines. If any commands that use this memory are in-progress or
are enqueued, then the behavior is undefined.

Preconditions:

Expand Down