Skip to content

[SYCL][CUDA] Make piextKernelSetArgMemObj setting an error message #6521

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 1 commit into from
Sep 9, 2022

Conversation

pgorlani
Copy link
Contributor

@pgorlani pgorlani commented Aug 3, 2022

This patch makes cuda_piextKernelSetArgMemObj setting an error message instead of std::terminate in case of the image format is not supported. This error message is encapsulated in an exception thrown by the RT.

This allows to continue the SYCL-CTS execution in case of tests using unsupported channel types, see #2119 (comment).

@pgorlani pgorlani requested a review from a team as a code owner August 3, 2022 13:27
@pgorlani pgorlani requested a review from sergey-semenov August 3, 2022 13:27
@@ -337,12 +337,9 @@ namespace detail {
namespace pi {

// Report error and no return (keeps compiler from printing warnings).
// TODO: Probably change that to throw a catchable exception,
// but for now it is useful to see every failure.
//
[[noreturn]] void die(const char *Message) {
std::cerr << "pi_die: " << Message << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to hear what others think about pushing messages to std::cerr here instead of attaching it to the exception.

Copy link
Contributor Author

@pgorlani pgorlani Aug 3, 2022

Choose a reason for hiding this comment

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

Yes, me too. I followed the same way of check_error, i.e.,

cuGetErrorString(result, &errorString);
std::stringstream ss;
ss << "\nPI CUDA ERROR:"
<< "\n\tValue: " << result
<< "\n\tName: " << errorName
<< "\n\tDescription: " << errorString
<< "\n\tFunction: " << function << "\n\tSource Location: " << file
<< ":" << line << "\n"
<< std::endl;
std::cerr << ss.str();
}
if (std::getenv("PI_CUDA_ABORT") != nullptr) {
std::abort();
}
throw map_error(result);
}

Note: In case, I could add the check of PI_CUDA_ABORT to pie::die.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we don't call std::terminate here anymore, I would definitely prefer attaching the message to the exception and leave it to the caller to handle however they see fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized I took the wrong approach modifying the pi::die function. Basically a function that std::terminate could be needed, also the other PIs have it.

In order to solve #2119 (comment), I made cuda_piextKernelSetArgMemObj returning an error and setting a message.
In such a way the RT will throw an exception with the specified message allowing SYCL-CTS to continue the test execution, i.e.,

../util/sycl_exceptions.h:252: warning:
  SYCL exception
  with category name: 'sycl'
  with code: 'runtime'
  with code raw value: '1'
  with code message: 'SYCL Error'
  with what: 'Native API failed. Native API returns: -996 (Function exists but
  address is not available)
  PI CUDA kernels only support images with channel types int32, uint32, float,
  and half.
   -996 (Function exists but address is not available)'

../util/test_base.cpp:28: FAILED:
explicitly with message:
  a SYCL exception was caught: Native API failed. Native API returns: -996
  (Function exists but address is not available)
  PI CUDA kernels only support images with channel types int32, uint32, float,
  and half.
   -996 (Function exists but address is not available)

@pgorlani pgorlani force-pushed the picuda-die-with-exception branch from 8450d87 to c7712f3 Compare August 4, 2022 17:01
@pgorlani pgorlani changed the title [SYCL][CUDA] Make pi::die throwing an exception [SYCL][CUDA] Make piextKernelSetArgMemObj throwing an exception Aug 4, 2022
@pgorlani
Copy link
Contributor Author

pgorlani commented Sep 6, 2022

Hi @bader, are you happy with the proposed solution?

@bader
Copy link
Contributor

bader commented Sep 6, 2022

Hi @bader, are you happy with the proposed solution?

I'm okay with it.
@sergey-semenov, please, take a look.

@smaslov-intel
Copy link
Contributor

[SYCL][CUDA] Make piextKernelSetArgMemObj throwing an exception

Please update comment as this is not throwing an exception now

@pgorlani pgorlani changed the title [SYCL][CUDA] Make piextKernelSetArgMemObj throwing an exception [SYCL][CUDA] Make piextKernelSetArgMemObj setting an error message Sep 6, 2022
@pgorlani
Copy link
Contributor Author

pgorlani commented Sep 6, 2022

[SYCL][CUDA] Make piextKernelSetArgMemObj throwing an exception

Please update comment as this is not throwing an exception now

@smaslov-intel Done.

@bader bader merged commit 2542e6a into intel:sycl Sep 9, 2022
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