Skip to content

[UR][L0] Implement support for device to device copy #16977

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

Closed
wants to merge 14 commits into from
Closed

[UR][L0] Implement support for device to device copy #16977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d3fea2b
[UR][L0] Implement support for device to device copy
winstonzhang-intel Feb 11, 2025
833460d
[UR][L0] Implement support for device to device copy
winstonzhang-intel Feb 20, 2025
a52376e
[UR][L0] Enable copy test in sycl/test-e2e/bindless_images/copies
winstonzhang-intel Mar 7, 2025
b25cf93
[UR][L0] added UNSUPPORTED-INTENDED for e2e tests
winstonzhang-intel Mar 20, 2025
1bcfc50
[UR][L0] Simplified to two cases.
winstonzhang-intel Mar 20, 2025
27efe31
Rebased against llvm/sycl
winstonzhang-intel Apr 1, 2025
2994199
Rebased against llvm/sycl
winstonzhang-intel Apr 1, 2025
0da11e5
Disabled irrelevant tests
winstonzhang-intel Apr 1, 2025
133f31b
Format error correction
winstonzhang-intel Apr 1, 2025
c3c5fa4
Testing different pitched images cases
winstonzhang-intel Apr 7, 2025
b339a50
testing with before changes
winstonzhang-intel Apr 8, 2025
cff04e6
With changes and pitched/unpitched usm to unpitched/pitched usm copy
winstonzhang-intel Apr 8, 2025
a76ea40
Temporarily disable tests
winstonzhang-intel Apr 8, 2025
8c1555f
Reenable all tests to start debug
winstonzhang-intel Apr 16, 2025
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
4 changes: 3 additions & 1 deletion sycl/test-e2e/bindless_images/copies/device_to_device_copy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// REQUIRES: aspect-ext_oneapi_bindless_images
// REQUIRES: cuda

// UNSUPPORTED: target-amd
// UNSUPPORTED-INTENDED: currently not supporting amd for bindless image d2d
// copy
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

Expand Down
4 changes: 3 additions & 1 deletion sycl/test-e2e/bindless_images/copies/device_to_device_copy_1D_subregion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// REQUIRES: aspect-ext_oneapi_bindless_images
// REQUIRES: cuda
// UNSUPPORTED: target-amd
// UNSUPPORTED-INTENDED: currently not supporting amd for bindless image d2d
// copy

// RUN: %{build} -o %t.out
// RUN: %{run} %t.out
Expand Down
4 changes: 3 additions & 1 deletion sycl/test-e2e/bindless_images/copies/device_to_device_copy_2D_subregion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// REQUIRES: aspect-ext_oneapi_bindless_images
// REQUIRES: cuda

// UNSUPPORTED: target-amd
// UNSUPPORTED-INTENDED: currently not supporting amd for bindless image d2d
// copy
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

Expand Down
3 changes: 1 addition & 2 deletions sycl/test-e2e/bindless_images/copies/device_to_device_pitched.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
// REQUIRES: aspect-ext_oneapi_bindless_images
// REQUIRES: aspect-ext_oneapi_bindless_images_2d_usm
// REQUIRES: cuda
//
// UNSUPPORTED: cuda
// UNSUPPORTED: target-amd
// UNSUPPORTED-TRACKER: https://github.com/intel/llvm/issues/17231

// RUN: %{build} -o %t.out
Expand Down
4 changes: 3 additions & 1 deletion sycl/test-e2e/bindless_images/copies/host_to_host_pitched.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
// REQUIRES: aspect-ext_oneapi_bindless_images
// REQUIRES: aspect-ext_oneapi_bindless_images_2d_usm
// REQUIRES: cuda

// UNSUPPORTED: target-amd
// UNSUPPORTED-INTENDED: currently not supporting amd for bindless image d2d
// copy
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out

