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.

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

[SYCL][CUDA] Make plugin specific error return an error #10626

Merged
merged 5 commits into from
Aug 2, 2023

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Jul 31, 2023

The UR_RESULT_ADAPTER_SPECIFIC_ERROR was not returning an error to the SYCL RT which meant all errors were treated as warnings and ignored unless SYCL_RT_WARNING_LEVEL is set to geq 2. This changes things so the adapter specific error is now reported as such, meaning all uses UR_RESULT_ADAPTER_SPECIFIC_ERROR meant as warnings are now caught as errors.

@hdelan hdelan requested review from a team as code owners July 31, 2023 14:51
@hdelan hdelan requested a review from sergey-semenov July 31, 2023 14:51
@ldrumm
Copy link
Contributor

ldrumm commented Jul 31, 2023

Most of your description should be in the commit message proper. Code looks good to me.

Also, because of how this project has been configured, your mention of steffen will end up in project history. I've tried arguing against that without success, but for now you should treat descriptions on github as if they will end up in git history (because they will)

@hdelan
Copy link
Contributor Author

hdelan commented Jul 31, 2023

(Editing after @ldrumm has mentioned we should move conversation out of PR description to avoid contaminating the commit message.)

I am not sure if this is desirable behaviour. @steffenlarsen what do you think?

I have also added a test to check that the SYCL_PI_CUDA_MAX_LOCAL_MEM_SIZE env variable can fail reliably.

@hdelan hdelan temporarily deployed to aws July 31, 2023 15:06 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to aws July 31, 2023 15:33 — with GitHub Actions Inactive
@steffenlarsen
Copy link
Contributor

I am not sure if this is desirable behaviour. @steffenlarsen what do you think?

What behavior? @ldrumm is right that the description is what normally ends up as the commit message as that's what is used when we squash the commits. This is the intended behavior, so it would indeed be preferable if you keep the description in line with how you want the final commit to appear and avoid tagging inside it.

@hdelan
Copy link
Contributor Author

hdelan commented Jul 31, 2023

Sorry I meant the behaviour introduced by this PR. I have actually changed the behaviour so UR_RESULT_ADAPTER_SPECIFIC_ERROR can correctly error out if setErrorMessage is called with UR_RESULT_ADAPTER_SPECIFIC_ERROR as second arg, and continue if UR_RESULT_SUCCESS is second arg. So everything working as intended.

As a sidenote I think for clarity it might be nice to split UR_RESULT_ADAPTER_SPECIFIC_ERROR into two: UR_RESULT_ADAPTER_SPECIFIC_ERROR and UR_RESULT_ADAPTER_SPECIFIC_WARNING so the semantics of setErrorMessage was a bit clearer.

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Good catch! I believe this was indeed the intended behavior. Tag @smaslov-intel on his thoughts as well as for the split error code for distinguishing between warnings and errors.

@hdelan hdelan temporarily deployed to aws July 31, 2023 16:05 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to aws July 31, 2023 16:50 — with GitHub Actions Inactive
@hdelan hdelan temporarily deployed to aws July 31, 2023 17:30 — with GitHub Actions Inactive
Copy link
Contributor

@jchlanda jchlanda left a comment

Choose a reason for hiding this comment

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

👍 for error/warning split

@hdelan
Copy link
Contributor Author

hdelan commented Aug 1, 2023

@intel/llvm-gatekeepers This can be merged. I'll make a new ticket to track the UR_RESULT_ADAPTER_SPECIFIC_ERROR vs UR_RESULT_ADAPTER_SPECIFIC_WARNING

@hdelan
Copy link
Contributor Author

hdelan commented Aug 1, 2023

@steffenlarsen
Copy link
Contributor

@intel/dpcpp-l0-pi-reviewers approval is missing. Friendly ping.

@JackAKirk
Copy link
Contributor

@intel/llvm-gatekeepers can this now be merged?

@steffenlarsen steffenlarsen merged commit b7a43a4 into intel:sycl Aug 2, 2023
fabiomestre pushed a commit to fabiomestre/llvm that referenced this pull request Sep 26, 2023
The `UR_RESULT_ADAPTER_SPECIFIC_ERROR` was not returning an error to the
SYCL RT which meant all errors were treated as warnings and ignored
unless `SYCL_RT_WARNING_LEVEL` is set to geq 2. This changes things so
the adapter specific error is now reported as such, meaning all uses
`UR_RESULT_ADAPTER_SPECIFIC_ERROR` meant as warnings are now caught as
errors.

---------

Co-authored-by: Hugh Delaney <[email protected]>
mdtoguchi pushed a commit to mdtoguchi/llvm that referenced this pull request Oct 18, 2023
The `UR_RESULT_ADAPTER_SPECIFIC_ERROR` was not returning an error to the
SYCL RT which meant all errors were treated as warnings and ignored
unless `SYCL_RT_WARNING_LEVEL` is set to geq 2. This changes things so
the adapter specific error is now reported as such, meaning all uses
`UR_RESULT_ADAPTER_SPECIFIC_ERROR` meant as warnings are now caught as
errors.

---------

Co-authored-by: Hugh Delaney <[email protected]>
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.

6 participants