-
Notifications
You must be signed in to change notification settings - Fork 112
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
[CUDA] Update hint functions to only return warnings instead of errors #989
[CUDA] Update hint functions to only return warnings instead of errors #989
Conversation
fabiomestre
commented
Oct 23, 2023
•
edited
Loading
edited
- The UR spec was recently changed to guarantee that hint entryponts never return errors. This commit changes the CUDA adapter to be conformant with this change.
- This commit also changes the type of PointerRangeSize which was causing a stack corruption.
Actually @fabiomestre after speaking to @GeorgeWeb I realize that what he has done here #1027
instead of replacing it with the is_supported check. |
Correct, the point is that Now, the thing that is not conforming with the definition in the UR spec wrt advise hints is the |
source/adapters/cuda/enqueue.cpp
Outdated
CU_MEM_ADVISE_UNSET_ACCESSED_BY, | ||
hQueue->getContext()->getDevice()->get())); | ||
} else { | ||
Result = setCuMemAdvise((CUdeviceptr)pMem, size, advice, |
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 function (setCuMemAdvise
) can throw if an unsupported advice flag is passed, but it should not as suggested by the UR spec. Hence, we should have it return the result code within the function instead of throw
ing an error, and then handle it here by checking the value of Result
and likely setting a warning with SUCCESS - that is if we want to verify and warn on unsupported advice flags, otherwise we can omit checking if the flags are supported altogether if that's more sensible to you but having a warning on the user-end is always nice.
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 have updated the PR to return UR_RESULT_ERROR_ADAPTER_SPECIFIC in this case
From the point of view of the current implementation, I think that makes sense. I wasn't aware that SYCL RT is treating UR_RESULT_ERROR_ADAPTER_SPECIFIC as a warning if it returns UR_RESULT_SUCCESS. It is quite a misleading error code and the UR spec is not clear about this. I will just confirm with the team that this is the behaviour that we expect from UR going forward. If it is, I'm happy to change this PR as you suggested. P.S. Sorry, I somehow edited your reply by mistake |
- The UR spec was recently changed to make hint entryponts always return UR_RESULT_SUCCESS. This commit changes the CUDA adapter to be conformant with this change. - This commit also changes the type of PointerRangeSize which was causing a stack corruption.
9c77328
to
e1902fc
Compare
LGTM! One last thing - I am not too verse with the merging policies in this repo, but it may be worth rewording the PR name (and description) now that it is addressing a different thing (replacing error throwing with setting a warning in the memadvise API). |
Thanks Georgi. I updated the title. I would expect that this will be merged in separate PR. So, will write a better commit message when I create it. |