[SYCL] Implement device_global host-side memory operations #8022

steffenlarsen · 2023-01-17T15:29:19Z

This commit implements the copy and memcpy operations to and from device_global. If the device_global does not have device_image_scope the memory operation will be on the underlying USM memory, while if the operation is on a device_global with device_image_scope the runtime will try to find a suitable program in the program cache or build a new program using the image using it.

This commit implements the copy and memcpy operations to and from device_global. If the device_global does not have device_image_scope the memory operation will be on the underlying USM memory, while if the operation is on a device_global with device_image_scope the runtime will try to find a suitable program in the program cache or build a new program using the image using it. Signed-off-by: Steffen Larsen <[email protected]>

aelovikov-intel

Some early feedback after a brief read.

sycl/include/sycl/handler.hpp

sycl/include/sycl/queue.hpp

sycl/source/detail/device_global_map_entry.hpp

sycl/source/detail/kernel_program_cache.hpp

sycl/unittests/Extensions/DeviceGlobal.cpp

…memops

Signed-off-by: Larsen, Steffen <[email protected]>

aelovikov-intel · 2023-01-19T18:20:09Z

sycl/source/detail/program_manager/program_manager.cpp

+  // CacheKey is captured by reference so when we overwrite it later we can
+  // reuse this function.


Would it be helpful to modify the comment to point where such an overwrite is happening? Or would it become stale too soon?

I worry it will be outdated too easily if we are specific here, but there is a comment with

// Change device in the cache key to reduce copying of spec const data.

the place where the key is mutated.

sycl/source/detail/memory_manager.cpp

aelovikov-intel · 2023-01-19T18:29:02Z

sycl/source/detail/memory_manager.cpp

