[SYCL] Fix event destruction (#5697)

The fix 34a4689 introduced problem with event::get_wait_list(). If a command has only dependencies introduced by user through "handler::depends_on()", these dependencies will be removed with the command. 
Proposed solution suggests not to remove direct dependencies of this event, but dependencies of the dependencies of the event when a command is released.

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

Author: maximdimakov

sycl/source/detail/event_impl.cpp

@@ -411,6 +411,16 @@ void event_impl::cleanupDependencyEvents() {
   MPreparedHostDepsEvents.clear();
 }
 
+void event_impl::cleanDepEventsThroughOneLevel() {
+  std::lock_guard<std::mutex> Lock(MMutex);
+  for (auto &Event : MPreparedDepsEvents) {
+    Event->cleanupDependencyEvents();
+  }
+  for (auto &Event : MPreparedHostDepsEvents) {
+    Event->cleanupDependencyEvents();
+  }
+}
+
 } // namespace detail
 } // namespace sycl
 } // __SYCL_INLINE_NAMESPACE(cl)

sycl/source/detail/event_impl.hpp

@@ -193,9 +193,12 @@ class event_impl {
   /// to the device yet.
   void flushIfNeeded(const QueueImplPtr &UserQueue);
 
-  /// Cleans dependencies of this event_impl
+  /// Cleans dependencies of this event_impl.
   void cleanupDependencyEvents();
 
+  /// Cleans dependencies of this event's dependencies.
+  void cleanDepEventsThroughOneLevel();
+
   /// Checks if this event is discarded by SYCL implementation.
   ///
   /// \return true if this event is discarded.

sycl/source/detail/scheduler/commands.hpp

@@ -189,7 +189,7 @@ class Command {
     return nullptr;
   }
 
-  virtual ~Command() { MEvent->cleanupDependencyEvents(); }
+  virtual ~Command() { MEvent->cleanDepEventsThroughOneLevel(); }
 
   const char *getBlockReason() const;
 

sycl/unittests/CMakeLists.txt

@@ -33,3 +33,4 @@ add_subdirectory(program_manager)
 add_subdirectory(assert)
 add_subdirectory(Extensions)
 add_subdirectory(windows)
+add_subdirectory(event)

sycl/unittests/event/CMakeLists.txt

@@ -0,0 +1,3 @@
+add_sycl_unittest(EventTests OBJECT
+  EventDestruction.cpp
+)

sycl/unittests/event/EventDestruction.cpp

@@ -0,0 +1,208 @@
+//==------- EventDestruction.cpp --- Check correct event destruction -------==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <helpers/CommonRedefinitions.hpp>
+#include <helpers/PiMock.hpp>
+#include <helpers/TestKernel.hpp>
+
+#include <gtest/gtest.h>
+
+#include <iostream>
+
+using namespace sycl;
+
+static int ReleaseCounter = 0;
+static pi_result redefinedEventRelease(pi_event event) {
+  ++ReleaseCounter;
+  return PI_SUCCESS;
+}
+
+pi_result redefinedMemBufferCreate(pi_context, pi_mem_flags, size_t size,
+                                   void *, pi_mem *,
+                                   const pi_mem_properties *) {
+  return PI_SUCCESS;
+}
+
+class EventDestructionTest : public ::testing::Test {
+public:
+  EventDestructionTest() : Plt{default_selector()} {}
+
+protected:
+  void SetUp() override {
+    if (Plt.is_host() || Plt.get_backend() != backend::opencl) {
+      std::cout << "This test is only supported on OpenCL backend\n";
+      std::cout << "Current platform is "
+                << Plt.get_info<sycl::info::platform::name>();
+      return;
+    }
+
+    Mock = std::make_unique<unittest::PiMock>(Plt);
+
+    setupDefaultMockAPIs(*Mock);
+    Mock->redefine<detail::PiApiKind::piEventRelease>(redefinedEventRelease);
+    Mock->redefine<sycl::detail::PiApiKind::piMemBufferCreate>(
+        redefinedMemBufferCreate);
+  }
+
+protected:
+  std::unique_ptr<unittest::PiMock> Mock;
+  platform Plt;
+};
+
+// Test that events are destructed in correct time
+TEST_F(EventDestructionTest, EventDestruction) {
+  if (Plt.is_host() || Plt.get_backend() != backend::opencl) {
+    return;
+  }
+  sycl::context Context{Plt};
+  sycl::queue Queue{Context, sycl::default_selector{}};
+
+  {
+    ReleaseCounter = 0;
+    sycl::event E1{};
+
+    {
+      sycl::event E0 = Queue.submit([&](cl::sycl::handler &cgh) {
+        cgh.single_task<TestKernel>([]() {});
+      });
+      E1 = Queue.submit([&](cl::sycl::handler &cgh) {
+        cgh.depends_on(E0);
+        cgh.single_task<TestKernel>([]() {});
+      });
+      E1.wait();
+    }
+    // When a command is cleared we clear now only dependencies of the
+    // dependencies of the associated event. So, when the command
+    // associated with E0 event is destroyed, this event is still in
+    // E1 dependencies, which will not be cleared.
+    // Therefore no event release should be called until here.
+    EXPECT_EQ(ReleaseCounter, 0);
+
+    sycl::event E2 = Queue.submit([&](cl::sycl::handler &cgh) {
+      cgh.depends_on(E1);
+      cgh.single_task<TestKernel>([]() {});
+    });
+    E2.wait();
+    // Dependencies of E1 should be cleared here. It depends on E0.
+    EXPECT_EQ(ReleaseCounter, 1);
+
+    sycl::event E3 = Queue.submit([&](cl::sycl::handler &cgh) {
+      cgh.depends_on({E1, E2});
+      cgh.single_task<TestKernel>([]() {});
+    });
+    E3.wait();
+    // Dependency of E1 has already cleared. E2 depends on E1 that
+    // can't be cleared yet.
+    EXPECT_EQ(ReleaseCounter, 1);
+  }
+
+  {
+    ReleaseCounter = 0;
+    int data[2] = {0, 1};
+    sycl::buffer<int, 1> Buf(&data[0], sycl::range<1>(2));
+    Queue.submit([&](cl::sycl::handler &cgh) {
+      auto Acc = Buf.get_access<sycl::access::mode::read_write>(cgh);
+      cgh.single_task<TestKernel>([=]() {});
+    });
+
+    Queue.submit([&](cl::sycl::handler &cgh) {
+      auto Acc = Buf.get_access<sycl::access::mode::read_write>(cgh);
+      cgh.single_task<TestKernel>([=]() {});
+    });
+    sycl::event E1 = Queue.submit([&](cl::sycl::handler &cgh) {
+      auto Acc = Buf.get_access<sycl::access::mode::read_write>(cgh);
+      cgh.single_task<TestKernel>([=]() {});
+    });
+    sycl::event E2 = Queue.submit([&](cl::sycl::handler &cgh) {
+      auto Acc = Buf.get_access<sycl::access::mode::read_write>(cgh);
+      cgh.single_task<TestKernel>([=]() {});
+    });
+    E2.wait();
+    // Dependencies are deleted through one level of dependencies. When
+    // fourth command group is submitted the destructor of third command
+    // is called. It depends on second command, so dependencies of second
+    // command will be cleared. It leads to release event associated with
+    // first command
+    EXPECT_EQ(ReleaseCounter, 1);
+  }
+}
+
+// Test for event::get_wait_list
+TEST_F(EventDestructionTest, GetWaitList) {
+  if (Plt.is_host() || Plt.get_backend() != backend::opencl) {
+    return;
+  }
+  ReleaseCounter = 0;
+  sycl::context Context{Plt};
+  sycl::queue Queue{Context, sycl::default_selector{}};
+  // Test for get_wait_list with host_task
+  {
+    sycl::event eA =
+        Queue.submit([&](sycl::handler &cgh) { cgh.host_task([]() {}); });
+    sycl::event eB = Queue.submit([&](sycl::handler &cgh) {
+      cgh.depends_on(eA);
+      cgh.host_task([]() {});
+    });
+
+    auto wait_list = eB.get_wait_list();
+    ASSERT_EQ(wait_list.size(), (size_t)1);
+    ASSERT_EQ(wait_list[0], eA);
+
+    sycl::event eC = Queue.submit([&](sycl::handler &cgh) {
+      cgh.depends_on({eA, eB});
+      cgh.host_task([]() {});
+    });
+
+    wait_list = eC.get_wait_list();
+    ASSERT_EQ(wait_list.size(), (size_t)2);
+    ASSERT_EQ(wait_list[0], eA);
+    ASSERT_EQ(wait_list[1], eB);
+
+    eC.wait();
+  }
+
+  // Test for get_wait_list with single_task
+  {
+    sycl::event E1{};
+
+    {
+      sycl::event E0 = Queue.submit([&](cl::sycl::handler &cgh) {
+        cgh.single_task<TestKernel>([]() {});
+      });
+      E1 = Queue.submit([&](cl::sycl::handler &cgh) {
+        cgh.depends_on(E0);
+        cgh.single_task<TestKernel>([]() {});
+      });
+      E1.wait();
+      auto wait_list = E1.get_wait_list();
+      ASSERT_EQ(wait_list.size(), (size_t)1);
+      ASSERT_EQ(wait_list[0], E0);
+    }
+
+    auto wait_list = E1.get_wait_list();
+    ASSERT_EQ(wait_list.size(), (size_t)1);
+    EXPECT_EQ(ReleaseCounter, 0);
+
+    sycl::event E2 = Queue.submit([&](cl::sycl::handler &cgh) {
+      cgh.depends_on(E1);
+      cgh.single_task<TestKernel>([]() {});
+    });
+    E2.wait();
+
+    sycl::event E3 = Queue.submit([&](cl::sycl::handler &cgh) {
+      cgh.depends_on({E1, E2});
+      cgh.single_task<TestKernel>([]() {});
+    });
+    E3.wait();
+
+    wait_list = E3.get_wait_list();
+    ASSERT_EQ(wait_list.size(), (size_t)2);
+    ASSERT_EQ(wait_list[0], E1);
+    ASSERT_EQ(wait_list[1], E2);
+  }
+}

sycl/unittests/queue/Wait.cpp

@@ -177,31 +177,6 @@ TEST(QueueWait, QueueWaitTest) {
     ASSERT_TRUE(TestContext.PiQueueFinishCalled);
   }
 
-  // Test for event::get_wait_list
-  {
-    sycl::event eA =
-        Q.submit([&](sycl::handler &cgh) { cgh.host_task([]() {}); });
-    sycl::event eB = Q.submit([&](sycl::handler &cgh) {
-      cgh.depends_on(eA);
-      cgh.host_task([]() {});
-    });
-
-    auto res = eB.get_wait_list();
-    assert(res.size() == 1);
-    ASSERT_EQ(res[0], eA);
-
-    sycl::event eC = Q.submit([&](sycl::handler &cgh) {
-      cgh.depends_on({eA, eB});
-      cgh.host_task([]() {});
-    });
-
-    res = eC.get_wait_list();
-    assert(res.size() == 2);
-    ASSERT_EQ(res[0], eA);
-    ASSERT_EQ(res[1], eB);
-
-    eC.wait();
-  }
   // Test behaviour for emulating an OOO queue with multiple in-order ones.
   TestContext = {};
   TestContext.SupportOOO = false;