Skip to content

[DO NOT MERGE] simpler investigation of event_impl destruct issue #18690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 8 commits into
base: sycl
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions sycl/include/sycl/event.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@
#include <variant> // for hash
#include <vector> // for vector

#ifdef _WIN32
#include <intrin.h>
#endif
// also defined in event_imp.hpp. probably need to move it elsewhere
//#define CP_LOG_EVENT_LIFECYCLE 1

namespace sycl {
inline namespace _V1 {
// Forward declaration
Expand Down Expand Up @@ -58,13 +64,51 @@ class __SYCL_EXPORT event : public detail::OwnerLessBase<event> {
event(cl_event ClEvent, const context &SyclContext);
#endif

#ifdef CP_LOG_EVENT_LIFECYCLE
// Copy Constructor // event(const event &rhs) = default;
event(const event &rhs) : impl(rhs.impl) { // Calls std::shared_ptr's copy constructor
std::cout << "EVENT: Copy Constructor (of " << this << ") from " << &rhs << " - new impl: " << impl.get() << " (use_count: " << impl.use_count() << ")" << std::endl;
}

// Move Constructor // event(event &&rhs) = default;
event(event &&rhs) noexcept : impl(std::move(rhs.impl)) { // Calls std::shared_ptr's move constructor
std::cout << "EVENT: Move Constructor (of " << this << ") from " << &rhs << " - new impl: " << impl.get() << " (use_count: " << impl.use_count() << ")" << std::endl;
}

// Copy Assignment Operator //event &operator=(const event &rhs) = default;
event &operator=(const event &rhs) {
if (this != &rhs) {
impl = rhs.impl; // Calls std::shared_ptr's copy assignment operator
}
std::cout << "EVENT: Copy Assignment (of " << this << ") from " << &rhs << " - new impl: " << impl.get() << " (use_count: " << impl.use_count() << ")" << std::endl;
return *this;
}

// Move Assignment Operator // event &operator=(event &&rhs) = default;
event &operator=(event &&rhs) noexcept {
if (this != &rhs) {
impl = std::move(rhs.impl); // Calls std::shared_ptr's move assignment operator
}
std::cout << "EVENT: Move Assignment (of " << this << ") from " << &rhs << " - new impl: " << impl.get() << " (use_count: " << impl.use_count() << ")" << std::endl;
return *this;
}

// Destructor
~event() {
std::cout << "EVENT: Destructor (of " << this << ") - impl: " << impl.get() << " (use_count: " << impl.use_count() << ")" << std::endl;
}

#else
event(const event &rhs) = default;

event(event &&rhs) = default;

event &operator=(const event &rhs) = default;

event &operator=(event &&rhs) = default;

~event() = default; // CP
#endif

bool operator==(const event &rhs) const;

Expand Down
3 changes: 3 additions & 0 deletions sycl/source/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
#2. Use AddLLVM to modify the build and access config options
#cmake_policy(SET CMP0057 NEW)
#include(AddLLVM)

set(CMAKE_BUILD_TYPE Debug)

include(CheckLinkerFlag)
configure_file(
${CMAKE_CURRENT_SOURCE_DIR}/version.rc.in
Expand Down
74 changes: 71 additions & 3 deletions sycl/source/detail/event_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@
#include <sstream>
#endif





namespace sycl {
inline namespace _V1 {
namespace detail {
Expand All @@ -43,17 +47,66 @@ void event_impl::initContextIfNeeded() {
}

event_impl::~event_impl() {
auto Handle = this->getHandle();

#ifdef CP_LOG_EVENT_LIFECYCLE
std::cout << "~event_impl destructor of (" << this
<< ") event_impl.cpp:54 UREvent: " << Handle << std::endl;
#endif
#ifdef CP_LOG_EARLY_RELEASE
if (MHasBeenReleased == 0xDEADBEEF)
std::cout << "~event_impl MHasBeenReleased is already set to 0xDEADBEEF this: "
<< this << " UREvent: " << Handle
<< " USMMemcopyCalled: " << sycl::detail::USMMemcopyCalled
<< std::endl;
else if(MHasBeenReleased != 0)
std::cout << "~event_impl MHasBeenReleased corrupted? " << std::hex << MHasBeenReleased << std::endl;
#endif

try {
auto Handle = this->getHandle();
if (Handle)


if (Handle){
#ifdef CP_LOG_EARLY_RELEASE
// CP abuse
// turn on when calling memcpy, off when releasing an event with a URHandle.
// check
if (!sycl::detail::USMMemcopyCalled) {
// put breakpoint here:
std::cout << "GOTCHA!! ~event_impl this: (" << this
<< ") UREvent: " << std::hex << Handle
<< " USMMemcopyCalled: " << sycl::detail::USMMemcopyCalled
<< std::endl;
}
sycl::detail::USMMemcopyCalled = false; // toggle it off
#endif
getAdapter()->call<UrApiKind::urEventRelease>(Handle);

}
MHasBeenReleased = 0xDEADBEEF;
} catch (std::exception &e) {
__SYCL_REPORT_EXCEPTION_TO_STREAM("exception in ~event_impl", e);
}
}


void event_impl::waitInternal(bool *Success) {
auto Handle = this->getHandle();

#ifdef CP_LOG_EARLY_RELEASE
// CP -- this does not trip
if(MHasBeenReleased == 0xDEADBEEF){
std::cout << "waitInternal HasBeenReleased is already set to "
"0xDEADBEEF. this: "
<< this << " UREvent: " << std::hex << Handle
<< " USMMemcopyCalled: " << sycl::detail::USMMemcopyCalled
<< std::endl;
}else if(MHasBeenReleased != 0){
std::cout << "waitInternal MHasBeenReleased corrupted? " << std::hex << MHasBeenReleased << std::endl;
}
#endif


if (!MIsHostEvent && Handle) {
// Wait for the native event
ur_result_t Err =
Expand Down Expand Up @@ -145,7 +198,9 @@ void event_impl::setContextImpl(const ContextImplPtr &Context) {
event_impl::event_impl(ur_event_handle_t Event, const context &SyclContext)
: MEvent(Event), MContext(detail::getSyclObjImpl(SyclContext)),
MIsFlushed(true), MState(HES_Complete) {

#ifdef CP_LOG_EVENT_LIFECYCLE
std::cout << "event_impl constructor. of (" << this << ") event_impl.cpp:178" << std::endl;
#endif
ur_context_handle_t TempContext;
getAdapter()->call<UrApiKind::urEventGetInfo>(
this->getHandle(), UR_EVENT_INFO_CONTEXT, sizeof(ur_context_handle_t),
Expand All @@ -161,6 +216,9 @@ event_impl::event_impl(ur_event_handle_t Event, const context &SyclContext)

event_impl::event_impl(const QueueImplPtr &Queue)
: MQueue{Queue}, MIsProfilingEnabled{!Queue || Queue->MIsProfilingEnabled} {
#ifdef CP_LOG_EVENT_LIFECYCLE
std::cout << "event_impl constructor from QueImplPtr of (" << this << ") event_impl.cpp:197" << std::endl;
#endif
if (Queue)
this->setContextImpl(Queue->getContextImplPtr());
else {
Expand Down Expand Up @@ -246,6 +304,16 @@ void event_impl::instrumentationEpilog(void *TelemetryEvent,

void event_impl::wait(std::shared_ptr<sycl::detail::event_impl> Self,
bool *Success) {
#ifdef CP_LOG_EARLY_RELEASE
// CP -- this trips
if(MHasBeenReleased == 0xDEADBEEF) {
std::cout << "wait HasBeenRelease already set to 0xDEADBEEF. this: " << this << std::endl;
//__debugbreak();
}else if(MHasBeenReleased != 0){
std::cout << "wait MHasBeenReleased corrupted? " << std::hex << MHasBeenReleased << std::endl;
}
#endif

if (MState == HES_Discarded)
throw sycl::exception(make_error_code(errc::invalid),
"wait method cannot be used for a discarded event.");
Expand Down
31 changes: 30 additions & 1 deletion sycl/source/detail/event_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,20 @@
#include <condition_variable>
#include <optional>


// CP -- enables logging of "early release" and also global var to track memcpy :: ~event_impl tracking
#define CP_LOG_EARLY_RELEASE 1

#ifdef _WIN32
#include <intrin.h>
#else
// CP no op
void __debugbreak() { }
#endif
// also in event.hpp. Probably needs to be moved elsewhere
//#define CP_LOG_EVENT_LIFECYCLE 1


namespace sycl {
inline namespace _V1 {
namespace ext::oneapi::experimental::detail {
Expand All @@ -35,6 +49,11 @@ using QueueImplPtr = std::shared_ptr<sycl::detail::queue_impl>;
class event_impl;
using EventImplPtr = std::shared_ptr<sycl::detail::event_impl>;

#ifdef CP_LOG_EARLY_RELEASE
// CP Adding a global
bool USMMemcopyCalled = false;
#endif

class event_impl {
public:
enum HostEventState : int {
Expand All @@ -56,6 +75,9 @@ class event_impl {
// event methods. This ::get() call uses static vars to read and parse the
// ODS env var exactly once.
SYCLConfig<ONEAPI_DEVICE_SELECTOR>::get();
#ifdef CP_LOG_EVENT_LIFECYCLE
std::cout << "event_impl ready constructor of (" << this << ") event_impl.hpp:53" << std::endl;
#endif
}

/// Constructs an event instance from a UR event handle.
Expand All @@ -67,6 +89,12 @@ class event_impl {
/// \param SyclContext is an instance of SYCL context.
event_impl(ur_event_handle_t Event, const context &SyclContext);
event_impl(const QueueImplPtr &Queue);

// delete the copy constructors and assignment operators
event_impl(const event_impl&) = delete;
event_impl &operator=(const event_impl&) = delete;
event_impl(event_impl&&) = delete;
event_impl &operator=(event_impl&&) = delete;

/// Sets a queue associated with the event
///
Expand Down Expand Up @@ -347,7 +375,7 @@ class event_impl {
// queue and command, as well as the fact that it is not in enqueued state.
return MEvent && MQueue.expired() && !MIsEnqueued && !MCommand;
}

uint32_t MHasBeenReleased = 0;
protected:
// When instrumentation is enabled emits trace event for event wait begin and
// returns the telemetry event generated for the wait
Expand Down Expand Up @@ -433,6 +461,7 @@ class event_impl {
// MEvent is lazily created in first ur handle query.
bool MIsDefaultConstructed = false;
bool MIsHostEvent = false;

};

} // namespace detail
Expand Down
5 changes: 5 additions & 0 deletions sycl/source/detail/memory_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,11 @@ void MemoryManager::copy_usm(const void *SrcMem, QueueImplPtr SrcQueue,
"NULL pointer argument in memory copy operation.");

const AdapterPtr &Adapter = SrcQueue->getAdapter();
// CP
#ifdef CP_LOG_EARLY_RELEASE
sycl::detail::USMMemcopyCalled = true; // turn on when calling memcpy, off when releasing. Check if imbalance.
#endif

Adapter->call<UrApiKind::urEnqueueUSMMemcpy>(SrcQueue->getHandleRef(),
/* blocking */ false, DstMem,
SrcMem, Len, DepEvents.size(),
Expand Down
19 changes: 17 additions & 2 deletions sycl/source/event.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,25 @@
#include <memory>
#include <unordered_set>

#ifdef _WIN32
#include <intrin.h>
#endif

namespace sycl {
inline namespace _V1 {

event::event() : impl(std::make_shared<detail::event_impl>(std::nullopt)) {}
event::event() : impl(std::make_shared<detail::event_impl>(std::nullopt)) {
#ifdef CP_LOG_EVENT_LIFECYCLE
std::cout << "EVENT() constructor (of " << this << ") impl: " << impl.get() << " (use_count: " << impl.use_count() << ") event.cpp:25" << std::endl;
#endif
}

event::event(cl_event ClEvent, const context &SyclContext)
: impl(std::make_shared<detail::event_impl>(
detail::ur::cast<ur_event_handle_t>(ClEvent), SyclContext)) {
#ifdef CP_LOG_EVENT_LIFECYCLE
std::cout << "EVENT(ClEvent, Context) constructor. event.cpp:31" << std::endl;
#endif
// This is a special interop constructor for OpenCL, so the event must be
// retained.
__SYCL_OCL_CALL(clRetainEvent, ClEvent);
Expand Down Expand Up @@ -62,7 +73,11 @@ std::vector<event> event::get_wait_list() {
}

event::event(std::shared_ptr<detail::event_impl> event_impl)
: impl(std::move(event_impl)) {}
: impl(std::move(event_impl)) {
#ifdef CP_LOG_EVENT_LIFECYCLE
std::cout << "EVENT(impl) constructor of (" << this << ") impl: " << impl.get() << " (use_count: " << impl.use_count() << ") event.cpp:71" << std::endl;
#endif
}

template <typename Param>
typename detail::is_event_info_desc<Param>::return_type
Expand Down
2 changes: 2 additions & 0 deletions unified-runtime/source/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
# See LICENSE.TXT
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

set(CMAKE_BUILD_TYPE Debug)

add_definitions(-DUR_VERSION="${PROJECT_VERSION_MAJOR}")
add_definitions(-DUR_VALIDATION_LAYER_SUPPORTED_VERSION="${PROJECT_VERSION_MAJOR}")

Expand Down
1 change: 1 addition & 0 deletions unified-runtime/source/adapters/level_zero/common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,7 @@ getZeStructureType<ze_intel_device_block_array_exp_properties_t>() {
thread_local int32_t ErrorMessageCode = 0;
thread_local char ErrorMessage[MaxMessageSize]{};
thread_local int32_t ErrorAdapterNativeCode;
bool ReleaseTracker = true;

// Utility function for setting a message and warning
[[maybe_unused]] void setErrorMessage(const char *pMessage, int32_t ErrorCode,
Expand Down
2 changes: 2 additions & 0 deletions unified-runtime/source/adapters/level_zero/common.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -381,3 +381,5 @@ extern thread_local int32_t ErrorAdapterNativeCode;
// Utility function for setting a message and warning
[[maybe_unused]] void setErrorMessage(const char *pMessage, int32_t ErrorCode,
int32_t AdapterErrorCode);

extern bool ReleaseTracker;
10 changes: 8 additions & 2 deletions unified-runtime/source/adapters/level_zero/event.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,7 @@ urEventWait(uint32_t NumEvents,
//
ur_event_handle_t_ *Event = ur_cast<ur_event_handle_t_ *>(e);
if (!Event->hasExternalRefs())
die("urEventWait must not be called for an internal event");
die("FIRST urEventWait must not be called for an internal event");

ze_event_handle_t ZeHostVisibleEvent;
if (auto Res = Event->getOrCreateHostVisibleEvent(ZeHostVisibleEvent))
Expand All @@ -819,7 +819,7 @@ urEventWait(uint32_t NumEvents,
{
std::shared_lock<ur_shared_mutex> EventLock(Event->Mutex);
if (!Event->hasExternalRefs())
die("urEventWait must not be called for an internal event");
die("SECOND urEventWait must not be called for an internal event");

if (!Event->Completed) {
auto HostVisibleEvent = Event->HostVisibleEvent;
Expand Down Expand Up @@ -882,6 +882,12 @@ urEventRetain(/** [in] handle of the event object */ ur_event_handle_t Event) {
ur_result_t

urEventRelease(/** [in] handle of the event object */ ur_event_handle_t Event) {
// The releaseTracker is not a per-class var, just a global
//if (ReleaseTracker) {
//std::cout << "Double release call?" << std::endl; // changes timing.
//__debugbreak();
//}
ReleaseTracker = true;
Event->RefCountExternal--;
bool isEventsWaitCompleted =
(Event->CommandType == UR_COMMAND_EVENTS_WAIT ||
Expand Down