Expand Down
64 changes: 52 additions & 12 deletions unified-runtime/source/adapters/level_zero/image.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,18 +538,58 @@ ur_result_t urBindlessImagesImageCopyExp(
WaitList.Length, WaitList.ZeEventList));
}
} else if (imageCopyFlags == UR_EXP_IMAGE_COPY_FLAG_DEVICE_TO_DEVICE) {
ze_image_region_t DstRegion;
UR_CALL(getImageRegionHelper(ZeImageDesc, &pCopyRegion->dstOffset,
&pCopyRegion->copyExtent, DstRegion));
ze_image_region_t SrcRegion;
UR_CALL(getImageRegionHelper(ZeImageDesc, &pCopyRegion->srcOffset,
&pCopyRegion->copyExtent, SrcRegion));
auto *UrImageDst = static_cast<_ur_image *>(pDst);
auto *UrImageSrc = static_cast<const _ur_image *>(pSrc);
ZE2UR_CALL(zeCommandListAppendImageCopyRegion,
(ZeCommandList, UrImageDst->ZeImage, UrImageSrc->ZeImage,
&DstRegion, &SrcRegion, ZeEvent, WaitList.Length,
WaitList.ZeEventList));
if (pSrcImageDesc->rowPitch != 0 && pDstImageDesc->rowPitch != 0) {
Copy link
Contributor

@JackAKirk JackAKirk Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

if either src->rowPitch or dst->rowPitch = 0, but the other is non-zero, this would imply that the one with value zero is pitched usm, and the one with value non-zero is using the zeImage opaque type. This circumstance does seem to be possible to arrive at if the user mistakenly manually sets a pitch 0 etc.

In such a case the logic will currently assume both are zeImage types and take the other branch that instead calls zeCommandListAppendImageCopyRegion.

Now I'm not sure that this can be improved upon. But I thought it is worth asking the question of whether the error messaging in this implementation is optimal wrt the above case (and similar). It is a bit cleaner in the cuda backend since there is not the distinction of the two separate apis, since for the cuda backend an opaque type like zeImage doesn't exist for such copy apis: the bare pointers are always passed along with an appropriate descriptor.

I think generally it is important to have tests cases for all types and possible valid inputs (as mentioned here #16977 (comment)) to clarify such issues as the above and make this PR generally easier to review, as well as guarantee that any future breaks of this functionality are flagged from failing tests.

Copy link
Contributor

@JackAKirk JackAKirk Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Note also that the l0 failures for https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/bindless_images/array/read_write_1d_subregion.cpp and read_write_2d_subregion.cpp can also be pinpointed to failures in these copy functions. These cases correspond to a situation which is dealt with differently in the cuda adapter via the query https://github.com/intel/llvm/blob/sycl/unified-runtime/source/adapters/cuda/image.cpp#L628
However it is not clear to me from examining the l0 spec whether/how this case can be dealt with differently in the l0 backend. It is a device to device copy case, so it would be good to at least clarify at this point whether this overlaps with any of the changes made in this PR, and if when taking into account these additional failures, the changes in this PR might have to be reconsidered.

read_write_1d_subregion.cpp fails due to

"SYCL exception caught! : Enqueue process failed."

This can be pinpointed to triggered asserts that look out of date in the function getImageRegionHelper. This is I think a minor issue.
However once this is fixed read_write_1d_subregion.cpp fails in the same way as read_write_2d_subregion.cpp:

free(): invalid pointer
Aborted

Further investigation reveals that this invalid pointer issue does not occur when commenting out the calls to ext_oneapi_copy using these array types.

I think that the same issue leads to the failure in array/read_sampled_array.cpp

// Copy from pitched USM memory to pitched USM memory
uint32_t SrcRowPitch = pSrcImageDesc->rowPitch;
uint32_t DstRowPitch = pDstImageDesc->rowPitch;
ze_copy_region_t ZeDstRegion = {(uint32_t)pCopyRegion->dstOffset.x,
(uint32_t)pCopyRegion->dstOffset.y,
(uint32_t)pCopyRegion->dstOffset.z,
DstRowPitch,
(uint32_t)pCopyRegion->copyExtent.height,
(uint32_t)pCopyRegion->copyExtent.depth};
ze_copy_region_t ZeSrcRegion = {(uint32_t)pCopyRegion->srcOffset.x,
(uint32_t)pCopyRegion->srcOffset.y,
(uint32_t)pCopyRegion->srcOffset.z,
SrcRowPitch,
(uint32_t)pCopyRegion->copyExtent.height,
(uint32_t)pCopyRegion->copyExtent.depth};
uint32_t DstSlicePitch = 0;
uint32_t SrcSlicePitch = 0;
ZE2UR_CALL(zeCommandListAppendMemoryCopyRegion,
(ZeCommandList, pDst, &ZeDstRegion, DstRowPitch, DstSlicePitch,
pSrc, &ZeSrcRegion, SrcRowPitch, SrcSlicePitch, ZeEvent,
WaitList.Length, WaitList.ZeEventList));
} else if (pSrcImageDesc->rowPitch == 0 && pDstImageDesc->rowPitch != 0) {
// Handle the case where source row pitch is zero and destination row
// pitch is non-zero
logger::error(
"urBindlessImagesImageCopyExp: Source row pitch is zero, but "
"destination row pitch is non-zero. Potential "
"misconfiguration detected.");
return UR_RESULT_ERROR_INVALID_ARGUMENT;
} else if (pSrcImageDesc->rowPitch != 0 && pDstImageDesc->rowPitch == 0) {
// Handle the case where destination row pitch is zero and source row
// pitch is non-zero
logger::error(
"urBindlessImagesImageCopyExp: Destination row pitch is zero, but "
"source row pitch is non-zero. Potential misconfiguration detected.");
return UR_RESULT_ERROR_INVALID_ARGUMENT;
} else {
// Copy from Non-USM memory to Non-USM memory
ze_image_region_t DstRegion;
UR_CALL(getImageRegionHelper(ZeImageDesc, &pCopyRegion->dstOffset,
&pCopyRegion->copyExtent, DstRegion));
ze_image_region_t SrcRegion;
UR_CALL(getImageRegionHelper(ZeImageDesc, &pCopyRegion->srcOffset,
&pCopyRegion->copyExtent, SrcRegion));
auto *UrImageDst = static_cast<_ur_image *>(pDst);
auto *UrImageSrc = static_cast<const _ur_image *>(pSrc);
ZE2UR_CALL(zeCommandListAppendImageCopyRegion,
(ZeCommandList, UrImageDst->ZeImage, UrImageSrc->ZeImage,
&DstRegion, &SrcRegion, ZeEvent, WaitList.Length,
WaitList.ZeEventList));
}
} else {
logger::error("urBindlessImagesImageCopyExp: unexpected imageCopyFlags");
return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
Expand Down
Loading