Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Correct level of indirection used in KernelSetArgPointer calls. #805

Merged
merged 5 commits into from
Jun 21, 2024
Merged

Correct level of indirection used in KernelSetArgPointer calls. #805

merged 5 commits into from
Jun 21, 2024

Conversation

aarongreig
Copy link
Contributor

@aarongreig aarongreig commented Aug 15, 2023 •
edited
Loading

Also attempt to clarify the wording around this a bit.

fixes #558

LLVM testing intel/llvm#12270

@aarongreig
Copy link
Contributor Author

I was able to properly test this on more than just cuda this morning and I can confirm this will need a small patch for the cuda and opencl adapters when it gets into intel-llvm. Maybe merging this should wait until the adapters move in here and they can be patched simultaneously?

@kbenzie
Copy link
Contributor

kbenzie commented Aug 28, 2023

Maybe merging this should wait until the adapters move in here and they can be patched simultaneously?

That makes sense, convert this to draft so we don't merge it unintentionally.

@aarongreig aarongreig marked this pull request as draft August 28, 2023 11:29
@aarongreig aarongreig force-pushed the aaron/kernelSetArgIndirectionFix branch from bface7e to 740f676 Compare December 29, 2023 16:01
@aarongreig aarongreig mentioned this pull request Dec 29, 2023
Merged
@aarongreig aarongreig force-pushed the aaron/kernelSetArgIndirectionFix branch from 740f676 to 1178193 Compare December 29, 2023 16:19
@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2023 •
edited
Loading

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (fb83446) 15.72% compared to head (1c7d0c7) 15.72%.

Files Patch % Lines
test/conformance/kernel/urKernelSetArgPointer.cpp 0.00% 5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #805   +/-   ##
=======================================
  Coverage   15.72%   15.72%           
=======================================
  Files         223      223           
  Lines       31475    31475           
  Branches     3557     3557           
=======================================
  Hits         4951     4951           
  Misses      26473    26473           
  Partials       51       51

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aarongreig aarongreig force-pushed the aaron/kernelSetArgIndirectionFix branch 2 times, most recently from 1de74d7 to 1c7d0c7 Compare December 29, 2023 16:41
@aarongreig aarongreig mentioned this pull request Dec 29, 2023
Merged
@aarongreig aarongreig marked this pull request as ready for review December 29, 2023 17:08
@aarongreig aarongreig requested review from a team as code owners December 29, 2023 17:08
@aarongreig aarongreig mentioned this pull request Jan 10, 2024
Closed
nrspruit
nrspruit approved these changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@nrspruit nrspruit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for level zero

@kbenzie kbenzie added specification Changes or additions to the specification cuda CUDA adapter specific issues hip HIP adapter specific issues opencl OpenCL adapter specific issues native-cpu Native CPU adapter specific issues conformance Conformance test suite issues. labels Apr 10, 2024
@kbenzie
Copy link
Contributor

kbenzie commented Apr 30, 2024

@oneapi-src/unified-runtime-native-cpu-write @mmoadeli would be great to get your reviews on this.

PietroGhg
PietroGhg approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@PietroGhg PietroGhg left a 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

@kbenzie
Copy link
Contributor

kbenzie commented May 6, 2024

@oneapi-src/unified-runtime-cuda-write @oneapi-src/unified-runtime-hip-write would appriciate a review on this, the CUDA/HIP changes are very small.

jchlanda
jchlanda approved these changes May 6, 2024
View reviewed changes
Copy link
Contributor

@jchlanda jchlanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CUDA/HIP 👍

@kbenzie kbenzie added the ready to merge Added to PR's which are ready to merge label May 6, 2024
@aarongreig aarongreig force-pushed the aaron/kernelSetArgIndirectionFix branch from 1c7d0c7 to d2cc130 Compare May 7, 2024 14:29
@github-actions github-actions bot added loader Loader related feature/bug level-zero L0 adapter specific issues labels May 7, 2024
@aarongreig aarongreig force-pushed the aaron/kernelSetArgIndirectionFix branch from d2cc130 to c409f71 Compare May 16, 2024 15:28
@aarongreig aarongreig requested a review from a team as a code owner May 16, 2024 15:28
@aarongreig aarongreig force-pushed the aaron/kernelSetArgIndirectionFix branch 2 times, most recently from f5ea371 to c409f71 Compare May 20, 2024 09:28
@github-actions github-actions bot added the sanitizer Sanitizer layer issues/changes/specification label May 20, 2024
@aarongreig aarongreig force-pushed the aaron/kernelSetArgIndirectionFix branch from 884367b to 1db31e7 Compare May 20, 2024 15:14
@aarongreig aarongreig mentioned this pull request May 20, 2024
Merged
@kbenzie kbenzie removed the ready to merge Added to PR's which are ready to merge label May 21, 2024
@kbenzie
Copy link
Contributor

kbenzie commented May 21, 2024

I've removed ready to merge since the CUDA checks aren't passing, please add it again once they go green.

@aarongreig aarongreig force-pushed the aaron/kernelSetArgIndirectionFix branch from 1db31e7 to acb3dda Compare May 31, 2024 15:05
@aarongreig aarongreig added the ready to merge Added to PR's which are ready to merge label Jun 3, 2024
@kbenzie kbenzie force-pushed the aaron/kernelSetArgIndirectionFix branch from acb3dda to 938c533 Compare June 12, 2024 21:29
@kbenzie kbenzie force-pushed the aaron/kernelSetArgIndirectionFix branch from 938c533 to 5698e57 Compare June 18, 2024 17:00
@kbenzie kbenzie merged commit 1e9b1b4 into oneapi-src:main Jun 21, 2024
49 of 51 checks passed
steffenlarsen pushed a commit to intel/llvm that referenced this pull request Jun 21, 2024
@aarongreig @kbenzie
[UR] Pull in changes from UR PR #805 (#12270)
088a947 
oneapi-src/unified-runtime#805

---------

Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PietroGhg PietroGhg PietroGhg approved these changes

@martygrant martygrant martygrant approved these changes

@nrspruit nrspruit nrspruit approved these changes

@fabiomestre fabiomestre fabiomestre approved these changes

@EwanC EwanC EwanC approved these changes

@kbenzie kbenzie kbenzie approved these changes

@jchlanda jchlanda jchlanda approved these changes

@mmoadeli mmoadeli Awaiting requested review from mmoadeli mmoadeli is a code owner automatically assigned from oneapi-src/unified-runtime-hip-write

Assignees
No one assigned
Labels
conformance Conformance test suite issues. cuda CUDA adapter specific issues hip HIP adapter specific issues level-zero L0 adapter specific issues loader Loader related feature/bug native-cpu Native CPU adapter specific issues opencl OpenCL adapter specific issues ready to merge Added to PR's which are ready to merge sanitizer Sanitizer layer issues/changes/specification specification Changes or additions to the specification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clarify urKernelSetArgPointer spec
9 participants
@aarongreig @kbenzie @codecov-commenter @jchlanda @EwanC @fabiomestre @nrspruit @martygrant @PietroGhg