[SYCL][Docs] Make last event optional for queues with no previous work (#16645)

This commit changes the extension documentation and implementation of
ext_oneapi_get_last_event to return an std::optional and return a
std::nullopt for the case where the queue had no work previously
submitted to it.

---------

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

Author: steffenlarsen

sycl/doc/extensions/experimental/sycl_ext_oneapi_in_order_queue_events.asciidoc

@@ -96,7 +96,7 @@ namespace sycl {
 class queue {
   ...
 
-  event ext_oneapi_get_last_event() const { /*...*/ }
+  std::optional<event> ext_oneapi_get_last_event() const { /*...*/ }
 
   void ext_oneapi_set_external_event(const event &external_event) { /*...*/ }
 }
@@ -113,13 +113,16 @@ These new APIs have the following behaviour:
 a| 
 [source, c++]
 ----
-event ext_oneapi_get_last_event() const;
+std::optional<event> ext_oneapi_get_last_event() const;
 ----
 | Returns an event representing the execution of the last command submitted to
 the queue. If a call to `ext_oneapi_set_external_event()` on the queue happened
 after all previously submitted commands to the queue, this function returns a
 copy of the event that was passed to `ext_oneapi_set_external_event()`.
 
+If no commands have been submitted to the queue prior to a call to
+`ext_oneapi_set_external_event()`, the call will return `std::nullopt`.
+
 Calls to this member function throw a `sycl::exception` with `errc::invalid` if
 the queue does not have the `property::queue::in_order` property.
 

sycl/include/sycl/detail/optional.hpp

@@ -26,12 +26,12 @@ template <typename T> class optional {
   template <typename U>
   constexpr optional(const optional<U> &Other)
       : ContainsValue{Other.ContainsValue} {
-    new (Storage) T(Other.Value);
+    new (Storage) T(Other.value());
   }
   template <typename U>
-  constexpr optional(optional<U> &&Other)
-      : ContainsValue{std::move(Other.ContainsValue)} {
-    new (Storage) T(std::move(Other.Value));
+  constexpr optional(optional<U> &&Other) : ContainsValue{Other.ContainsValue} {
+    new (Storage) T(std::move(Other.value()));
+    Other.ContainsValue = false;
   }
 
   constexpr optional(T &&Value) : ContainsValue{true} {
@@ -137,6 +137,11 @@ template <typename T> class optional {
   constexpr T &&operator*() && { return value(); }
   constexpr const T &&operator*() const && { return value(); }
 
+  constexpr operator std::optional<T>() {
+    return has_value() ? std::optional<T>{value()}
+                       : std::optional<T>{std::nullopt};
+  }
+
 private:
   alignas(alignof(T)) char Storage[sizeof(T)] = {0};
   bool ContainsValue = false;

sycl/include/sycl/queue.hpp

@@ -2738,7 +2738,9 @@ class __SYCL_EXPORT queue : public detail::OwnerLessBase<queue> {
 
   ur_native_handle_t getNative(int32_t &NativeHandleDesc) const;
 
-  event ext_oneapi_get_last_event() const;
+  std::optional<event> ext_oneapi_get_last_event() const {
+    return static_cast<std::optional<event>>(ext_oneapi_get_last_event_impl());
+  }
 
   void ext_oneapi_set_external_event(const event &external_event);
 
@@ -3007,6 +3009,11 @@ class __SYCL_EXPORT queue : public detail::OwnerLessBase<queue> {
                                const std::vector<event> &DepEvents);
   const property_list &getPropList() const;
 
+  // Helper implementation for ext_oneapi_get_last_event. This is needed to
+  // avoid issues where std::optional has a different layout between user-code
+  // and library.
+  sycl::detail::optional<event> ext_oneapi_get_last_event_impl() const;
+
   template <typename KernelName>
   static constexpr detail::code_location getCodeLocation() {
     return {detail::getKernelFileName<KernelName>(),

sycl/source/detail/queue_impl.cpp

@@ -277,7 +277,7 @@ event queue_impl::memcpyFromDeviceGlobal(
       DeviceGlobalPtr, IsDeviceImageScope, Self, NumBytes, Offset, Dest);
 }
 
-event queue_impl::getLastEvent() {
+sycl::detail::optional<event> queue_impl::getLastEvent() {
   {
     // The external event is required to finish last if set, so it is considered
     // the last event if present.
@@ -287,12 +287,12 @@ event queue_impl::getLastEvent() {
   }
 
   std::lock_guard<std::mutex> Lock{MMutex};
+  if (MGraph.expired() && !MDefaultGraphDeps.LastEventPtr)
+    return std::nullopt;
   if (MDiscardEvents)
     return createDiscardedEvent();
   if (!MGraph.expired() && MExtGraphDeps.LastEventPtr)
     return detail::createSyclObjFromImpl<event>(MExtGraphDeps.LastEventPtr);
-  if (!MDefaultGraphDeps.LastEventPtr)
-    MDefaultGraphDeps.LastEventPtr = std::make_shared<event_impl>(std::nullopt);
   return detail::createSyclObjFromImpl<event>(MDefaultGraphDeps.LastEventPtr);
 }
 

sycl/source/detail/queue_impl.hpp

@@ -180,7 +180,7 @@ class queue_impl {
 #endif
   }
 
-  event getLastEvent();
+  sycl::detail::optional<event> getLastEvent();
 
 private:
   void queue_impl_interop(ur_queue_handle_t UrQueue) {

sycl/source/queue.cpp

@@ -288,7 +288,13 @@ getBarrierEventForInorderQueueHelper(const detail::QueueImplPtr QueueImpl) {
   assert(!QueueImpl->getCommandGraph() &&
          "Should not be called in on graph recording.");
 
-  return QueueImpl->getLastEvent();
+  sycl::detail::optional<event> LastEvent = QueueImpl->getLastEvent();
+  if (LastEvent)
+    return *LastEvent;
+
+  // If there was no last event, we create an empty one.
+  return detail::createSyclObjFromImpl<event>(
+      std::make_shared<detail::event_impl>(std::nullopt));
 }
 
 /// Prevents any commands submitted afterward to this queue from executing
@@ -406,16 +412,22 @@ bool queue::device_has(aspect Aspect) const {
 // TODO(#15184) Remove this function in the next ABI-breaking window.
 bool queue::ext_codeplay_supports_fusion() const { return false; }
 
-event queue::ext_oneapi_get_last_event() const {
+sycl::detail::optional<event> queue::ext_oneapi_get_last_event_impl() const {
   if (!is_in_order())
     throw sycl::exception(
         make_error_code(errc::invalid),
         "ext_oneapi_get_last_event() can only be called on in-order queues.");
 
-  event LastEvent = impl->getLastEvent();
+  sycl::detail::optional<event> LastEvent = impl->getLastEvent();
+
+  // If there was no last event, the queue is yet to have any work submitted and
+  // we return a std::nullopt.
+  if (!LastEvent)
+    return std::nullopt;
+
   // If the last event was discarded or a NOP, we insert a marker to represent
   // an event at end.
-  auto LastEventImpl = detail::getSyclObjImpl(LastEvent);
+  auto LastEventImpl = detail::getSyclObjImpl(*LastEvent);
   if (LastEventImpl->isDiscarded() || LastEventImpl->isNOP())
     LastEvent =
         detail::createSyclObjFromImpl<event>(impl->insertMarkerEvent(impl));

sycl/test-e2e/InOrderEventsExt/get_last_event.cpp

@@ -21,8 +21,18 @@
 
 template <typename F>
 int Check(const sycl::queue &Q, const char *CheckName, const F &CheckFunc) {
-  sycl::event E = CheckFunc();
-  if (E != Q.ext_oneapi_get_last_event()) {
+  std::optional<sycl::event> E = CheckFunc();
+  if (!E) {
+    std::cout << "No result event return by CheckFunc()" << std::endl;
+    return 1;
+  }
+  std::optional<sycl::event> LastEvent = Q.ext_oneapi_get_last_event();
+  if (!LastEvent) {
+    std::cout << "No result event return by ext_oneapi_get_last_event()"
+              << std::endl;
+    return 1;
+  }
+  if (*E != *LastEvent) {
     std::cout << "Failed " << CheckName << std::endl;
     return 1;
   }
@@ -34,12 +44,17 @@ int main() {
 
   int Failed = 0;
 
-  // Check that a valid event is returned on the empty queue.
-  Q.ext_oneapi_get_last_event().wait();
+  // Check that a std::nullopt is returned on the empty queue.
+  std::optional<sycl::event> EmptyEvent = Q.ext_oneapi_get_last_event();
+  if (EmptyEvent.has_value()) {
+    std::cout << "Unexpected event return by ext_oneapi_get_last_event()"
+              << std::endl;
+    ++Failed;
+  }
 
   // Check that a valid event is returned after enqueuing work without events.
   sycl::ext::oneapi::experimental::single_task(Q, []() {});
-  Q.ext_oneapi_get_last_event().wait();
+  Q.ext_oneapi_get_last_event()->wait();
 
   // Check event equivalences - This is an implementation detail, but useful
   // for checking behavior.

sycl/test/abi/sycl_symbols_linux.dump

@@ -3660,8 +3660,8 @@ _ZNK4sycl3_V15queue16get_backend_infoINS0_4info6device7versionEEENS0_6detail20is
 _ZNK4sycl3_V15queue16get_backend_infoINS0_4info8platform7versionEEENS0_6detail20is_backend_info_descIT_E11return_typeEv
 _ZNK4sycl3_V15queue20ext_oneapi_get_graphEv
 _ZNK4sycl3_V15queue20ext_oneapi_get_stateEv
-_ZNK4sycl3_V15queue25ext_oneapi_get_last_eventEv
 _ZNK4sycl3_V15queue28ext_codeplay_supports_fusionEv
+_ZNK4sycl3_V15queue30ext_oneapi_get_last_event_implEv
 _ZNK4sycl3_V15queue3getEv
 _ZNK4sycl3_V15queue8get_infoINS0_4info5queue15reference_countEEENS0_6detail18is_queue_info_descIT_E11return_typeEv
 _ZNK4sycl3_V15queue8get_infoINS0_4info5queue6deviceEEENS0_6detail18is_queue_info_descIT_E11return_typeEv

sycl/test/abi/sycl_symbols_windows.dump

@@ -3847,7 +3847,8 @@
 ?ext_oneapi_get_graph@queue@_V1@sycl@@QEBA?AV?$command_graph@$0A@@experimental@oneapi@ext@23@XZ
 ?ext_oneapi_get_kernel@kernel_bundle_plain@detail@_V1@sycl@@AEAA?AVkernel@34@Vstring_view@234@@Z
 ?ext_oneapi_get_kernel@kernel_bundle_plain@detail@_V1@sycl@@QEAA?AVkernel@34@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Z
-?ext_oneapi_get_last_event@queue@_V1@sycl@@QEBA?AVevent@23@XZ
+?ext_oneapi_get_last_event_impl@queue@_V1@sycl@@AEBA?AV?$optional@Vevent@_V1@sycl@@@detail@23@XZ
+?ext_oneapi_get_last_event@queue@_V1@sycl@@QEBA?AV?$optional@Vevent@_V1@sycl@@@std@@XZ
 ?ext_oneapi_get_state@queue@_V1@sycl@@QEBA?AW4queue_state@experimental@oneapi@ext@23@XZ
 ?ext_oneapi_graph@handler@_V1@sycl@@QEAAXV?$command_graph@$00@experimental@oneapi@ext@23@@Z
 ?ext_oneapi_graph@queue@_V1@sycl@@QEAA?AVevent@23@V?$command_graph@$00@experimental@oneapi@ext@23@AEBUcode_location@detail@23@@Z

sycl/unittests/Extensions/EnqueueFunctionsEvents.cpp

@@ -111,8 +111,9 @@ class EnqueueFunctionsEventsTests : public ::testing::Test {
 
 inline void CheckLastEventDiscarded(sycl::queue &Q) {
   auto QueueImplPtr = sycl::detail::getSyclObjImpl(Q);
-  event LastEvent = QueueImplPtr->getLastEvent();
-  auto LastEventImplPtr = sycl::detail::getSyclObjImpl(LastEvent);
+  sycl::detail::optional<event> LastEvent = QueueImplPtr->getLastEvent();
+  ASSERT_TRUE(LastEvent.has_value());
+  auto LastEventImplPtr = sycl::detail::getSyclObjImpl(*LastEvent);
   ASSERT_TRUE(LastEventImplPtr->isDiscarded());
 }
 

sycl/unittests/Extensions/GetLastEvent.cpp

@@ -29,17 +29,9 @@ TEST(GetLastEventEmptyQueue, CheckEmptyQueueLastEvent) {
   unittest::UrMock<> Mock;
   platform Plt = sycl::platform();
 
-  MarkerEventLatest = nullptr;
-  mock::getCallbacks().set_after_callback("urEnqueueEventsWait",
-                                          &redefinedEnqueueEventsWaitAfter);
-  mock::getCallbacks().set_before_callback("urEventRelease",
-                                           &redefinedEventRelease);
-
   queue Q{property::queue::in_order{}};
-  event E = Q.ext_oneapi_get_last_event();
-  ur_event_handle_t UREvent = detail::getSyclObjImpl(E)->getHandle();
-  ASSERT_NE(MarkerEventLatest, ur_event_handle_t{nullptr});
-  ASSERT_EQ(UREvent, MarkerEventLatest);
+  std::optional<event> E = Q.ext_oneapi_get_last_event();
+  ASSERT_FALSE(E.has_value());
 }
 
 TEST(GetLastEventEmptyQueue, CheckEventlessWorkQueue) {
@@ -57,8 +49,9 @@ TEST(GetLastEventEmptyQueue, CheckEventlessWorkQueue) {
   // The following single_task does not return an event, so it is expected that
   // the last event query creates a new marker event.
   sycl::ext::oneapi::experimental::single_task<TestKernel<>>(Q, []() {});
-  event E = Q.ext_oneapi_get_last_event();
-  ur_event_handle_t UREvent = detail::getSyclObjImpl(E)->getHandle();
+  std::optional<event> E = Q.ext_oneapi_get_last_event();
+  ASSERT_TRUE(E.has_value());
+  ur_event_handle_t UREvent = detail::getSyclObjImpl(*E)->getHandle();
   ASSERT_NE(MarkerEventLatest, ur_event_handle_t{nullptr});
   ASSERT_EQ(UREvent, MarkerEventLatest);
 }