Skip to content

[SYCL][CUDA] Fetch the adapter source from the UR repo #11342

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

Merged
againull merged 4 commits into from
Oct 5, 2023
Merged

[SYCL][CUDA] Fetch the adapter source from the UR repo #11342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
18b21f7
[SYCL][CUDA] Fetch the adapter source from UR repo
Sep 28, 2023
345a913
Update UR commit
Oct 2, 2023
f2c1433
Merge branch 'sycl' into move_cuda_adapter
Oct 4, 2023
fe35477
Merge remote-tracking branch 'origin/sycl' into move_cuda_adapter
Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 11 additions & 59 deletions sycl/plugins/cuda/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,11 @@
message(STATUS "Including the PI API CUDA backend.")

# cannot rely on cmake support for CUDA; it assumes runtime API is being used.
# we only require the CUDA driver API to be used
# CUDA_CUDA_LIBRARY variable defines the path to libcuda.so, the CUDA Driver API library.

find_package(CUDA 10.1 REQUIRED)

# Make imported library global to use it within the project.
add_library(cudadrv SHARED IMPORTED GLOBAL)

if (WIN32)
Copy link
Contributor

@JackAKirk JackAKirk Sep 28, 2023

Choose a reason for hiding this comment

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

I'm not sure of the significance of this part, but should/have you checked whether the removal of this code affects windows builds?

Copy link
Contributor Author

@fabiomestre fabiomestre Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we officially support windows builds in CUDA? CI doesn't seem to run it.

This code was moved to unified-runtime repo: https://github.com/oneapi-src/unified-runtime/blob/adapters/source/adapters/cuda/CMakeLists.txt

It was causing issues because cudadrv was being imported twice. I think the behaviour should stay the same after the move.

For what it is worth, the windows job passed with this changes: https://github.com/intel/llvm/actions/runs/6340282334

Copy link
Contributor

@JackAKirk JackAKirk Sep 28, 2023

Choose a reason for hiding this comment

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

Windows support is experimental for cuda backend but I don't think it is tested by any CI. You should be able to build and run most tests on it though. Would be good to just check if still builds if the CMake changes could affect it. Codeplay has a windows machine available for this. If you are sure it won't affect windows build then of course this won't be worthwhile.

Copy link
Contributor

@JackAKirk JackAKirk Sep 29, 2023

Choose a reason for hiding this comment

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

I think since the code is moved to UR, and linux CI cuda testing works, it is expected to still work on windows too.

set_target_properties(
cudadrv PROPERTIES
IMPORTED_IMPLIB ${CUDA_CUDA_LIBRARY}
INTERFACE_INCLUDE_DIRECTORIES ${CUDA_INCLUDE_DIRS}
)
else()
set_target_properties(
cudadrv PROPERTIES
IMPORTED_LOCATION ${CUDA_CUDA_LIBRARY}
INTERFACE_INCLUDE_DIRECTORIES ${CUDA_INCLUDE_DIRS}
)
endif()

if (SYCL_ENABLE_XPTI_TRACING)
# cannot rely on cmake support for CUDA; it assumes runtime API is being used.
# we only require the CUDA driver API to be used
# CUDA_CUDA_LIBRARY variable defines the path to libcuda.so, the CUDA Driver API library.
find_package(CUDA 10.1 REQUIRED)
Copy link
Contributor

@JackAKirk JackAKirk Sep 28, 2023

Choose a reason for hiding this comment

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

Why does cmake need 10.1 available when dpc++ is not usually built with 10.1? Can this be removed?:

find_package(CUDA 10.1 REQUIRED)

Copy link
Contributor Author

@fabiomestre fabiomestre Sep 28, 2023

Choose a reason for hiding this comment

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

I think this means that a version of 10.1 or higher is required to build. The Cmake in UR repo is asking for the same version.

Maybe there is an argument that 10.1 is no longer enough? In that case we should decide which version is required and update this cmake and the one in UR (but that is probably out of scope for this PR).

I don't think this can be removed because include(FindCUDACupti) relies on variables set by find_package(CUDA 10.1 REQUIRED) such as CUDA_TOOLKIT_ROOT_DIR.

Copy link
Contributor

@JackAKirk JackAKirk Sep 28, 2023

Choose a reason for hiding this comment

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

I see, yeah it probably makes sense to leave it as it is then. People might still want to use 10.1

Copy link
Contributor

@JackAKirk JackAKirk Sep 28, 2023

Choose a reason for hiding this comment

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

Was the cuda check moved inside if (SYCL_ENABLE_XPTI_TRACING) intentionally? Shouldn't it check there is a cuda version available even when not using XPTI?

Copy link
Contributor Author

@fabiomestre fabiomestre Sep 29, 2023

Choose a reason for hiding this comment

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

If the code reached this point, find_package(CUDA) has already been invoked from unified-runtime and the library cudadrv already exists. The only reason to invoke it here is to get the env variables. I moved it inside the if intentionally because it's the only place where it is needed.

This implicit dependency from unified-runtime CmakeLists is not very intuitive. But PI will be removed soon, so I think there is no need to refactor this too much.


