[UR][CUDA][HIP] Remove duplicated parameter checking #17984
Conversation
Most of the parameter checking is done in the UR validation layer, so we don't need to re-do it in the adapters. The adapters should only do parameter checking that can't be done in the validation layer.
| UR_ASSERT((widthInBytes > 0), UR_RESULT_ERROR_INVALID_VALUE); | ||
| UR_ASSERT((height > 0), UR_RESULT_ERROR_INVALID_VALUE); | ||
| UR_ASSERT((elementSizeBytes > 0), UR_RESULT_ERROR_INVALID_VALUE); |
There was a problem hiding this comment.
Is it generally preferable to have as many checks as possible in the validation layer?
The other checks highlighted here could easily also be moved similarly to the validation layer. Is this something we should inform teams of doing, or do ourselves?
Otherwise, LGTM!
Edit: Interesting to note the difference between the validation layer check == 0 vs the check in the adapter > 0 which works out to be fine for the size_t here. However, they also report different error values UR_RESULT_ERROR_INVALID_USM_SIZE in the validation layer vs UR_RESULT_ERROR_INVALID_VALUE in the adapter.
There was a problem hiding this comment.
Yes! With the caveat that not all checks can be done in the validation layer, and also that the validation layer applies to all adapters, and should only check things properly defined in the specification.
The validation layer is auto-generated based on the yaml specification along with the docs and headers, so it will always have the checks and the specification consistent for example.
I have a small list of cases including this one of checks that could/should be moved up, but they'll need more discussion to see if they make sense in the spec or if they're just for cuda/hip.
|
@intel/llvm-gatekeepers this is ready to merge |
Most of the parameter checking is done in the UR validation layer, so we don't need to re-do it in the adapters.
The adapters should only do parameter checking that can't be done in the validation layer.