-
Notifications
You must be signed in to change notification settings - Fork 124
Rename INVALID_FUNCTION_NAME to FUNCTION_ADDRESS_NOT_AVAILABLE. #1136
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
Rename INVALID_FUNCTION_NAME to FUNCTION_ADDRESS_NOT_AVAILABLE. #1136
Conversation
210120d to
98624fc
Compare
|
I have updated the target branch of this PR from the |
18d229b to
2dbd1e0
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1136 +/- ##
=======================================
Coverage 15.72% 15.73%
=======================================
Files 223 223
Lines 31475 31476 +1
Branches 3557 3557
=======================================
+ Hits 4951 4954 +3
+ Misses 26473 26472 -1
+ Partials 51 50 -1 ☔ View full report in Codecov by Sentry. |
2dbd1e0 to
0667a9a
Compare
|
friendly ping @oneapi-src/unified-runtime-maintain @oneapi-src/unified-runtime-opencl-write @oneapi-src/unified-runtime-cuda-write @oneapi-src/unified-runtime-native-cpu-write @oneapi-src/unified-runtime-hip-write @oneapi-src/unified-runtime-level-zero-write @oneapi-src/unified-runtime-command-buffer-write the changes are insignificant despite apparently touching absolutely everything |
0667a9a to
f19816d
Compare
PietroGhg
left a comment
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.
Native CPU lgtm, thank you
|
What happens if the function name can't be found in the program? |
we use UR_RESULT_ERROR_INVALID_KERNEL_NAME for that |
nrspruit
left a comment
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.
Level zero LGTM
|
ping @oneapi-src/unified-runtime-cuda-write @oneapi-src/unified-runtime-hip-write |
source/adapters/cuda/program.cpp
Outdated
| if (Ret == CUDA_ERROR_NOT_FOUND) { | ||
| *ppFunctionPointer = 0; | ||
| Result = UR_RESULT_ERROR_INVALID_FUNCTION_NAME; | ||
| Result = UR_RESULT_ERROR_FUNCTION_ADDRESS_NOT_AVAILABLE; |
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.
It looks to me like this mapping isn't quite right:
UR_RESULT_ERROR_FUNCTION_ADDRESS_NOT_AVAILABLE
If pFunctionName could be located, but its address couldn't be retrieved.
But see :
- ... If no function of that name exists, cuModuleGetFunction() returns CUDA_ERROR_NOT_FOUND.
To me it seems contradictory for the functionname to be located but it's address not being able to be retrieved when cuda docs state that no function with that name exists?
I think what you have here is fine from a general point of view wrt cuda/hip backends: you have a cuda/hip error that can be returned from multiple driver functions, but it's semantics change depending on which function returned it, so you are mapping it to a UR error that gives more info. I think this is a fine idea (although normally I think it is better to return plugin specific errors with the native error in cases where the native error is more clear than a UR mapping); it just looks like the mapping isn't quite right currently.
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.
Only just read @ldrumm s comment which is making the point a lot more succinctly: #1136 (comment)
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.
Thinking about this further. Maybe it is better to append e.g.:
+ void setPluginSpecificMessage(CUresult cu_res, UR_RESULT UR_res) {
- void setPluginSpecificMessage(CUresult cu_res) {
const char *error_string;
const char *error_name;
cuGetErrorName(cu_res, &error_name);
cuGetErrorString(cu_res, &error_string);
char *message = (char *)malloc(strlen(error_string) + strlen(error_name) + 2);
strcpy(message, error_name);
strcat(message, "\n");
strcat(message, error_string);
+ setErrorMessage(message, UR_res);
- setErrorMessage(message, UR_RESULT_ERROR_ADAPTER_SPECIFIC);
free(message);
}
from
Then this gives the option of passing in a UR_RESULT (or whatever it is called) different from UR_RESULT_ERROR_ADAPTER_SPECIFIC (in such cases that there is a clarifying mapping as I described in my first comment).
Then you can
- preserve the native error
- clarify it by providing a UR mapping if one exists
- If one doesn't exist (which might be the case in this example from your code) then you can pass in UR_RESULT_ERROR_ADAPTER_SPECIFIC, possibly with an accompanying message.
If such a protocol were decided across backends, then documented, this would make it easier to prevent such errors as this example in the future (i.e making mappings from native errors to UR that don't make sense), and provide a consistent error message framework for contributors to use.
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.
yeah FUNCTION_ADDRESS_NOT_AVAILABLE is a CL exclusive failure mode, it must have proliferated when it got renamed to be more generic, I've replaced all uses of it outside of the CL adapter with INVALID_KERNEL_NAME
This lines up better with the original PI error code and what it was meant to convey. Also clarify the spec around return codes for urProgramGetFunctionPointer and update the relevant CTS test.
f19816d to
11d5613
Compare
|
merged into #1212 |
This lines up better with the original PI error code and what it was meant to convey.
Also clarify the spec around return codes for urProgramGetFunctionPointer and update the relevant CTS test.
LLVM testing intel/llvm#12263