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

[UR][CUDA] Add tensor map APIs #1811

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[UR][CUDA] Add tensor map APIs #1811

wants to merge 11 commits into from

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Jul 2, 2024

Intended to target the APIs here

@hdelan hdelan requested a review from a team as a code owner July 2, 2024 17:04
@hdelan hdelan marked this pull request as draft July 2, 2024 17:06
@github-actions github-actions bot added loader Loader related feature/bug specification Changes or additions to the specification experimental Experimental feature additions/changes/specification labels Jul 2, 2024
@hdelan hdelan force-pushed the tensormap-exp-api branch 3 times, most recently from cc67afb to 221e4db Compare July 3, 2024 09:17
@github-actions github-actions bot added the cuda CUDA adapter specific issues label Jul 3, 2024
@hdelan hdelan force-pushed the tensormap-exp-api branch 2 times, most recently from 8f63fb0 to ae22a1d Compare July 3, 2024 11:54
@hdelan hdelan marked this pull request as ready for review July 3, 2024 11:55
@hdelan hdelan requested a review from a team as a code owner July 3, 2024 11:55
@hdelan hdelan requested a review from frasercrmck July 3, 2024 11:55
@hdelan hdelan changed the title [draft] Add tensor map APIs [UR][SYCL] Add tensor map APIs Jul 3, 2024
@hdelan hdelan changed the title [UR][SYCL] Add tensor map APIs [UR][CUDA] Add tensor map APIs Jul 3, 2024
frasercrmck
frasercrmck reviewed Jul 4, 2024
View reviewed changes
include/ur_api.h Outdated Show resolved Hide resolved
include/ur_api.h Outdated Show resolved Hide resolved
include/ur_api.h Show resolved Hide resolved
include/ur_api.h Outdated Show resolved Hide resolved
include/ur_api.h Outdated Show resolved Hide resolved
source/adapters/cuda/tensor_map.cpp Outdated Show resolved Hide resolved
source/adapters/cuda/tensor_map.cpp Outdated Show resolved Hide resolved
source/adapters/cuda/tensor_map.cpp Show resolved Hide resolved
source/adapters/cuda/tensor_map.cpp Outdated Show resolved Hide resolved
source/loader/layers/validation/ur_valddi.cpp Outdated Show resolved Hide resolved
@hdelan
Copy link
Contributor Author

hdelan commented Jul 4, 2024

@frasercrmck have responded to all of your comments I think.

frasercrmck
frasercrmck reviewed Jul 4, 2024
View reviewed changes
scripts/core/exp-tensor-map.yml Outdated Show resolved Hide resolved
scripts/core/exp-tensor-map.yml Outdated Show resolved Hide resolved
scripts/core/exp-tensor-map.yml Outdated Show resolved Hide resolved
scripts/core/exp-tensor-map.yml Outdated Show resolved Hide resolved
@hdelan hdelan force-pushed the tensormap-exp-api branch 2 times, most recently from 573ba28 to ba8a391 Compare July 4, 2024 16:01
frasercrmck
frasercrmck reviewed Jul 4, 2024
View reviewed changes
scripts/core/registry.yml Outdated
@@ -592,6 +592,12 @@ etors:
- name: ENQUEUE_NATIVE_COMMAND_EXP
desc: Enumerator for $xEnqueueNativeCommandExp
value: '228'
- name: TENSOR_MAP_ENCODE_IM_2_COL_EXP
desc: Enumerator for $xTensorMapEncodeIm2ColExp
value: '231'
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is where everything's generated from, is this why we're seeing enumerators skip a few values? Should these be 229 and 230?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This registry.yml is itself autogenerated. I don't know how to modify this number. Maybe @kbenzie has an idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

The script will only assign a number automatically if the function is absent from registry.yml. You can always add it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha interesting. Thanks @pbalcer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated @frasercrmck

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is to delete the entry from the registry and run the generate target again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, thanks

@hdelan hdelan requested a review from frasercrmck July 15, 2024 11:53
@hdelan hdelan requested review from a team as code owners July 15, 2024 14:11
@github-actions github-actions bot added the level-zero L0 adapter specific issues label Jul 15, 2024
@github-actions github-actions bot added hip HIP adapter specific issues native-cpu Native CPU adapter specific issues labels Jul 15, 2024
@npmiller
Copy link
Contributor