+    DepEvents.push_back(*ZIEvent);
+
+  MemoryManager::copy_usm(Src, Queue, NumBytes,
+                          reinterpret_cast<char *>(Dest) + Offset, DepEvents,


I hope this accepts DepEvents by ref, because otherwise the same lifetime/retain concerns...

It does not seem to, but since the events are just handles it should not be a problem. Since we are now keeping the event from being released for the duration of this call we do not have to worry about it dying while inside copy_usm. If copy_usm needs it to stay alive for longer than that it will have to maintain the lifetime itself.

That said, there could be an argument for changing the DepEvents arguments of these to references. I have done this for the new interfaces, but the rest should be done separately. Note that changing them may require an ABI break.

sycl/source/detail/memory_manager.cpp

sycl/unittests/Extensions/DeviceGlobal.cpp

This commit introduces a new RAII struct for guarding the lifetime of the zero-initialization event. This is used to prevent from being released while it is in use. Signed-off-by: Larsen, Steffen <[email protected]>

Signed-off-by: Larsen, Steffen <[email protected]>

This commit adds behavior for reading and writing to memory related to device_globals in the corresponding unittests. This allows the tests to check that the behavior of the memory operations are as expected. Signed-off-by: Larsen, Steffen <[email protected]>

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen · 2023-01-25T18:59:45Z

#8104 is the subset of this patch that refactors build and kernel cache to allow the memory operations to look up existing programs in the cache for device_globals with device_image_scope. @romanovvlad | @sergey-semenov - Could you please have a look at that patch?

Signed-off-by: Larsen, Steffen <[email protected]>

…memops

Signed-off-by: Larsen, Steffen <[email protected]>

sycl/source/detail/context_impl.cpp

sycl/source/detail/program_manager/program_manager.cpp

Signed-off-by: Larsen, Steffen <[email protected]>

sycl/source/detail/context_impl.cpp

aelovikov-intel · 2023-02-15T20:34:11Z

sycl/source/detail/device_global_map_entry.cpp

@@ -47,13 +47,12 @@ DeviceGlobalUSMMem::getZeroInitEvent(const plugin &Plugin) {
            *MZeroInitEvent, Plugin) == info::event_command_status::complete) {
      Plugin.call<PiApiKind::piEventRelease>(*MZeroInitEvent);
      MZeroInitEvent = {};
-      return std::nullopt;
+      return OwnedPiEvent(Plugin);


nit: Another option is to change reference to a pointer and default to nullptr.

Reference seems to be in line with how it's normally passed around, so since we have the plugin available here we may as well keep it to the norm. It also means we could have a variant that gets an event after the fact, though that's just a sidenote.

Signed-off-by: Larsen, Steffen <[email protected]>

bader · 2023-02-23T03:21:11Z

sycl/source/detail/device_global_map_entry.cpp

+      MZeroInitEvent = {};
+      return OwnedPiEvent(Plugin);
+    } else {
+      return std::move(OwnedPiEvent(*MZeroInitEvent, Plugin));


This code doesn't compile with clang.
https://github.com/intel/llvm/actions/runs/4244006981/jobs/7377519363

steffenlarsen requested a review from a team as a code owner January 17, 2023 15:29

steffenlarsen requested review from aelovikov-intel and romanovvlad January 17, 2023 15:29

steffenlarsen temporarily deployed to aws January 17, 2023 16:01 — with GitHub Actions Inactive

romanovvlad requested a review from sergey-semenov January 17, 2023 17:23

aelovikov-intel reviewed Jan 17, 2023

View reviewed changes

steffenlarsen added 2 commits January 18, 2023 09:28

Merge remote-tracking branch 'intel/sycl' into steffen/device_global_…

a30be07

…memops

Address comments

b8669bf

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen temporarily deployed to aws January 19, 2023 12:16 — with GitHub Actions Inactive

Fix double-release

ba5e673

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen temporarily deployed to aws January 19, 2023 14:19 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws January 19, 2023 15:32 — with GitHub Actions Inactive

aelovikov-intel reviewed Jan 19, 2023

View reviewed changes

sycl/unittests/Extensions/DeviceGlobal.cpp Outdated Show resolved Hide resolved

sycl/unittests/Extensions/DeviceGlobal.cpp Show resolved Hide resolved

sycl/unittests/Extensions/DeviceGlobal.cpp Outdated Show resolved Hide resolved

steffenlarsen added 3 commits January 25, 2023 03:32

Fix lifetime issue for zero-initialization event

7171256

This commit introduces a new RAII struct for guarding the lifetime of the zero-initialization event. This is used to prevent from being released while it is in use. Signed-off-by: Larsen, Steffen <[email protected]>

Fix formatting

2f322da

Signed-off-by: Larsen, Steffen <[email protected]>

Change DepEvents to const refs

5c5aa28

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen temporarily deployed to aws January 25, 2023 12:14 — with GitHub Actions Inactive

steffenlarsen added 2 commits January 25, 2023 04:18

Fix use of ActualDepEvents

ad84686

Signed-off-by: Larsen, Steffen <[email protected]>

Fix Windows symbols

2076136

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen temporarily deployed to aws January 25, 2023 12:46 — with GitHub Actions Inactive

Refactor common setup in device_global unittests

217ae06

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen temporarily deployed to aws January 25, 2023 13:35 — with GitHub Actions Inactive

steffenlarsen added 2 commits January 25, 2023 05:56

Fix formatting

2077d1c

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen temporarily deployed to aws January 25, 2023 14:25 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws January 25, 2023 14:57 — with GitHub Actions Inactive

steffenlarsen added 3 commits January 25, 2023 11:31

Make zero-init mandatory

130c48c

Signed-off-by: Larsen, Steffen <[email protected]>

Fix comments

0b23231

Signed-off-by: Larsen, Steffen <[email protected]>

Merge remote-tracking branch 'intel/sycl' into steffen/device_global_…

ff72519

…memops

steffenlarsen temporarily deployed to aws February 6, 2023 18:55 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws February 8, 2023 12:24 — with GitHub Actions Inactive

steffenlarsen requested a review from aelovikov-intel February 15, 2023 10:05

Change OwnedPiEvent to use std::optional internally

8fd798d

Signed-off-by: Larsen, Steffen <[email protected]>

aelovikov-intel reviewed Feb 15, 2023

View reviewed changes

sycl/source/detail/context_impl.cpp Outdated Show resolved Hide resolved

sycl/source/detail/program_manager/program_manager.cpp Outdated Show resolved Hide resolved

steffenlarsen added 2 commits February 15, 2023 11:16

Use OwnedPiEvent instead of optional. Address additional comments

4603345

Signed-off-by: Larsen, Steffen <[email protected]>

Fix formatting

d8ae8d0

Signed-off-by: Larsen, Steffen <[email protected]>

aelovikov-intel approved these changes Feb 15, 2023

View reviewed changes

steffenlarsen temporarily deployed to aws February 15, 2023 23:00 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws February 17, 2023 12:24 — with GitHub Actions Inactive

Merge branch 'sycl' into steffen/device_global_memops

aeb62a8

bader temporarily deployed to aws February 18, 2023 19:37 — with GitHub Actions Inactive

Fix use of OOO flag

3a014e2

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen temporarily deployed to aws February 22, 2023 11:05 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to aws February 22, 2023 11:48 — with GitHub Actions Inactive

steffenlarsen merged commit d6f5b35 into intel:sycl Feb 22, 2023

bader reviewed Feb 23, 2023

View reviewed changes

		// CacheKey is captured by reference so when we overwrite it later we can
		// reuse this function.

[SYCL] Implement device_global host-side memory operations #8022

[SYCL] Implement device_global host-side memory operations #8022

Uh oh!

Conversation

steffenlarsen commented Jan 17, 2023

Uh oh!

aelovikov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steffenlarsen commented Jan 25, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!