[SPEC] Add urProgramGetGlobalVariablePointer entrypoint #1255

fabiomestre · 2024-01-16T14:39:14Z

Adds new entrypoint urProgramGetGlobalVariablePointer.
Adds conformance tests for the new entrypoint.
Adds OpenCL, CUDA, HIP and Level Zero implementations for urProgramGetGlobalVariablePointer
Fixes existing testing urEnqueueDeviceGetGlobalVariableRead by adding the required metadata and sycl properties to the kernel.

Closes: #1214

codecov-commenter · 2024-01-16T14:56:29Z

Codecov Report

Attention: Patch coverage is 5.88235% with 128 lines in your changes are missing coverage. Please review.

Project coverage is 12.47%. Comparing base (78ef1ca) to head (6297b7f).
Report is 151 commits behind head on main.

Files	Patch %	Lines
include/ur_print.hpp	0.00%	32 Missing ⚠️
...ance/program/urProgramGetGlobalVariablePointer.cpp	0.00%	23 Missing ⚠️
source/loader/layers/validation/ur_valddi.cpp	12.50%	21 Missing ⚠️
source/loader/layers/tracing/ur_trcddi.cpp	23.07%	10 Missing ⚠️
source/loader/ur_ldrddi.cpp	9.09%	10 Missing ⚠️
source/loader/ur_libapi.cpp	0.00%	8 Missing ⚠️
test/conformance/testing/include/uur/fixtures.h	0.00%	8 Missing ⚠️
source/adapters/null/ur_nullddi.cpp	12.50%	7 Missing ⚠️
source/loader/ur_print.cpp	0.00%	4 Missing ⚠️
test/conformance/source/environment.cpp	0.00%	4 Missing ⚠️
... and 1 more

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1255      +/-   ##
==========================================
- Coverage   14.82%   12.47%   -2.36%     
==========================================
  Files         250      240      -10     
  Lines       36220    36126      -94     
  Branches     4094     4097       +3     
==========================================
- Hits         5369     4506     -863     
- Misses      30800    31616     +816     
+ Partials       51        4      -47

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

source/adapters/cuda/program.cpp

source/adapters/hip/program.cpp

hdelan

Thanks for changes. CUDA/HIP adapters LGTM

fabiomestre · 2024-01-22T14:24:05Z

Gentle ping for reviews from @oneapi-src/unified-runtime-maintain / @oneapi-src/unified-runtime-opencl-write / @oneapi-src/unified-runtime-level-zero-write

nrspruit

LGTM for level-zero

aarongreig · 2024-01-23T15:05:59Z

source/adapters/opencl/program.cpp

