[SYCL] Return nullptr when allocation size is zero in usm allocator #12765
Conversation
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| auto ctxt = q.get_context(); | ||
|
|
||
| if (dev.get_info<info::device::usm_host_allocations>()) { | ||
| sycl::usm_allocator<int, sycl::usm::alloc::host> ua{ctxt, dev}; |
There was a problem hiding this comment.
Don't we have another overload that accepts queue here? If not, I think the common spelling in our sources is ctx.
| auto dev = q.get_device(); | ||
| auto ctxt = q.get_context(); | ||
|
|
||
| if (dev.get_info<info::device::usm_host_allocations>()) { |
There was a problem hiding this comment.
Why do we only test "host" here?
There was a problem hiding this comment.
I'm not sure how adding more allocation types will increase the test coverage of this change. Nevertheless, I've expanded the test to also check for zero-sized shared allocations. usm::alloc::device is not supported by usm_allocator so, I've skipped that.
| assert(!p && "usm_allocator should return a null pointer when allocation " | ||
| "size is zero."); |
There was a problem hiding this comment.
I'd reword as "our implementation is expected to ...".
| #include <sycl/sycl.hpp> | ||
|
|
||
| using namespace sycl; | ||
| #define ALLOC_SIZE 0 |
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
|
||
| assert(!p && "Our implementation of usm_allocator is expected to return a " | ||
| "null pointer when allocation " | ||
| "size is zero."); |
There was a problem hiding this comment.
I think this is fine, but another option is to simply remove the assert. From an API perspective, the important thing is that you can call allocate with a zero size and that you can then call deallocate on the returned pointer, all without any exception being raised. It's not important whether or not the returned pointer is null.
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Currently, usm allocator throws when the allocation size is zero. However, this behavior is not aligned with that of std::allocator.
Refer KhronosGroup/SYCL-Docs#355 for discussion regarding this. The spec says that the allocation functions must succeed when the size is zero. The value returned in this case is unspecified (it can either be a NULL pointer or a non-NULL pointer)
This PR makes USM allocator return a null pointer when the allocation size is zero.