# The following two if's can be removed when FindCUDA -> FindCUDAToolkit.
# CUDA_CUPTI_INCLUDE_DIR -> CUDAToolkit_CUPTI_INCLUDE_DIR
include(FindCUDACupti)
Expand All @@ -46,46 +28,15 @@ if (SYCL_ENABLE_XPTI_TRACING)
)
endif()

# Get the CUDA adapter sources so they can be shared with the CUDA PI plugin
get_target_property(UR_CUDA_ADAPTER_SOURCES ur_adapter_cuda SOURCES)

add_sycl_plugin(cuda
SOURCES
${UR_CUDA_ADAPTER_SOURCES}
# Some code is shared with the UR adapter
"../unified_runtime/pi2ur.hpp"
"../unified_runtime/pi2ur.cpp"
"../unified_runtime/ur/ur.hpp"
"../unified_runtime/ur/ur.cpp"
"../unified_runtime/ur/adapters/cuda/adapter.cpp"
"../unified_runtime/ur/adapters/cuda/adapter.hpp"
"../unified_runtime/ur/adapters/cuda/command_buffer.cpp"
"../unified_runtime/ur/adapters/cuda/command_buffer.hpp"
"../unified_runtime/ur/adapters/cuda/common.cpp"
"../unified_runtime/ur/adapters/cuda/common.hpp"
"../unified_runtime/ur/adapters/cuda/context.cpp"
"../unified_runtime/ur/adapters/cuda/context.hpp"
"../unified_runtime/ur/adapters/cuda/device.cpp"
"../unified_runtime/ur/adapters/cuda/device.hpp"
"../unified_runtime/ur/adapters/cuda/enqueue.cpp"
"../unified_runtime/ur/adapters/cuda/event.cpp"
"../unified_runtime/ur/adapters/cuda/event.hpp"
"../unified_runtime/ur/adapters/cuda/image.cpp"
"../unified_runtime/ur/adapters/cuda/image.hpp"
"../unified_runtime/ur/adapters/cuda/kernel.cpp"
"../unified_runtime/ur/adapters/cuda/kernel.hpp"
"../unified_runtime/ur/adapters/cuda/memory.cpp"
"../unified_runtime/ur/adapters/cuda/memory.hpp"
"../unified_runtime/ur/adapters/cuda/platform.cpp"
"../unified_runtime/ur/adapters/cuda/platform.hpp"
"../unified_runtime/ur/adapters/cuda/program.cpp"
"../unified_runtime/ur/adapters/cuda/program.hpp"
"../unified_runtime/ur/adapters/cuda/queue.cpp"
"../unified_runtime/ur/adapters/cuda/queue.hpp"
"../unified_runtime/ur/adapters/cuda/sampler.cpp"
"../unified_runtime/ur/adapters/cuda/sampler.hpp"
"../unified_runtime/ur/adapters/cuda/tracing.cpp"
"../unified_runtime/ur/adapters/cuda/ur_interface_loader.cpp"
"../unified_runtime/ur/adapters/cuda/usm.cpp"
"../unified_runtime/ur/adapters/cuda/usm.hpp"
"../unified_runtime/ur/adapters/cuda/usm_p2p.cpp"
# ---
"${sycl_inc_dir}/sycl/detail/pi.h"
"${sycl_inc_dir}/sycl/detail/pi.hpp"
"pi_cuda.hpp"
Expand All @@ -94,7 +45,8 @@ add_sycl_plugin(cuda
INCLUDE_DIRS
${sycl_inc_dir}
${XPTI_INCLUDE}
${CMAKE_CURRENT_SOURCE_DIR}/../unified_runtime
${CMAKE_CURRENT_SOURCE_DIR}/../unified_runtime # for Unified Runtime
${UNIFIED_RUNTIME_SOURCE_DIR}/source/ # for adapters/cuda
LIBRARIES
cudadrv
${XPTI_LIBS}
Expand Down
20 changes: 10 additions & 10 deletions sycl/plugins/cuda/pi_cuda.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,16 @@
#define _PI_CUDA_PLUGIN_VERSION_STRING \
_PI_PLUGIN_VERSION_STRING(_PI_CUDA_PLUGIN_VERSION)

#include <ur/adapters/cuda/command_buffer.hpp>
#include <ur/adapters/cuda/context.hpp>
#include <ur/adapters/cuda/device.hpp>
#include <ur/adapters/cuda/event.hpp>
#include <ur/adapters/cuda/kernel.hpp>
#include <ur/adapters/cuda/memory.hpp>
#include <ur/adapters/cuda/platform.hpp>
#include <ur/adapters/cuda/program.hpp>
#include <ur/adapters/cuda/queue.hpp>
#include <ur/adapters/cuda/sampler.hpp>
#include <adapters/cuda/command_buffer.hpp>
#include <adapters/cuda/context.hpp>
#include <adapters/cuda/device.hpp>
#include <adapters/cuda/event.hpp>
#include <adapters/cuda/kernel.hpp>
#include <adapters/cuda/memory.hpp>
#include <adapters/cuda/platform.hpp>
#include <adapters/cuda/program.hpp>
#include <adapters/cuda/queue.hpp>
#include <adapters/cuda/sampler.hpp>

// Share code between the PI Plugin and UR Adapter
#include <pi2ur.hpp>
Expand Down
64 changes: 6 additions & 58 deletions sycl/plugins/unified_runtime/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@
if (NOT DEFINED UNIFIED_RUNTIME_LIBRARY OR NOT DEFINED UNIFIED_RUNTIME_INCLUDE_DIR)
include(FetchContent)

# The UR tag should be from the 'adapters' branch
set(UNIFIED_RUNTIME_REPO "https://github.com/oneapi-src/unified-runtime.git")
set(UNIFIED_RUNTIME_TAG 9265d33b3b1e538bb1d27aea29d30f2ed47859be)

set(UR_BUILD_ADAPTER_L0 ON)

if ("cuda" IN_LIST SYCL_ENABLE_PLUGINS)
set(UR_BUILD_ADAPTER_CUDA ON)
endif()

set(UMF_ENABLE_POOL_TRACKING ON)
message(STATUS "Will fetch Unified Runtime from ${UNIFIED_RUNTIME_REPO}")
FetchContent_Declare(unified-runtime
Expand Down Expand Up @@ -81,63 +85,7 @@ add_sycl_plugin(unified_runtime
add_dependencies(sycl-runtime-libraries ur_adapter_level_zero)

if ("cuda" IN_LIST SYCL_ENABLE_PLUGINS)
# Build CUDA adapter
add_sycl_library("ur_adapter_cuda" SHARED
SOURCES
"ur/ur.hpp"
"ur/ur.cpp"
"ur/adapters/cuda/adapter.cpp"
"ur/adapters/cuda/adapter.hpp"
"ur/adapters/cuda/command_buffer.cpp"
"ur/adapters/cuda/command_buffer.hpp"
"ur/adapters/cuda/common.cpp"
"ur/adapters/cuda/common.hpp"
"ur/adapters/cuda/context.cpp"
"ur/adapters/cuda/context.hpp"
"ur/adapters/cuda/device.cpp"
"ur/adapters/cuda/device.hpp"
"ur/adapters/cuda/enqueue.cpp"
"ur/adapters/cuda/event.cpp"
"ur/adapters/cuda/event.hpp"
"ur/adapters/cuda/image.cpp"
"ur/adapters/cuda/image.hpp"
"ur/adapters/cuda/kernel.cpp"
"ur/adapters/cuda/kernel.hpp"
"ur/adapters/cuda/memory.cpp"
"ur/adapters/cuda/memory.hpp"
"ur/adapters/cuda/platform.cpp"
"ur/adapters/cuda/platform.hpp"
"ur/adapters/cuda/program.cpp"
"ur/adapters/cuda/program.hpp"
"ur/adapters/cuda/queue.cpp"
"ur/adapters/cuda/queue.hpp"
"ur/adapters/cuda/sampler.cpp"
"ur/adapters/cuda/sampler.hpp"
"ur/adapters/cuda/tracing.cpp"
"ur/adapters/cuda/ur_interface_loader.cpp"
"ur/adapters/cuda/usm.cpp"
"ur/adapters/cuda/usm.hpp"
"ur/adapters/cuda/usm_p2p.cpp"
INCLUDE_DIRS
${sycl_inc_dir}
LIBRARIES
UnifiedRuntime-Headers
UnifiedRuntimeCommon
Threads::Threads
cudadrv
)

set_target_properties("ur_adapter_cuda" PROPERTIES
VERSION "0.0.0"
SOVERSION "0"
)

if(UMF_ENABLE_POOL_TRACKING)
target_compile_definitions("ur_adapter_cuda" PRIVATE
UMF_ENABLE_POOL_TRACKING)
else()
message(WARNING "CUDA adapter USM pools are disabled, set UMF_ENABLE_POOL_TRACKING to enable them")
endif()
add_dependencies(sycl-runtime-libraries ur_adapter_cuda)
endif()

if ("hip" IN_LIST SYCL_ENABLE_PLUGINS)
Expand Down
7 changes: 7 additions & 0 deletions sycl/plugins/unified_runtime/ur/adapters/cuda/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Cuda adapter
The source for the Cuda adapter has been moved to the
[adapters](https://github.com/oneapi-src/unified-runtime/tree/adapters) branch
of the [Unified Runtime](https://github.com/oneapi-src/unified-runtime/) repo.
Changes can be made by opening pull requests against that branch, and updating
the Unified Runtime commit in the parent
[CMakeLists.txt](../../../CMakeLists.txt).
89 changes: 0 additions & 89 deletions sycl/plugins/unified_runtime/ur/adapters/cuda/adapter.cpp
View file
Open in desktop

This file was deleted.

11 changes: 0 additions & 11 deletions sycl/plugins/unified_runtime/ur/adapters/cuda/adapter.hpp
View file
Open in desktop

This file was deleted.

Loading