[SYCL][Doc] USM allocation functions with properties support #6346

jessicadavies-intel · 2022-06-22T18:04:27Z

This extension introduces USM memory allocation functions that take a sycl::ext::oneapi::experimental::properties argument instead of a sycl::property_list. They return a sycl::ext::oneapi::experimental::annotated_ptr object.

This PR replaces the initial draft #5656 opened by Sherry who is no longer working at Intel.

GarveyJoe · 2022-06-24T20:14:20Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+All properties that are valid for annotated_ptr must be supported by the USM memory allocation functions with `properties` support.
+Unless otherwise specified, supported properties do not affect the behavior of the USM memory allocation functions with `properties` support.
+
+[NOTE]


This note repeats the same information said immediately before it. You probably only need to say it once.

I tried to reword this so it is less repetitive. The NOTE now just mentions that the motivation is for user convenience in only needing to specify the properties in one place.

GarveyJoe · 2022-06-24T20:17:18Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+The `usm_alloc` property is supported by annotated_ptr, and therefore by the USM memory allocation functions defined in this extension.
+This property will always appear on the annotated_ptr returned by the USM memory allocation functions defined in this extension that do not have a `usm::alloc` parameter.
+The `usm_alloc` property may also be passed to the USM memory allocation functions defined in this extension.
+If the USM memory allocation kind specified by a function's `usm::alloc` parameter is different from the `usm_alloc` property, the result is undefined behavior.


This is easy to diagnose at compile-time, right? If so, let's make it an "error, diagnostic required" instead.

The usm::alloc parameter's value is not known until runtime. I will change this from UB to an exception.

GarveyJoe · 2022-10-27T22:03:41Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+
+// This section needs to be after the document title.
+:doctype: book
+:toc2:


Did you intend to add a table of contents? I don't think I've seen it on any of the other extensions.

I removed these lines. I don't want a table of contents.

GarveyJoe · 2022-10-27T22:03:45Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

@@ -0,0 +1,1468 @@
+


I added the title. Thanks.

GarveyJoe · 2022-10-27T22:04:14Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+== Examples
+
+Runtime and compile-time constant properties can be passed to the USM memory allocation functions introduced by this extension.
+Properties passed to an allocation function may or may not appear on the returned `annotated_ptr` object:


Suggested change

Properties passed to an allocation function may or may not appear on the returned `annotated_ptr` object:

Properties passed to an allocation function may or may not appear on the returned `annotated_ptr` object;

Why isn't a colon appropriate here?

The part before the punctuation doesn't introduce the part after. Each part is a full sentence. You could use a period, but a semi-colon seems more appropriate since you want to show that they are related.

I changed my mind after looking at more style guides. I think the colon is fine here.

GarveyJoe · 2022-10-27T22:11:43Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+
+----
+
+If the USM memory allocation kind specified by a parameter to the allocation function is different than the USM memory allocation kind specified by the `sycl::ext::oneapi::experimental::usm_kind` property, the USM memory allocation kind specified by the property takes precedence.


I think you said you would make this throw an exception.

Yes, I have now updated this to throw an exception.

GarveyJoe · 2022-10-27T22:16:55Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+auto APtr11 = annotated_malloc_host<int>(N, q, P7); // Error
+----
+
+The following example uses the compile-time-constant property `alignment`, defined in the link:../proposed/sycl_ext_oneapi_annotated_ptr.asciidoc[sycl_ext_oneapi_annotated_ptr] extension.


link is broken

