-
Notifications
You must be signed in to change notification settings - Fork 794
[UR][SYCL] Replace urEnqueueKernelLaunch property list with pNext chain property structs #20596
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
base: sycl
Are you sure you want to change the base?
Conversation
5e31e12 to
b0df01e
Compare
b0df01e to
a000354
Compare
a000354 to
1ae0f9d
Compare
1ae0f9d to
01c7fc1
Compare
01c7fc1 to
2a106f0
Compare
2a106f0 to
5f3a8b9
Compare
5f3a8b9 to
94998d8
Compare
| return UR_RESULT_ERROR_UNSUPPORTED_FEATURE; | ||
| } | ||
|
|
||
| while (_launchPropList != nullptr) { |
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.
You can parse those properties using find_stype_node (like here https://github.com/intel/llvm/blob/sycl/unified-runtime/source/adapters/level_zero/usm.cpp#L1052).
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 it cannot be used in this place, since we are not looking for a specific type here. Is it right?
94998d8 to
5dc229b
Compare
5dc229b to
d1589af
Compare
|
Rebased |
17fdcc7 to
889ed38
Compare
889ed38 to
017d5f3
Compare
| name: numPropsInLaunchPropList | ||
| desc: "[in] size of the launch prop list" | ||
| - type: const $x_kernel_launch_property_t* | ||
| - type: const $x_kernel_launch_ext_properties_t* |
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.
My understanding was that it was only experimental interfaces that were allowed to change arguments. Is that not the case?
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.
@pbalcer ?
017d5f3 to
a512bd9
Compare
|
Just for future reference, it would be preferable if you avoid doing force-pushes. It makes if more difficult to track what changes between reviews. Merges squash commits and uses the description and title of the PR, so it will still become a single commit in the end. |
|
The |
That is interesting. It looks like it is related to #20610, but UR CI don't run for libclc, so I don't think we caught it as part of that. @jinge90 - Are the changes it suggest valid? It doesn't look valid to me. |
steffenlarsen
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.
SYCL RT, NativeCPU, CUDA and HIP changes LGTM!
We can ignore it, the linked PR (not this one) caused some nonsense on Windows. @ldorau Please rebase on top of HEAD and run CI again |
107a00b to
832ea04
Compare
Rebased. |
|
FYI, the failures happening in https://github.com/intel/llvm/actions/runs/19344092070/job/55340836099?pr=20596 seem to be due to a race condition that has existed since the V1/V2 adapters for L0 were allowed to be "skipped". The adapters were not being properly torndown resulting in sporadic crashes. This fixes that problem #20642 . To fix that issue you will need to merge my PR first. |
nevermind, my fix does resolve an issue, but this sporadic UR only issue in V2 persists. I at least cannot reproduce the hang outside of the CI. |
mmichel11
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.
Command buffer changes LGTM
…ty structs Signed-off-by: Lukasz Dorau <[email protected]>
Replace
urEnqueueKernelLaunch()property list withpNextchain property structures.It resolves Jira URT-990