-
Notifications
You must be signed in to change notification settings - Fork 115
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
rocr: Allow 0/NULL/invalid signal handles for wait operations to be no-op #296
base: amd-staging
Are you sure you want to change the base?
Conversation
…o-op Remove hard assertions for signal validation on hsa_amd_signal_wait_* operations, instead ignore 0/NULL/invalid signals in the dependency condition evaluation to align with HSA specs for barrier-AND and barrier-OR packets. satisfying_values of 0/NULL/invalid signals are set to 0 for hsa_amd_signal_wait_all. Signed-off-by: zichguan-amd <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thanks!
I have a few minor requests.
Signed-off-by: zichguan-amd <[email protected]>
Signed-off-by: zichguan-amd <[email protected]>
591ee25
to
195cd02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should return UINT32_MAX for the error cases. @atgutier what do you think?
// Return if there's no valid signal to wait on | ||
// satisfying_value is ignored | ||
if (valid_signals.empty()){ | ||
return uint32_t(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return UINT32_MAX to differentiate with the first signal satisfying the criteria?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that is a good idea, but would use c++ std::numeric_limits<uint32_t>::max().
@zichguan-amd can you update this and reflect in the comments that the return will be max if there were no valid signals and then the user can decide to move on if that is ok/expected or can generate an error or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
@@ -578,7 +578,7 @@ uint32_t hsa_amd_signal_wait_all(uint32_t signal_count, hsa_signal_t* hsa_signal | |||
TRY; | |||
if (!core::Runtime::runtime_singleton_->IsOpen()) { | |||
assert(false && "hsa_amd_signal_wait_all called while not initialized."); | |||
return 0; | |||
return uint32_t(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, should this return UIN32T_MAX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I also modify the return value for wait_any in case the runtime is not initialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change both and document it
Remove hard assertions for signal validation on hsa_amd_signal_wait_* operations, instead ignore 0/NULL/invalid signals in the dependency condition evaluation to align with HSA specs for barrier-AND and barrier-OR packets. satisfying_values of 0/NULL/invalid signals are set to 0 for hsa_amd_signal_wait_all.