[SYCL][L0] Add support for pinned host memory. #2633

rbegam · 2020-10-14T02:53:12Z

This change implements the support for pinned host memory in level_zero plugin. It allocates pinned host memory when PI_MEM_FLAGS_HOST_PTR_ALLOC is set. Pinned host memories are automatically accessible from the device through PCI. This property also ensures that the memory map/unmap operations are free of cost and the buffer is optimized for frequent accesses from the host.
A test is added for this change in a separate PR for the intel/llvm-test-suite: intel/llvm-test-suite#83

Signed-off-by: rbegam [email protected]

smaslov-intel

see comments

sycl/plugins/level_zero/pi_level_zero.cpp

smaslov-intel

Add a test

sycl/plugins/level_zero/pi_level_zero.cpp

smaslov-intel

see comments

smaslov-intel · 2020-11-03T23:07:26Z

We do need a test for this. Please make sure to cover the interaction of such (pinned) allocated memory and mapping/unmapping.

smaslov-intel · 2020-11-03T23:10:04Z

sycl/plugins/level_zero/pi_level_zero.cpp


-    ZE_CALL(
-        zeMemAllocDevice(Context->ZeContext, &ZeDesc, Size, 1, ZeDevice, &Ptr));
+    ZE_CALL(zeMemAllocShared(Context->ZeContext, &ZeDeviceMemDesc,


If the DeviceIsIntegrated then the zeMemAllocHost is sufficient, and I believe is faster. Otherwise, as requested by user, the allocation should be accessible from the host, so zeMemAllocSahred seems good then. I am not sure what will be the interaction of such allocated buffer with the map/unmap operations, whose implementations are highly coupled with this create one. Please add a functional test.

there is a test for pinned memory https://github.com/intel/llvm/pull/2080/files?file-filters%5B%5D=.cpp#diff-c0fd7ed69152328e50c4747dc8870b42d5e085b05a3f20cba5c42f79a4002e14

smaslov-intel · 2020-11-03T23:13:44Z

sycl/plugins/level_zero/pi_level_zero.cpp

@@ -1957,6 +1973,8 @@ pi_result piMemBufferCreate(pi_context Context, pi_mem_flags Flags, size_t Size,
                                              HostPtr, Size, nullptr, 0,
                                              nullptr));
      }
