raise exception in malloc when device does not supports usm #10348

pwisniewskimobica · 2023-07-13T13:23:39Z

Due to issue #9513
I made changes where alignedAlloc method throws exception when device does not supports usm

al42and · 2023-07-13T17:23:22Z

Ideally, should also check malloc_host:

Throws a synchronous exception with the errc::feature_not_supported error code if no device in syclContext has aspect::usm_host_allocations.

al42and · 2023-07-13T17:21:57Z

sycl/source/detail/usm/usm_impl.cpp

@@ -232,6 +232,11 @@ void *alignedAllocInternal(size_t Alignment, size_t Size,
 void *alignedAlloc(size_t Alignment, size_t Size, const context &Ctxt,
                   const device &Dev, alloc Kind, const property_list &PropList,
                   const detail::code_location &CodeLoc) {
+  if (Dev.has(sycl::aspect::usm_device_allocations) == false) {


Depending on Kind, we should check either usm_device_allocations or usm_shared_allocations. I doubt there are relevant devices with one but not the other, but the spec says what the spec says.

@al42and Thanks for that!
All your remarks I just applied in newest commit.
Please take a look

…dification

al42and · 2023-07-14T15:37:38Z

sycl/source/detail/usm/usm_impl.cpp

+  // feature_not_supported exception
+  bool raiseUsmException = true;
+  for (auto device : Ctxt.get_devices()) {
+    if (device.has(sycl::aspect::usm_device_allocations)) {


Suggested change

if (device.has(sycl::aspect::usm_device_allocations)) {

if (device.has(sycl::aspect::usm_host_allocations)) {

Also, I think it might be slightly more concise if std::any_of / std::none_of is used, but that's a matter of taste?

+1 for std::none_of; it gets rid of the extra auxiliary variable

al42and · 2023-07-14T15:43:41Z

sycl/source/detail/usm/usm_impl.cpp

+  // if no device in context has spect::usm_host_allocations then raise
+  // feature_not_supported exception
+  bool raiseUsmException = true;
+  for (auto device : Ctxt.get_devices()) {


Nit:

Suggested change

for (auto device : Ctxt.get_devices()) {

for (const auto& device : Ctxt.get_devices()) {

ldrumm · 2023-08-08T10:42:09Z

sycl/source/detail/usm/usm_impl.cpp

+        sycl::make_error_code(sycl::errc::feature_not_supported),
+        "usm not supported");
+  } else if (Kind == alloc::host &&
+             Dev.has(sycl::aspect::usm_host_allocations) == false) {


Suggested change

Dev.has(sycl::aspect::usm_host_allocations) == false) {

!Dev.has(sycl::aspect::usm_host_allocations)) {

ldrumm · 2023-08-08T10:42:23Z

sycl/source/detail/usm/usm_impl.cpp

+        sycl::make_error_code(sycl::errc::feature_not_supported),
+        "usm not supported");
+  } else if (Kind == alloc::shared &&
+             Dev.has(sycl::aspect::usm_shared_allocations) == false) {


Suggested change

Dev.has(sycl::aspect::usm_shared_allocations) == false) {

!Dev.has(sycl::aspect::usm_shared_allocations)) {

ldrumm · 2023-08-08T10:42:36Z

sycl/source/detail/usm/usm_impl.cpp

@@ -232,6 +246,23 @@ void *alignedAllocInternal(size_t Alignment, size_t Size,
 void *alignedAlloc(size_t Alignment, size_t Size, const context &Ctxt,
                   const device &Dev, alloc Kind, const property_list &PropList,
                   const detail::code_location &CodeLoc) {
+  if (Kind == alloc::device &&
+      Dev.has(sycl::aspect::usm_device_allocations) == false) {


Suggested change

Dev.has(sycl::aspect::usm_device_allocations) == false) {

!Dev.has(sycl::aspect::usm_device_allocations)) {

ldrumm · 2023-08-08T10:42:59Z

sycl/source/detail/usm/usm_impl.cpp

+      raiseUsmException = false;
+    }
+  }
+  if (raiseUsmException == true) {


Suggested change

if (raiseUsmException == true) {

if (raiseUsmException) {

ldrumm · 2023-08-08T10:43:43Z

sycl/source/detail/usm/usm_impl.cpp

@@ -41,6 +41,20 @@ namespace usm {
 void *alignedAllocHost(size_t Alignment, size_t Size, const context &Ctxt,
                       alloc Kind, const property_list &PropList,
                       const detail::code_location &CodeLoc) {
+  // if no device in context has spect::usm_host_allocations then raise
+  // feature_not_supported exception
+  bool raiseUsmException = true;


The variable names introduced are not consistent with the surrounding convention

ldrumm · 2023-08-08T10:44:50Z

sycl/source/detail/usm/usm_impl.cpp

+  // feature_not_supported exception
+  bool raiseUsmException = true;
+  for (auto device : Ctxt.get_devices()) {
+    if (device.has(sycl::aspect::usm_device_allocations)) {


+1 for std::none_of; it gets rid of the extra auxiliary variable

github-actions · 2024-09-15T02:02:14Z

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

github-actions · 2024-10-16T02:00:30Z

This pull request was closed because it has been stalled for 30 days with no activity.

raise exception in malloc when device does not supports usm

ec43718

pwisniewskimobica requested a review from a team as a code owner July 13, 2023 13:23

pwisniewskimobica requested a review from sergey-semenov July 13, 2023 13:23

pwisniewskimobica mentioned this pull request Jul 13, 2023

[SYCL][OpenCL] malloc_device returns 0 when USM is not supported, should throw #9513

Open

pwisniewskimobica temporarily deployed to aws July 13, 2023 13:38 — with GitHub Actions Inactive

pwisniewskimobica temporarily deployed to aws July 13, 2023 14:19 — with GitHub Actions Inactive

al42and reviewed Jul 13, 2023

View reviewed changes

Take care of Kind argument in allignedAlloc(), allignedAllocHost() mo…

24eed85

…dification

pwisniewskimobica temporarily deployed to aws July 14, 2023 10:53 — with GitHub Actions Inactive

pwisniewskimobica temporarily deployed to aws July 14, 2023 11:33 — with GitHub Actions Inactive

al42and reviewed Jul 14, 2023

View reviewed changes

ldrumm requested changes Aug 8, 2023

View reviewed changes

github-actions bot added the Stale label Sep 15, 2024

github-actions bot closed this Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raise exception in malloc when device does not supports usm #10348

raise exception in malloc when device does not supports usm #10348

pwisniewskimobica commented Jul 13, 2023

al42and commented Jul 13, 2023

al42and Jul 13, 2023

pwisniewskimobica Jul 14, 2023

al42and Jul 14, 2023

ldrumm Aug 8, 2023

al42and Jul 14, 2023

ldrumm Aug 8, 2023

ldrumm Aug 8, 2023

ldrumm Aug 8, 2023

ldrumm Aug 8, 2023

ldrumm Aug 8, 2023

ldrumm Aug 8, 2023

github-actions bot commented Sep 15, 2024

github-actions bot commented Oct 16, 2024

	if (device.has(sycl::aspect::usm_device_allocations)) {
	if (device.has(sycl::aspect::usm_host_allocations)) {

	for (auto device : Ctxt.get_devices()) {
	for (const auto& device : Ctxt.get_devices()) {

	Dev.has(sycl::aspect::usm_host_allocations) == false) {
	!Dev.has(sycl::aspect::usm_host_allocations)) {

	Dev.has(sycl::aspect::usm_shared_allocations) == false) {
	!Dev.has(sycl::aspect::usm_shared_allocations)) {

	Dev.has(sycl::aspect::usm_device_allocations) == false) {
	!Dev.has(sycl::aspect::usm_device_allocations)) {

raise exception in malloc when device does not supports usm #10348

raise exception in malloc when device does not supports usm #10348

Conversation

pwisniewskimobica commented Jul 13, 2023

al42and commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 15, 2024

github-actions bot commented Oct 16, 2024