The link will be broken until the annotated_ptr spec PR is merged. I will wait to merge the malloc extension until annotated_ptr is merged (and then I'll also double-check the links).

GarveyJoe · 2022-10-27T22:20:32Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+
+properties P9{alignment<1>};
+// Allocate N integers.
+// Alignment must be at least sizeof(int) bytes.


Does this actually convey what you want to say? I worry that an alignment of 7 satisfies this requirement as written. Can we simply say that the resulting pointer has an alignment of sizeof(int) bytes? Isn't it already part of the definition of alignment that alignment of N can be satisfied by a pointer whose value is any multiple of N (and can therefore be a multiple of XN as well where X is a natural number)?

"alignment must be at least sizeof(int) bytes" is a necessary condition but not sufficient.
I can change this to "alignment must be a positive non-zero integer multiple of sizeof(int)".

GarveyJoe · 2022-10-27T22:21:21Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+----
+
+This extension also introduces USM memory allocation functions with `properties` support that allow alignment to be specified at runtime, using a separate parameter of type `size_t`.
+If the compile-time constant `alignment` property is also passed in, it takes precedence over the alignment specified by the parameter of type `size_t`.


This doesn't seem user friendly. I would expect the resulting pointer to have both specified alignments. I.e. the resulting pointer value should be multiple of the lowest common multiple of the two specified alignments. Assuming both specified alignments are powers of two, that would mean that the larger of the two should take precedence.

I have made this change. I've removed all exceptions and errors that were caused by differing alignments.

GarveyJoe · 2022-10-27T22:26:38Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+
+[[table.usm.device.allocs]]
+.USM Device Memory Allocation Functions with properties Support
+[width="100%",options="header",separator="@",cols="65%,35%"]


This table doesn't render nicely for me. Is that just my setup or is there something wrong with it?

I improved the formatting of the first column that contains the function declarations.

GarveyJoe · 2022-10-27T22:56:39Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+
+The following five tables list all functions introduced by this extension.
+
+TODO: How is propertyListB going to be inferred? Do we need support for getting the type of a properties list with only the compile-time properties, and also adding a new compile-time property?


Yes, I think we want both of these.

zjin-lcf · 2022-11-10T16:46:46Z

I am not clear how to view the rendered asciidoc file. Could you please explain how CUDA's constant memory is migrated to SYCL using USM ? Thanks.

GarveyJoe · 2023-01-25T20:00:52Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+device that is contained by that context, otherwise this function throws a
+synchronous `exception` with the `errc::invalid` error code.
+
+And error is reported if `propList` contains


Suggested change

And error is reported if `propList` contains

An error is reported if `propList` contains

Thanks. Done.

GarveyJoe · 2023-01-25T21:21:05Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+The `sycl::ext::oneapi::experimental::usm_kind` property is a compile-time constant property with a single non-type parameter. This parameter is a value belonging to the enumeration `sycl::usm::alloc`.
+The `sycl::ext::oneapi::experimental::usm_kind` property is supported by `annotated_ptr` and the USM memory allocation functions defined in this extension.
+
+=== Deallocation


Should we add an overload of sycl::free that takes an annotated_ptr to make this easier?

keryell

Good proposal which trusts the C++ type-system instead of fighting against it.

keryell · 2023-01-26T11:46:43Z

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

+success.  The allocation size is specified in bytes.  This memory is not
+accessible on the host.  Memory allocated by `annotated_malloc_device`
+must be deallocated with `sycl::free` to avoid memory leaks.
+On failure, the raw pointer of the returned `annotated_ptr` will be `nullptr`.


I guess it is not possible to return nullptr here which is a C++ keyword and obviously is of type https://en.cppreference.com/w/cpp/types/nullptr_t
Perhaps you could have a using nullptr_t = ... and constexpr static nullptr_v = ... inside your annotated pointer class for this?

We are discussing adding a comparison operator to annotated_ptr, e.g.:

bool operator == (const nullptr_t &other) const { return this->ptr == nullptr; }
which would allow us to write

annotated_ptr x = malloc_device_annotated<int>(N, q); if (x == nullptr) {…}

Great idea.
You could also have a conversion-to-bool operator, if you do not have one already.

The annotated_ptr spec currently lists a conversion to bool operator:

explicit operator bool() const noexcept;
Returns false if the underlying pointer is null, returns true otherwise.

gmlueck

Some specific comments below, but I'd also like to bikeshed on the names. The current naming is like:

annotated_malloc_host

This seems weird to me, though. The function is primarily used for memory allocation, so shouldn't the "malloc" come first? How about:

malloc_host_annotated

It would be nice to drop the "annotated" entirely, but I don't see we can do that and still coexist with the sycl::malloc_host that is already in the spec.

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

…support extension.

…tr can now differ from those passed to the allocation function.

…l properties supported by annotated_ptr are supported by malloc. All compile-time and no runtime properties are propagated from malloc to the returned annotated_ptr.

…y on the annotated_ptr by default. Add support for specifying alignment and allocation kind at runtime. Provide additional functions that require the usm_kind property.

…d default argument syntax for propList.

…bles.

…instead of a raw pointer.

jessicadavies-intel · 2023-01-30T20:29:23Z

Some specific comments below, but I'd also like to bikeshed on the names. The current naming is like:

annotated_malloc_host

This seems weird to me, though. The function is primarily used for memory allocation, so shouldn't the "malloc" come first? How about:

malloc_host_annotated

I like malloc_host_annotated a lot better, so thanks for the suggestion. I made this change throughout the text.

…e SYCL 2020 spec.

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

…roperty key.

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc

bader · 2023-02-09T00:54:27Z

@jessicadavies-intel, please, update PR title tags when PR is ready for merge.
E.g. [WIP] -> [SYCL][Doc].

jessicadavies-intel requested a review from a team as a code owner June 22, 2022 18:04

jessicadavies-intel mentioned this pull request Jun 22, 2022

Adding compile time and runtime properties for usm allocations #5656

Closed

GarveyJoe reviewed Jun 24, 2022

View reviewed changes

pvchupin requested a review from GarveyJoe October 12, 2022 23:54

GarveyJoe reviewed Oct 27, 2022

View reviewed changes

jessicadavies-intel force-pushed the malloc_properties branch from c620431 to ea0a68f Compare November 7, 2022 17:11

GarveyJoe reviewed Jan 25, 2023

View reviewed changes

GarveyJoe approved these changes Jan 25, 2023

View reviewed changes

keryell reviewed Jan 26, 2023

View reviewed changes

gmlueck reviewed Jan 30, 2023

View reviewed changes

jessicadavies-intel added 9 commits January 30, 2023 12:25

Initial draft of the USM memory allocation functions with properties …

c7653a3

…support extension.

List all functions explicitly. Properties returned in the annotated_p…

f6ca73d

…tr can now differ from those passed to the allocation function.

Add usm_alloc property. Remove APIs that take a runtime alignment. Al…

593f2dd

…l properties supported by annotated_ptr are supported by malloc. All compile-time and no runtime properties are propagated from malloc to the returned annotated_ptr.

Small fixes.

1d667a4

Add annotated_ prefix to all function names. Add the usm_kind propert…

d312c6a

…y on the annotated_ptr by default. Add support for specifying alignment and allocation kind at runtime. Provide additional functions that require the usm_kind property.

Conflicting alignments or allocation kinds now cause exceptions. Fixe…

431c713

…d default argument syntax for propList.

Remove use of code role.

03923f4

Cleanup and corrections.

3603422

Remove exceptions for differing alignments. Format the code in the ta…

6acb364

…bles.

jessicadavies-intel added 2 commits January 30, 2023 12:25

Add two new memory deallocation functions that take an annotated_ptr …

1c27d57

…instead of a raw pointer.

Rename annotated_malloc to malloc_annotated.

063cd09

jessicadavies-intel force-pushed the malloc_properties branch from bc3415b to 063cd09 Compare January 30, 2023 20:26

jessicadavies-intel added 2 commits January 30, 2023 13:53

Address most of Greg's comments.

16b083e

Update the function descriptions to be equivalent to Revision 6 of th…

b3ea6e7

…e SYCL 2020 spec.

gmlueck reviewed Jan 31, 2023

View reviewed changes

jessicadavies-intel added 2 commits February 2, 2023 14:36

Fix nullptr. Add notes to examples about runtime alignment.

ce4315c

Added the convenience vars usm_kind_[host|device|shared]. Added the p…

b370b85

…roperty key.

gmlueck reviewed Feb 3, 2023

View reviewed changes

sycl/doc/extensions/proposed/sycl_ext_oneapi_usm_malloc_properties.asciidoc Outdated Show resolved Hide resolved

Fix typo.

d2d7ce7

gmlueck approved these changes Feb 8, 2023

View reviewed changes

jessicadavies-intel changed the title ~~[WIP] USM allocation functions with properties support~~ [SYCL][Doc] USM allocation functions with properties support Feb 13, 2023

bader merged commit 3600f92 into intel:sycl Feb 13, 2023

jessicadavies-intel deleted the malloc_properties branch February 13, 2023 22:33

	Properties passed to an allocation function may or may not appear on the returned `annotated_ptr` object:
	Properties passed to an allocation function may or may not appear on the returned `annotated_ptr` object;


		----

		If the USM memory allocation kind specified by a parameter to the allocation function is different than the USM memory allocation kind specified by the `sycl::ext::oneapi::experimental::usm_kind` property, the USM memory allocation kind specified by the property takes precedence.


		The following five tables list all functions introduced by this extension.

		TODO: How is propertyListB going to be inferred? Do we need support for getting the type of a properties list with only the compile-time properties, and also adding a new compile-time property?

	And error is reported if `propList` contains
	An error is reported if `propList` contains

[SYCL][Doc] USM allocation functions with properties support #6346

[SYCL][Doc] USM allocation functions with properties support #6346

Uh oh!

Conversation

jessicadavies-intel commented Jun 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GarveyJoe Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GarveyJoe Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zjin-lcf commented Nov 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlueck left a comment

Choose a reason for hiding this comment

Uh oh!

GarveyJoe Oct 27, 2022 •

edited

Loading

GarveyJoe Oct 27, 2022 •

edited

Loading