+    } else if ((Flags & PI_MEM_FLAGS_HOST_PTR_ALLOC) != 0) {


You are not setting the HostPtr to the allocated memory, so you can't get here, right? (line 1962 should be false). So why is this change needed?

Maybe instead verify that HostPtr is null when PI_MEM_FLAGS_HOST_PTR_ALLOC is used?

@smaslov-intel are you saying that hostPtr and PI_MEM_FLAGS_HOST_PTR_ALLOC is mutually exclusive? My understanding was that they are not. fixing it.

Yes, see in https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/clCreateBuffer.html:

CL_MEM_ALLOC_HOST_PTR and CL_MEM_USE_HOST_PTR are mutually exclusive.

PI_MEM_FLAGS_HOST_PTR_ALLOC is analogous to CL_MEM_ALLOC_HOST_PTR

jbrodman · 2020-11-16T22:13:25Z

I'm confused about what this PR is doing.

Is it adding support for https://github.com/intel/llvm/tree/sycl/sycl/doc/extensions/UsePinnedMemoryProperty?

If so I'm confused: I don't get why USM shared allocations are used here at all.
It seems like a buffer should be represented as 2 allocations: 1 in host memory, 1 in device memory.
The property says that the host memory one should likely be done as a USM host allocation instead of system malloc as that usually means it's pinned.

sycl/plugins/level_zero/pi_level_zero.cpp

smaslov-intel · 2020-12-24T05:25:29Z

sycl/plugins/level_zero/pi_level_zero.hpp

@@ -390,6 +390,7 @@ struct _pi_mem : _pi_object {
  char *MapHostPtr;

  // Flag to indicate that this memory is allocated in host memory
+  // if created with PI_MEM_FLAGS_HOST_PTR_ALLOC and/or integrated devices.


NIT: Let's not list all the cases when memory is allocated on host, since that might change in future.

smaslov-intel

LGTM

romanovvlad · 2020-12-24T08:22:39Z

sycl/test/on-device/basic_tests/use_pinned_host_memory.cpp

+      buffer<int, 1> a(data1, range<1>(10), {property::buffer::use_host_ptr()});
+      buffer<int, 1> b(
+          range<1>(10),
+          {ext::oneapi::property::buffer::use_pinned_host_memory()});


Can we have a test which verifies that piMemBufferCreate with PI_MEM_FLAGS_HOST_PTR_ALLOC allocates memory using zeMemAllocHost?

romanovvlad · 2020-12-24T08:24:36Z

sycl/plugins/level_zero/pi_level_zero.cpp

+
+    ZE_CALL(zeMemAllocHost(Context->ZeContext, &ZeDesc, Size, 1, &Ptr));
+
+  } else if (DeviceIsIntegrated) {
    ze_host_mem_alloc_desc_t ZeDesc = {};
    ZeDesc.flags = 0;



Minor. Probably, these two branches could be merged.

romanovvlad · 2020-12-24T08:25:48Z

sycl/test/on-device/basic_tests/use_pinned_host_memory.cpp

+#include <cassert>



Suggested change

#include <cassert>

#include <cassert>

Minor.

rbegam · 2020-12-24T20:28:15Z

the use_pinned_host_memory.cpp test seems to be moved into intel/llvm-test-suite.
@romanovvlad I am going to remove the test from this PR and submit a new one for intel/llvm-test-suite. is that okay?

Signed-off-by: rbegam <[email protected]>

romanovvlad · 2020-12-25T09:26:12Z

the use_pinned_host_memory.cpp test seems to be moved into intel/llvm-test-suite.
@romanovvlad I am going to remove the test from this PR and submit a new one for intel/llvm-test-suite. is that okay?

That's OK.

rbegam · 2020-12-28T17:38:58Z

ping @smaslov-intel

smaslov-intel

LGTM. It would be great to put a link to the test in the PR (or better yet into description of the commit).

pvchupin · 2021-01-05T17:22:51Z

@rbegam, do you have link to test? This change seems ready to merge.

rbegam · 2021-01-05T18:20:05Z

@pvchupin yes, I am updating the commit msg with it.

When llvm-spirv is built in-tree as part of LLVM, and the LLVM build also includes libclc, libclc will require a native version of llvm-spirv to be built. To accommodate this, call setup_host_tool. This change by itself is intended to have no effect at the moment, but enables a future LLVM change. Original commit: KhronosGroup/SPIRV-LLVM-Translator@a276363ccac20bf

rbegam requested a review from smaslov-intel as a code owner October 14, 2020 02:53

smaslov-intel suggested changes Oct 14, 2020

View reviewed changes

smaslov-intel reviewed Oct 14, 2020

View reviewed changes

sycl/plugins/level_zero/pi_level_zero.cpp Outdated Show resolved Hide resolved

smaslov-intel suggested changes Oct 15, 2020

View reviewed changes

rbegam force-pushed the pinned-host branch from 3d90552 to 1f6f79a Compare October 20, 2020 00:16

smaslov-intel reviewed Oct 20, 2020

View reviewed changes

sycl/plugins/level_zero/pi_level_zero.cpp Outdated Show resolved Hide resolved

smaslov-intel suggested changes Oct 20, 2020

View reviewed changes

rbegam force-pushed the pinned-host branch 2 times, most recently from c621923 to bb4bd5e Compare October 26, 2020 18:02

pvchupin requested a review from smaslov-intel November 3, 2020 22:42

smaslov-intel suggested changes Nov 3, 2020

View reviewed changes

jbrodman self-requested a review November 16, 2020 22:13

rbegam force-pushed the pinned-host branch from bb4bd5e to 3fdc03b Compare November 30, 2020 21:52

smaslov-intel reviewed Dec 3, 2020

View reviewed changes

sycl/plugins/level_zero/pi_level_zero.cpp Show resolved Hide resolved

rbegam force-pushed the pinned-host branch from 5ed68b9 to 3b723da Compare December 23, 2020 23:42

rbegam requested a review from a team as a code owner December 23, 2020 23:42

smaslov-intel reviewed Dec 24, 2020

View reviewed changes

smaslov-intel previously approved these changes Dec 24, 2020

View reviewed changes

romanovvlad reviewed Dec 24, 2020

View reviewed changes

rbegam dismissed smaslov-intel’s stale review via cf6bf49 December 24, 2020 20:08

rbegam added 7 commits December 24, 2020 12:30

[SYCL][L0] Add support for pinned host memory.

a379b4c

Signed-off-by: rbegam <[email protected]>

clang formatted.

b8d55db

Signed-off-by: rbegam <[email protected]>

removed the copy from host to device.

9378a1f

Signed-off-by: rbegam <[email protected]>

added comment.

40a2fed

Signed-off-by: rbegam <[email protected]>

clang formatted.

0ec8219

Signed-off-by: rbegam <[email protected]>

remove copy.

689f3fc

Signed-off-by: rbegam <[email protected]>

resolved conflicts.

2de2710

Signed-off-by: rbegam <[email protected]>

rbegam added 9 commits December 24, 2020 12:30

clang formatted.

c30c840

Signed-off-by: rbegam <[email protected]>

add assert.

1244582

Signed-off-by: rbegam <[email protected]>

update allocation.

608ce45

Signed-off-by: rbegam <[email protected]>

corrects unnecessary code change.

fadbbfc

Signed-off-by: rbegam <[email protected]>

clang formatted.

cb74ace

Signed-off-by: rbegam <[email protected]>

adds a test and comments.

41562aa

Signed-off-by: rbegam <[email protected]>

clang format.

6de96dd

Signed-off-by: rbegam <[email protected]>

minor fixes.

47ac145

Signed-off-by: rbegam <[email protected]>

another minot change.

9f987b7

Signed-off-by: rbegam <[email protected]>

rbegam force-pushed the pinned-host branch from 06a77b4 to 9f987b7 Compare December 24, 2020 20:38

smaslov-intel approved these changes Jan 4, 2021

View reviewed changes

pvchupin merged commit 0b9a749 into intel:sycl Jan 5, 2021

[SYCL][L0] Add support for pinned host memory. #2633

[SYCL][L0] Add support for pinned host memory. #2633

Uh oh!

Conversation

rbegam commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

smaslov-intel commented Nov 3, 2020

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!

rbegam Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbrodman commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smaslov-intel left a comment

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!

rbegam commented Dec 24, 2020

Uh oh!

romanovvlad commented Dec 25, 2020

Uh oh!

rbegam commented Dec 28, 2020

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

pvchupin commented Jan 5, 2021

Uh oh!

rbegam commented Jan 5, 2021

Uh oh!

Uh oh!

rbegam commented Oct 14, 2020 •

edited

Loading

rbegam Nov 16, 2020 •

edited

Loading

jbrodman commented Nov 16, 2020 •

edited

Loading