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.

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

[SYCL][Graph] Support for read and write for 1d and 2d buffers #238

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

mfrancepillois
Copy link
Collaborator

Adds support required to handle:
- host to buffer memcpy for one-dimensional and 2d buffers
- buffer to host memcpy for 1d and 2d buffers
This commit also fixes a bug in buffer to buffer memcpy enabling to copy from/to buffers accessed with user-defined offsets.

Adds basic tests to check all use-cases of mixed host/buffer memcpy, and buffer to buffer memcpy with user-defined offsets.

Addresses Issue: #196

@mfrancepillois mfrancepillois added the Graph Implementation Related to DPC++ implementation and testing label Jun 22, 2023
Copy link
Collaborator

@Bensuo Bensuo left a comment

Choose a reason for hiding this comment

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

Good work! Mostly minor comments/nitpicks.

sycl/include/sycl/detail/pi.h Outdated Show resolved Hide resolved
sycl/include/sycl/detail/pi.h Outdated Show resolved Hide resolved
sycl/include/sycl/detail/pi.h Outdated Show resolved Hide resolved
sycl/source/detail/scheduler/commands.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

Looks great overall, have some minor comments.

sycl/test-e2e/Graph/RecordReplay/buffer_copy_offsets.cpp Outdated Show resolved Hide resolved
sycl/test-e2e/Graph/RecordReplay/buffer_copy_offsets.cpp Outdated Show resolved Hide resolved
sycl/include/sycl/detail/pi.h Outdated Show resolved Hide resolved
sycl/include/sycl/detail/pi.h Outdated Show resolved Hide resolved
sycl/include/sycl/detail/pi.h Outdated Show resolved Hide resolved
sycl/source/detail/scheduler/commands.cpp Outdated Show resolved Hide resolved
sycl/include/sycl/detail/pi.def Show resolved Hide resolved
sycl/plugins/cuda/pi_cuda.cpp Outdated Show resolved Hide resolved
sycl/plugins/cuda/pi_cuda.cpp Outdated Show resolved Hide resolved
@mfrancepillois mfrancepillois force-pushed the maxime/mixed-host-buffer-memcpy branch from 7cae0f2 to 719f400 Compare June 26, 2023 16:05
sycl/unittests/helpers/PiMockPlugin.hpp Outdated Show resolved Hide resolved
sycl/unittests/helpers/PiMockPlugin.hpp Outdated Show resolved Hide resolved
sycl/plugins/unified_runtime/pi_unified_runtime.cpp Outdated Show resolved Hide resolved
sycl/plugins/unified_runtime/pi2ur.hpp Outdated Show resolved Hide resolved
sycl/plugins/opencl/pi_opencl.cpp Outdated Show resolved Hide resolved
sycl/plugins/hip/pi_hip.cpp Outdated Show resolved Hide resolved
sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp Outdated Show resolved Hide resolved
sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp Outdated Show resolved Hide resolved
sycl/plugins/cuda/pi_cuda.cpp Outdated Show resolved Hide resolved
sycl/plugins/cuda/pi_cuda.cpp Outdated Show resolved Hide resolved
EwanC added a commit that referenced this pull request Jun 27, 2023
Pulls in changes from #238
which requires bumping the UR commit to
oneapi-src/unified-runtime#644

Co-authored-by: Maxime France-Pillois <maxime.francepillois@codeplay.com>
@mfrancepillois mfrancepillois force-pushed the maxime/mixed-host-buffer-memcpy branch from 719f400 to 0c4f0b6 Compare June 27, 2023 11:01
Copy link
Collaborator

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

Final couple files where the parameter naming needs touched up, but looks good to go otherwise.

sycl/include/sycl/detail/pi.h Outdated Show resolved Hide resolved
…uffers

Adds support required to handle:
    - host to buffer memcpy for 1d and 2d buffers
    - buffer to host memcpy for 1d and 2d buffers
This commit also fixes a bug in buffer to buffer memcpy enabling to copy
from/to buffers accessed with user-defined offsets.

Adds basic tests to check all use-cases of mixed host/buffer memcpy, and
buffer to buffer memcpy with user-defined offsets.

