Skip to content

[HIP] Refactor error handling in enqueue.cpp #1190

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 7, 2024
Merged

[HIP] Refactor error handling in enqueue.cpp #1190

merged 2 commits into from
Oct 7, 2024

Conversation

aarongreig
Copy link
Contributor

@aarongreig aarongreig commented Dec 14, 2023 •
edited
Loading

LLVM testing intel/llvm#12481

@aarongreig aarongreig force-pushed the aaron/refactorHipEnqueueErrors branch 2 times, most recently from f614c49 to c451bf2 Compare December 15, 2023 10:52
@aarongreig aarongreig marked this pull request as ready for review December 15, 2023 10:52
@aarongreig aarongreig requested a review from a team as a code owner December 15, 2023 10:52
@aarongreig aarongreig requested a review from npmiller December 15, 2023 10:52
@aarongreig aarongreig force-pushed the aaron/refactorHipEnqueueErrors branch from c451bf2 to f837719 Compare December 15, 2023 11:27
@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b66cf9b) 15.46% compared to head (f989b69) 15.46%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1190   +/-   ##
=======================================
  Coverage   15.46%   15.46%           
=======================================
  Files         238      238           
  Lines       33883    33883           
  Branches     3747     3747           
=======================================
  Hits         5239     5239           
  Misses      28593    28593           
  Partials       51       51

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hdelan
hdelan approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@hdelan hdelan left a comment

Choose a reason for hiding this comment

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

LGTM

jchlanda
jchlanda approved these changes Dec 19, 2023
View reviewed changes
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.

Really like this, needless accumulation of error codes in Result-like variables, instead of straight out returns, and not using UR_CHECK_ERROR are one of my top pet hates. Thank you!

@aarongreig aarongreig force-pushed the aaron/refactorHipEnqueueErrors branch from f837719 to 0011dd3 Compare January 23, 2024 10:33
@aarongreig
Copy link
Contributor Author

@npmiller please take a look when you can

npmiller
npmiller approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@npmiller npmiller left a comment

Choose a reason for hiding this comment

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

LGTM, sorry about the late review!

@aarongreig aarongreig mentioned this pull request Jan 24, 2024
Closed
@aarongreig aarongreig force-pushed the aaron/refactorHipEnqueueErrors branch from a2c1f16 to f989b69 Compare January 26, 2024 10:53
@kbenzie kbenzie added the hip HIP adapter specific issues label Apr 10, 2024
@aarongreig aarongreig force-pushed the aaron/refactorHipEnqueueErrors branch from f989b69 to 5ebd333 Compare September 25, 2024 16:30
GeorgeWeb
GeorgeWeb reviewed Sep 25, 2024
View reviewed changes
source/adapters/hip/enqueue.cpp Outdated
// but currently unmapped advice arguments as not supported by this
// platform. Therefore, warn the user instead of throwing and aborting
// the runtime.
if (Result == UR_RESULT_ERROR_INVALID_ENUMERATION) {
Copy link
Contributor

@GeorgeWeb GeorgeWeb Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I may be missing something here but is this if (Result == UR_RESULT_ERROR_INVALID_ENUMERATION) meant to be checked again inside the above if (Result == UR_RESULT_ERROR_INVALID_ENUMERATION)? It is already true in this case. Was it meant to be wrapped in if (Result != UR_RESULT_SUCCESS) -> check if equals invalid then msg and return else throw?

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 think I messed up the rebase here, now it just falls through to UR_CHECK_ERROR(Result) if we aren't doing the adapter specific thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that makes sense to me now. Thanks, I actually thuoght it must've been a dodgy rebase by the looks of it.

GeorgeWeb
GeorgeWeb reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor

@GeorgeWeb GeorgeWeb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I know I am super late to the party here. Generally looks good. Thank you for cleaning that up!
Just a little comment I was confused about.

@aarongreig aarongreig added the ready to merge Added to PR's which are ready to merge label Sep 26, 2024
@aarongreig
[HIP] Refactor error handling in enqueue.cpp
1da316c 
Mostly by taking the existing try/catch/UR_CHECK_ERROR based approach
and making sure it's used consistently so as not to drop any errors.
@aarongreig
Fix dodgy rebase.
96468e2
@aarongreig aarongreig force-pushed the aaron/refactorHipEnqueueErrors branch from d070776 to 96468e2 Compare October 3, 2024 10:49
@aarongreig aarongreig merged commit a9645bd into oneapi-src:main Oct 7, 2024
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jchlanda jchlanda jchlanda approved these changes

@GeorgeWeb GeorgeWeb GeorgeWeb approved these changes

@npmiller npmiller npmiller approved these changes

+1 more reviewer

@hdelan hdelan hdelan approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
hip HIP adapter specific issues ready to merge Added to PR's which are ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@aarongreig @codecov-commenter @jchlanda @GeorgeWeb @npmiller @hdelan @kbenzie