[SYCL] Switch from COPY_HOST_PTR to write for devices w/o HUM (#3105)

sergey-semenov · web-flow · commit ad8c9d172d7e · 2021-02-03T17:37:44.000+03:00
Switch from initializing native memory objects with COPY_HOST_PTR to
creating them without a host ptr and following up with a write
operation.

The reasoning behind this change is that some OpenCL runtimes (e. g.
FPGA) use the queue the memory object write is enqueued to as a hint to
which device from the context this memory should be copied to, as
opposed to COPY_HOST_PTR where only the context itself is known. This
led to unnecessary temporary host copy made by the FPGA runtime during
buffer creation.
diff --git a/sycl/include/CL/sycl/detail/sycl_mem_obj_t.hpp b/sycl/include/CL/sycl/detail/sycl_mem_obj_t.hpp
@@ -296,6 +296,8 @@ class __SYCL_EXPORT SYCLMemObjT : public SYCLMemObjI {
 
   ContextImplPtr getInteropContext() const override { return MInteropContext; }
 
+  bool hasUserDataPtr() const { return MUserPtr != nullptr; };
+
 protected:
   // An allocateMem helper that determines which host ptr to use
   void determineHostPtr(const ContextImplPtr &Context, bool InitFromUserData,
diff --git a/sycl/source/detail/memory_manager.cpp b/sycl/source/detail/memory_manager.cpp
@@ -128,24 +128,9 @@ RT::PiMemFlags getMemObjCreationFlags(const ContextImplPtr &TargetContext,
                                       void *UserPtr, bool HostPtrReadOnly) {
   // Create read_write mem object to handle arbitrary uses.
   RT::PiMemFlags Result = PI_MEM_FLAGS_ACCESS_RW;
-  if (UserPtr) {
-    if (HostPtrReadOnly)
-      Result |= PI_MEM_FLAGS_HOST_PTR_COPY;
-    else {
-      // Create the memory object using the host pointer only if the devices
-      // support host_unified_memory to avoid potential copy overhead.
-      // TODO This check duplicates the one performed in the GraphBuilder during
-      // AllocaCommand creation. This information should be propagated here
-      // instead, which would be a breaking ABI change.
-      bool HostUnifiedMemory = true;
-      for (const device &Device : TargetContext->getDevices())
-        HostUnifiedMemory &=
-            Device.get_info<info::device::host_unified_memory>();
-      Result |= HostUnifiedMemory ? PI_MEM_FLAGS_HOST_PTR_USE
-                                  : PI_MEM_FLAGS_HOST_PTR_COPY;
-    }
-  }
-
+  if (UserPtr)
+    Result |= HostPtrReadOnly ? PI_MEM_FLAGS_HOST_PTR_COPY
+                              : PI_MEM_FLAGS_HOST_PTR_USE;
   return Result;
 }
 
diff --git a/sycl/source/detail/scheduler/graph_builder.cpp b/sycl/source/detail/scheduler/graph_builder.cpp
@@ -635,45 +635,70 @@ AllocaCommandBase *Scheduler::GraphBuilder::getOrCreateAllocaForReq(
                                 0 /*ReMOffsetInBytes*/, false /*MIsSubBuffer*/);
       // Can reuse user data for the first allocation. Do so if host unified
       // memory is supported regardless of the access mode (the pointer will be
-      // reused) or if it's not and the access mode is not discard (the pointer
-      // will be copied).
+      // reused). For devices without host unified memory the initialization
+      // will be performed as a write operation.
       // TODO the case where the first alloca is made with a discard mode and
       // the user pointer is read-only is still not handled: it leads to
       // unnecessary copy on devices with unified host memory support.
+      const bool HostUnifiedMemory =
+          checkHostUnifiedMemory(Queue->getContextImplPtr());
       const bool InitFromUserData =
-          Record->MAllocaCommands.empty() &&
-          (checkHostUnifiedMemory(Queue->getContextImplPtr()) ||
-           (Req->MAccessMode != access::mode::discard_write &&
-            Req->MAccessMode != access::mode::discard_read_write));
-
+          Record->MAllocaCommands.empty() && HostUnifiedMemory;
       AllocaCommandBase *LinkedAllocaCmd = nullptr;
-      // If it is not the first allocation, try to setup a link
-      // FIXME: Temporary limitation, linked alloca commands for an image is not
-      // supported because map operation is not implemented for an image.
-      if (!Record->MAllocaCommands.empty() &&
-          Req->MSYCLMemObj->getType() == SYCLMemObjI::MemObjType::BUFFER)
-        // Current limitation is to setup link between current allocation and
-        // new one. There could be situations when we could setup link with
-        // "not" current allocation, but it will require memory copy.
-        // Can setup link between cl and host allocations only
-        if (Queue->is_host() != Record->MCurContext->is_host()) {
-          // Linked commands assume that the host allocation is reused by the
-          // plugin runtime and that can lead to unnecessary copy overhead on
-          // devices that do not support host unified memory. Do not link the
-          // allocations in this case.
-          const ContextImplPtr &NonHostCtx = Queue->is_host()
-                                                 ? Record->MCurContext
-                                                 : Queue->getContextImplPtr();
-          if (checkHostUnifiedMemory(NonHostCtx)) {
-            AllocaCommandBase *LinkedAllocaCmdCand =
-                findAllocaForReq(Record, Req, Record->MCurContext);
-
-            // Cannot setup link if candidate is linked already
-            if (LinkedAllocaCmdCand && !LinkedAllocaCmdCand->MLinkedAllocaCmd) {
-              LinkedAllocaCmd = LinkedAllocaCmdCand;
-            }
+
+      // For the first allocation on a device without host unified memory we
+      // might need to also create a host alloca right away in order to perform
+      // the initial memory write.
+      if (Record->MAllocaCommands.empty()) {
+        if (!HostUnifiedMemory &&
+            Req->MAccessMode != access::mode::discard_write &&
+            Req->MAccessMode != access::mode::discard_read_write) {
+          // There's no need to make a host allocation if the buffer is not
+          // initialized with user data.
+          // TODO casting is required here to get the necessary information
+          // without breaking ABI, replace with the next major version.
+          auto *MemObj = static_cast<SYCLMemObjT *>(Req->MSYCLMemObj);
+          if (MemObj->hasUserDataPtr()) {
+            QueueImplPtr DefaultHostQueue =
+                Scheduler::getInstance().getDefaultHostQueue();
+            AllocaCommand *HostAllocaCmd = new AllocaCommand(
+                DefaultHostQueue, FullReq, true /* InitFromUserData */,
+                nullptr /* LinkedAllocaCmd */);
+            Record->MAllocaCommands.push_back(HostAllocaCmd);
+            Record->MWriteLeaves.push_back(HostAllocaCmd);
+            ++(HostAllocaCmd->MLeafCounter);
+            Record->MCurContext = DefaultHostQueue->getContextImplPtr();
           }
         }
+      } else {
+        // If it is not the first allocation, try to setup a link
+        // FIXME: Temporary limitation, linked alloca commands for an image is
+        // not supported because map operation is not implemented for an image.
+        if (Req->MSYCLMemObj->getType() == SYCLMemObjI::MemObjType::BUFFER)
+          // Current limitation is to setup link between current allocation and
+          // new one. There could be situations when we could setup link with
+          // "not" current allocation, but it will require memory copy.
+          // Can setup link between cl and host allocations only
+          if (Queue->is_host() != Record->MCurContext->is_host()) {
+            // Linked commands assume that the host allocation is reused by the
+            // plugin runtime and that can lead to unnecessary copy overhead on
+            // devices that do not support host unified memory. Do not link the
+            // allocations in this case.
+            bool HostUnifiedMemoryOnNonHostDevice =
+                Queue->is_host() ? checkHostUnifiedMemory(Record->MCurContext)
+                                 : HostUnifiedMemory;
+            if (HostUnifiedMemoryOnNonHostDevice) {
+              AllocaCommandBase *LinkedAllocaCmdCand =
+                  findAllocaForReq(Record, Req, Record->MCurContext);
+
+              // Cannot setup link if candidate is linked already
+              if (LinkedAllocaCmdCand &&
+                  !LinkedAllocaCmdCand->MLinkedAllocaCmd) {
+                LinkedAllocaCmd = LinkedAllocaCmdCand;
+              }
+            }
+          }
+      }
 
       AllocaCmd =
           new AllocaCommand(Queue, FullReq, InitFromUserData, LinkedAllocaCmd);
diff --git a/sycl/unittests/scheduler/NoHostUnifiedMemory.cpp b/sycl/unittests/scheduler/NoHostUnifiedMemory.cpp
@@ -30,13 +30,11 @@ static pi_result redefinedDeviceGetInfo(pi_device Device,
   return PI_SUCCESS;
 }
 
-static RT::PiMemFlags ExpectedMemObjFlags;
-
 static pi_result
 redefinedMemBufferCreate(pi_context context, pi_mem_flags flags, size_t size,
                          void *host_ptr, pi_mem *ret_mem,
                          const pi_mem_properties *properties = nullptr) {
-  EXPECT_EQ(flags, ExpectedMemObjFlags);
+  EXPECT_EQ(flags, PI_MEM_FLAGS_ACCESS_RW);
   return PI_SUCCESS;
 }
 
@@ -85,59 +83,70 @@ TEST_F(SchedulerTest, NoHostUnifiedMemory) {
       new detail::queue_impl(detail::getSyclObjImpl(HostDevice), {}, {})};
 
   MockScheduler MS;
-  // Check non-host -> host alloca with non-discard access mode
+  // Check non-host alloca with non-discard access mode
   {
     int val;
     buffer<int, 1> Buf(&val, range<1>(1));
     detail::Requirement Req = getMockRequirement(Buf);
 
-    // The host pointer should be copied during the non-host allocation in this
-    // case.
-    ExpectedMemObjFlags = PI_MEM_FLAGS_ACCESS_RW | PI_MEM_FLAGS_HOST_PTR_COPY;
-
     detail::MemObjRecord *Record = MS.getOrInsertMemObjRecord(QImpl, &Req);
     detail::AllocaCommandBase *NonHostAllocaCmd =
         MS.getOrCreateAllocaForReq(Record, &Req, QImpl);
 
-    detail::AllocaCommandBase *HostAllocaCmd =
-        MS.getOrCreateAllocaForReq(Record, &Req, DefaultHostQueue);
+    // Both non-host and host allocations should be created in this case in
+    // order to perform a memory move.
+    EXPECT_EQ(Record->MAllocaCommands.size(), 2U);
+    detail::AllocaCommandBase *HostAllocaCmd = Record->MAllocaCommands[0];
+    EXPECT_TRUE(HostAllocaCmd->getQueue()->is_host());
     EXPECT_TRUE(!HostAllocaCmd->MLinkedAllocaCmd);
     EXPECT_TRUE(!NonHostAllocaCmd->MLinkedAllocaCmd);
+    EXPECT_TRUE(Record->MCurContext->is_host());
 
-    detail::Command *MemoryMove =
-        MS.insertMemoryMove(Record, &Req, DefaultHostQueue);
+    detail::Command *MemoryMove = MS.insertMemoryMove(Record, &Req, QImpl);
     EXPECT_EQ(MemoryMove->getType(), detail::Command::COPY_MEMORY);
   }
-  // Check non-host -> host alloca with discard access modes
+  // Check non-host alloca with discard access modes
   {
     int val;
     buffer<int, 1> Buf(&val, range<1>(1));
     detail::Requirement Req = getMockRequirement(Buf);
-    // The host pointer should be ignored due to the discard access mode.
-    ExpectedMemObjFlags = PI_MEM_FLAGS_ACCESS_RW;
 
     detail::Requirement DiscardReq = getMockRequirement(Buf);
     DiscardReq.MAccessMode = access::mode::discard_read_write;
 
+    // No need to create a host allocation in this case since the data can be
+    // discarded.
     detail::MemObjRecord *Record = MS.getOrInsertMemObjRecord(QImpl, &Req);
     MS.getOrCreateAllocaForReq(Record, &DiscardReq, QImpl);
+    EXPECT_EQ(Record->MAllocaCommands.size(), 1U);
+  }
+  // Check non-host alloca without user pointer
+  {
+    buffer<int, 1> Buf(range<1>(1));
+    detail::Requirement Req = getMockRequirement(Buf);
+
+    // No need to create a host allocation in this case since there's no data to
+    // initialize the buffer with.
+    detail::MemObjRecord *Record = MS.getOrInsertMemObjRecord(QImpl, &Req);
+    MS.getOrCreateAllocaForReq(Record, &Req, QImpl);
+    EXPECT_EQ(Record->MAllocaCommands.size(), 1U);
   }
   // Check host -> non-host alloca
   {
     int val;
     buffer<int, 1> Buf(&val, range<1>(1));
     detail::Requirement Req = getMockRequirement(Buf);
 
-    // No copy expected during the second allocation, it is performed as a
-    // separate command.
-    ExpectedMemObjFlags = PI_MEM_FLAGS_ACCESS_RW;
-
+    // No special handling required: alloca commands are created one after
+    // another and the transfer is done via a write operation.
     detail::MemObjRecord *Record =
         MS.getOrInsertMemObjRecord(DefaultHostQueue, &Req);
     detail::AllocaCommandBase *HostAllocaCmd =
         MS.getOrCreateAllocaForReq(Record, &Req, DefaultHostQueue);
+    EXPECT_EQ(Record->MAllocaCommands.size(), 1U);
     detail::AllocaCommandBase *NonHostAllocaCmd =
         MS.getOrCreateAllocaForReq(Record, &Req, QImpl);
+    EXPECT_EQ(Record->MAllocaCommands.size(), 2U);
     EXPECT_TRUE(!HostAllocaCmd->MLinkedAllocaCmd);
     EXPECT_TRUE(!NonHostAllocaCmd->MLinkedAllocaCmd);
 
@@ -150,7 +159,6 @@ TEST_F(SchedulerTest, NoHostUnifiedMemory) {
     int val;
     buffer<int, 1> Buf(&val, range<1>(1));
     detail::Requirement Req = getMockRequirement(Buf);
-    ExpectedMemObjFlags = PI_MEM_FLAGS_ACCESS_RW | PI_MEM_FLAGS_HOST_PTR_COPY;
 
     detail::Requirement DiscardReq = getMockRequirement(Buf);
     DiscardReq.MAccessMode = access::mode::discard_read_write;