+      CLContext, cl_ext::ExtFuncPtrCache->clGetDeviceGlobalVariablePointerCache,
+      cl_ext::GetDeviceGlobalVariablePointerName, &FuncT));
+
+  if (!FuncT) {


this already gets checked in getExtFuncFromContext

aarongreig · 2024-01-23T15:08:59Z

test/conformance/program/urProgramGetGlobalVariablePointer.cpp

+TEST_P(urProgramGetGlobalVariablePointerTest, Success) {
+
+    size_t global_variable_size = 0;


Suggested change

TEST_P(urProgramGetGlobalVariablePointerTest, Success) {

size_t global_variable_size = 0;

TEST_P(urProgramGetGlobalVariablePointerTest, Success) {

size_t global_variable_size = 0;

zhaomaosu · 2024-02-23T06:28:54Z

Hi, when can this PR be merged? We're implementing device sanitizer related features, and this new API can simplify our design.

kbenzie · 2024-02-23T12:34:01Z

@fabiomestre could you pull in the latest changes from main and resolve the conflicts?

fabiomestre · 2024-02-23T15:54:55Z

@fabiomestre could you pull in the latest changes from main and resolve the conflicts?

I have rebased and re-run the end to end tests. There is some failures there but it seems that's because the upstream branch is broken at the moment.

kbenzie · 2024-02-26T10:48:39Z

To actually use this in the SYCL RT there should also be a piextProgramGetGlobalVariablePointer which calls urProgramGetGlobalVariablePointer otherwise this will be unusable there.

fabiomestre · 2024-02-26T12:42:24Z

To actually use this in the SYCL RT there should also be a piextProgramGetGlobalVariablePointer which calls urProgramGetGlobalVariablePointer otherwise this will be unusable there.

Yes, I forgot about the SYCL RT side. I will update the intel-llvm PR with the new pi entrypoint

fabiomestre · 2024-02-26T16:33:57Z

To actually use this in the SYCL RT there should also be a piextProgramGetGlobalVariablePointer which calls urProgramGetGlobalVariablePointer otherwise this will be unusable there.

Yes, I forgot about the SYCL RT side. I will update the intel-llvm PR with the new pi entrypoint

It's updated now in intel/llvm#12496

kbenzie · 2024-02-27T15:21:34Z

Hi, when can this PR be merged? We're implementing device sanitizer related features, and this new API can simplify our design.

@zhaomaosu its waiting for approvals on intel/llvm#12496 and a couple of other UR PR's to merge first.

kbenzie · 2024-03-11T16:30:10Z

Please pull in the main branch to have up to date testing, also update the tag in the intel/llvm PR.

PietroGhg

Changes for native cpu lgtm, thank you

fabiomestre force-pushed the fabio/add_global_variable_pointer branch from bb3aea9 to 716f0bc Compare January 16, 2024 14:42

fabiomestre force-pushed the fabio/add_global_variable_pointer branch 7 times, most recently from f5e3493 to 0fc432c Compare January 16, 2024 21:41

fabiomestre marked this pull request as ready for review January 16, 2024 21:42

fabiomestre requested review from a team as code owners January 16, 2024 21:42

fabiomestre requested a review from hdelan January 16, 2024 21:42

hdelan reviewed Jan 17, 2024

View reviewed changes

source/adapters/cuda/program.cpp Outdated Show resolved Hide resolved

hdelan reviewed Jan 17, 2024

View reviewed changes

source/adapters/hip/program.cpp Outdated Show resolved Hide resolved

hdelan approved these changes Jan 22, 2024

View reviewed changes

fabiomestre force-pushed the fabio/add_global_variable_pointer branch from 15bc0e9 to 7d5ad11 Compare January 22, 2024 13:33

nrspruit approved these changes Jan 22, 2024

View reviewed changes

aarongreig approved these changes Jan 23, 2024

View reviewed changes

fabiomestre added ready to merge Added to PR's which are ready to merge and removed ready to merge Added to PR's which are ready to merge labels Jan 25, 2024

fabiomestre force-pushed the fabio/add_global_variable_pointer branch from be161f2 to f7bf99b Compare January 25, 2024 17:08

nrspruit mentioned this pull request Jan 26, 2024

[L0] Fix native kernel usage, multi device kernel pointer and WorkSize #1289

Merged

fabiomestre added the ready to merge Added to PR's which are ready to merge label Jan 29, 2024

fabiomestre force-pushed the fabio/add_global_variable_pointer branch from 2f52e17 to 27ce154 Compare February 23, 2024 13:50

fabiomestre mentioned this pull request Feb 27, 2024

[UR] Add urProgramGetGlobalVariablePointer entrypoint intel/llvm#12496

Merged

zhaomaosu mentioned this pull request Feb 27, 2024

[DeviceSanitizer] Support detecting out-of-bounds error on DeviceGlobals intel/llvm#12753

Merged

kbenzie added the v0.9.x Include in the v0.9.x release label Mar 11, 2024

fabiomestre requested a review from a team as a code owner March 11, 2024 17:42

PietroGhg approved these changes Mar 12, 2024

View reviewed changes

kbenzie self-assigned this Mar 14, 2024

kbenzie force-pushed the fabio/add_global_variable_pointer branch from 1bc658a to 6297b7f Compare March 14, 2024 22:02

fabiomestre added 2 commits March 18, 2024 20:05

[SPEC] Add urProgramGetGlobalVariablePointer entrypoint

05fbdf7

Add native cpu entrypoints

ca3da5a

kbenzie force-pushed the fabio/add_global_variable_pointer branch from 6297b7f to ca3da5a Compare March 18, 2024 20:05

kbenzie merged commit 4d0183a into oneapi-src:main Mar 18, 2024
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPEC] Add urProgramGetGlobalVariablePointer entrypoint #1255

[SPEC] Add urProgramGetGlobalVariablePointer entrypoint #1255

fabiomestre commented Jan 16, 2024 •

edited

Loading

codecov-commenter commented Jan 16, 2024 •

edited

Loading

hdelan left a comment

fabiomestre commented Jan 22, 2024

nrspruit left a comment

aarongreig Jan 23, 2024

aarongreig Jan 23, 2024

zhaomaosu commented Feb 23, 2024

kbenzie commented Feb 23, 2024

fabiomestre commented Feb 23, 2024

kbenzie commented Feb 26, 2024

fabiomestre commented Feb 26, 2024

fabiomestre commented Feb 26, 2024

kbenzie commented Feb 27, 2024

kbenzie commented Mar 11, 2024

PietroGhg left a comment

		TEST_P(urProgramGetGlobalVariablePointerTest, Success) {

		size_t global_variable_size = 0;

[SPEC] Add urProgramGetGlobalVariablePointer entrypoint #1255

[SPEC] Add urProgramGetGlobalVariablePointer entrypoint #1255

Conversation

fabiomestre commented Jan 16, 2024 • edited Loading

codecov-commenter commented Jan 16, 2024 • edited Loading

Codecov Report

hdelan left a comment

Choose a reason for hiding this comment

fabiomestre commented Jan 22, 2024

nrspruit left a comment

Choose a reason for hiding this comment

aarongreig Jan 23, 2024

Choose a reason for hiding this comment

aarongreig Jan 23, 2024

Choose a reason for hiding this comment

zhaomaosu commented Feb 23, 2024

kbenzie commented Feb 23, 2024

fabiomestre commented Feb 23, 2024

kbenzie commented Feb 26, 2024

fabiomestre commented Feb 26, 2024

fabiomestre commented Feb 26, 2024

kbenzie commented Feb 27, 2024

kbenzie commented Mar 11, 2024

PietroGhg left a comment

Choose a reason for hiding this comment

fabiomestre commented Jan 16, 2024 •

edited

Loading

codecov-commenter commented Jan 16, 2024 •

edited

Loading