Addresses Issue: #196
@mfrancepillois mfrancepillois force-pushed the maxime/mixed-host-buffer-memcpy branch from 0c4f0b6 to 4589950 Compare June 27, 2023 12:00
@mfrancepillois mfrancepillois merged commit 07ecae5 into sycl-graph-develop Jun 27, 2023
EwanC added a commit that referenced this pull request Jun 27, 2023
Fix some build errors I've seen with clang-17 after #238
was merged.

1) Arithmitic on void pointer, fixed by passing a `char*` rather than
`void*` when an offset is needed

```
/home/ewan/Development/dpcpp/sycl/source/detail/memory_manager.cpp:1316:44: error: arithmetic on a pointer to void
 1316 |           SrcAccessRangeWidthBytes, DstMem + DstXOffBytes, Deps.size(),
      |                                     ~~~~~~ ^
/home/ewan/Development/dpcpp/sycl/source/detail/memory_manager.cpp:1372:44: error: arithmetic on a pointer to void
 1372 |           DstAccessRangeWidthBytes, SrcMem + SrcXOffBytes, Deps.size(),
      |
	  ~~~~~~ ^

2) Fixup use of `numEventsInWaitList` when should be `numSyncPointsInWaitList`
/home/ewan/Development/dpcpp/sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_level_zero_command_buffer.cpp:611:13: error: use of undeclared identifier 'numEventsInWaitList'
  611 |       size, numEventsInWaitList, pSyncPointWaitList, pSyncPoint);
```
EwanC added a commit that referenced this pull request Jun 27, 2023
Fix some build errors I've seen with clang-17 after #238
was merged.

1) Arithmitic on void pointer, fixed by passing a `char*` rather than
`void*` when an offset is needed

```
/home/ewan/Development/dpcpp/sycl/source/detail/memory_manager.cpp:1316:44: error: arithmetic on a pointer to void
 1316 |           SrcAccessRangeWidthBytes, DstMem + DstXOffBytes, Deps.size(),
      |                                     ~~~~~~ ^
/home/ewan/Development/dpcpp/sycl/source/detail/memory_manager.cpp:1372:44: error: arithmetic on a pointer to void
 1372 |           DstAccessRangeWidthBytes, SrcMem + SrcXOffBytes, Deps.size(),
      |
	  ~~~~~~ ^

2) Fixup use of `numEventsInWaitList` when should be `numSyncPointsInWaitList`
/home/ewan/Development/dpcpp/sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_level_zero_command_buffer.cpp:611:13: error: use of undeclared identifier 'numEventsInWaitList'
  611 |       size, numEventsInWaitList, pSyncPointWaitList, pSyncPoint);
```
@mfrancepillois mfrancepillois deleted the maxime/mixed-host-buffer-memcpy branch June 28, 2023 13:28
EwanC pushed a commit that referenced this pull request Jun 29, 2023
…uffers (#238)

Adds support required to handle:
    - host to buffer memcpy for 1d and 2d buffers
    - buffer to host memcpy for 1d and 2d buffers
This commit also fixes a bug in buffer to buffer memcpy enabling to copy
from/to buffers accessed with user-defined offsets.

Adds basic tests to check all use-cases of mixed host/buffer memcpy, and
buffer to buffer memcpy with user-defined offsets.

Addresses Issue: #196
EwanC added a commit that referenced this pull request Jun 29, 2023
Fix some build errors I've seen with clang-17 after #238
was merged.

1) Arithmitic on void pointer, fixed by passing a `char*` rather than
`void*` when an offset is needed

```
/home/ewan/Development/dpcpp/sycl/source/detail/memory_manager.cpp:1316:44: error: arithmetic on a pointer to void
 1316 |           SrcAccessRangeWidthBytes, DstMem + DstXOffBytes, Deps.size(),
      |                                     ~~~~~~ ^
/home/ewan/Development/dpcpp/sycl/source/detail/memory_manager.cpp:1372:44: error: arithmetic on a pointer to void
 1372 |           DstAccessRangeWidthBytes, SrcMem + SrcXOffBytes, Deps.size(),
      |
	  ~~~~~~ ^

2) Fixup use of `numEventsInWaitList` when should be `numSyncPointsInWaitList`
/home/ewan/Development/dpcpp/sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_level_zero_command_buffer.cpp:611:13: error: use of undeclared identifier 'numEventsInWaitList'
  611 |       size, numEventsInWaitList, pSyncPointWaitList, pSyncPoint);
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Graph Implementation Related to DPC++ implementation and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants