[UR] Update spec to make kernel argument validation in urEnqueueKernelLaunch optional #17068
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RossBrunton
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
RossBrunton
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aarongreig
reviewed
Feb 24, 2025
Several adapters don't support validating kernel signatures when enqueued. To handle this, we now allow urEnqueueKernelLaunch to return `SUCCESS` even when parameters are invalid. Some tests have also been updated. The CUDA adapter has also been updated to handle invalid arguments.
RossBrunton
force-pushed
the
ross/new-argval
branch
from
604c599 to
3fc0d49
Compare
RossBrunton
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aarongreig
approved these changes
Feb 24, 2025
RossBrunton
changed the title
npmiller
reviewed
Feb 24, 2025
| /// @details | ||
| /// - Adapters may perform validation on the number of arguments set to the | ||
| /// kernel, but are not required to do so and may return | ||
| /// `::UR_RESULT_SUCCESS` even for invalid invocations. |
There was a problem hiding this comment.
I wonder if we should just say that passing incorrect kernel arguments is "undefined behavior" or something like that.
As shown in the test, if an adapter is unable to validate the arguments it's unlikely to be able to gracefully return success as well.
If we want to keep some level of argument validation, maybe it could be guarded behind a device property?
There was a problem hiding this comment.
I'm not sure the benefits of marking it as UB are worthwhile, especially since I don't think we have (explicit) UB anywhere else in UR. Cuda seems to be the only target with major issues with invalid arguments, do you see that resulting in UB?
RossBrunton
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Migrated from oneapi-src/unified-runtime#2564
Several adapters don't support validating kernel signatures when
enqueued. To handle this, we now allow urEnqueueKernelLaunch to return
SUCCESSeven when parameters are invalid.