Looks like this is causing some build issues in the L0 static build

Amongst other undefined symbols:

[ 56%] Building CXX object test/conformance/context/CMakeFiles/test-context.dir/__/source/environment.cpp.o
/usr/bin/ld: ../../../lib/libur_adapter_level_zero.a(ur_interface_loader.cpp.o): in function `ur::level_zero::urGetTensorMapExpProcAddrTable(ur_api_version_t, ur_tensor_map_exp_dditable_t*)':
/home/test-user/actions-runner/_work/unified-runtime/unified-runtime/source/adapters/level_zero/ur_interface_loader.cpp:431: undefined reference to `ur::level_zero::urTensorMapEncodeIm2ColExp(ur_device_handle_t_*, unsigned int, unsigned int, void*, unsigned long const*, unsigned long const*, int const*, int const*, unsigned int, unsigned int, unsigned int const*, unsigned int, unsigned int, unsigned int, unsigned int, ur_exp_tensor_map_handle_t_**)'
/usr/bin/ld: /home/test-user/actions-runner/_work/unified-runtime/unified-runtime/source/adapters/level_zero/ur_interface_loader.cpp:432: undefined reference to `ur::level_zero::urTensorMapEncodeTiledExp(ur_device_handle_t_*, unsigned int, unsigned int, void*, unsigned long const*, unsigned long const*, unsigned int const*, unsigned int const*, unsigned int, unsigned int, unsigned int, unsigned int, ur_exp_tensor_map_handle_t_**)'
/usr/bin/ld: ../../../lib/libur_adapter_level_zero.a(ur_interface_loader.cpp.o): in function `ur::level_zero::urGetTensorMapExpProcAddrTable(ur_api_version_t, ur_tensor_map_exp_dditable_t*)':
/home/test-user/actions-runner/_work/unified-runtime/unified-runtime/source/adapters/level_zero/ur_interface_loader.cpp:431: undefined reference to `ur::level_zero::urTensorMapEncodeIm2ColExp(ur_device_handle_t_*, unsigned int, unsigned int, void*, unsigned long const*, unsigned long const*, int const*, int const*, unsigned int, unsigned int, unsigned int const*, unsigned int, unsigned int, unsigned int, unsigned int, ur_exp_tensor_map_handle_t_**)'
/usr/bin/ld: /home/test-user/actions-runner/_work/unified-runtime/unified-runtime/source/adapters/level_zero/ur_interface_loader.cpp:432: undefined reference to `ur::level_zero::urTensorMapEncodeTiledExp(ur_device_handle_t_*, unsigned int, unsigned int, void*, unsigned long const*, unsigned long const*, unsigned int const*, unsigned int const*, unsigned int, unsigned int, unsigned int, unsigned int, ur_exp_tensor_map_handle_t_**)'
collect2: error: ld returned 1 exit status

@hdelan
Merge branch 'main' into tensormap-exp-api
c767268
@hdelan
Put UR entry points in ur::level_zero
9445457 
Fixes missing symbol at linking for static build of L0 adapter.
@hdelan
Copy link
Contributor Author

hdelan commented Oct 25, 2024

Looks like this is causing some build issues in the L0 static build

Should be fixed. Will keep an eye on CI.

@npmiller
Copy link
Contributor

npmiller commented Nov 5, 2024

ping @oneapi-src/unified-runtime-native-cpu-write @oneapi-src/unified-runtime-opencl-write @oneapi-src/unified-runtime-hip-write @oneapi-src/unified-runtime-level-zero-write

This should be ready to review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frasercrmck frasercrmck Awaiting requested review from frasercrmck

@kbenzie kbenzie Awaiting requested review from kbenzie

@pbalcer pbalcer Awaiting requested review from pbalcer

Assignees
No one assigned
Labels
cuda CUDA adapter specific issues experimental Experimental feature additions/changes/specification hip HIP adapter specific issues level-zero L0 adapter specific issues loader Loader related feature/bug native-cpu Native CPU adapter specific issues specification Changes or additions to the specification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hdelan @npmiller @frasercrmck @kbenzie @pbalcer