From 39e0bf9ad9de77d0c4b533762cefd584c2f817a3 Mon Sep 17 00:00:00 2001 From: Ivan Karachun Date: Fri, 20 Mar 2020 16:19:25 +0300 Subject: [PATCH 1/2] [SYCL] Don't throw exceptions from destructors SYCL RT can throw exceptions in case of enqueue process failure. Some commands are scheduled while destructor call. So possible exceptions should be handlen by destructor since throwing exceptions out of a destructor is undefined behaviour. Signed-off-by: Ivan Karachun --- sycl/include/CL/sycl/detail/buffer_impl.hpp | 7 ++- sycl/include/CL/sycl/detail/image_impl.hpp | 7 ++- sycl/source/detail/accessor_impl.cpp | 7 ++- sycl/source/detail/scheduler/commands.cpp | 21 ++++--- sycl/source/detail/scheduler/commands.hpp | 18 ++++-- .../source/detail/scheduler/graph_builder.cpp | 2 +- sycl/source/detail/scheduler/scheduler.cpp | 2 +- sycl/test/scheduler/HandleException.cpp | 50 +++++++++++++++ sycl/unittests/scheduler/BlockedCommands.cpp | 5 +- sycl/unittests/scheduler/CMakeLists.txt | 1 + sycl/unittests/scheduler/FailedCommands.cpp | 61 +++++++++++++++++++ 11 files changed, 160 insertions(+), 21 deletions(-) create mode 100644 sycl/test/scheduler/HandleException.cpp create mode 100644 sycl/unittests/scheduler/FailedCommands.cpp diff --git a/sycl/include/CL/sycl/detail/buffer_impl.hpp b/sycl/include/CL/sycl/detail/buffer_impl.hpp index 3a59254e31f14..54e0c2e5ccb76 100644 --- a/sycl/include/CL/sycl/detail/buffer_impl.hpp +++ b/sycl/include/CL/sycl/detail/buffer_impl.hpp @@ -108,7 +108,12 @@ class buffer_impl final : public SYCLMemObjT { MemObjType getType() const override { return MemObjType::BUFFER; } - ~buffer_impl() { BaseT::updateHostMemory(); } + ~buffer_impl() { + try { + BaseT::updateHostMemory(); + } catch (...) { + } + } }; } // namespace detail diff --git a/sycl/include/CL/sycl/detail/image_impl.hpp b/sycl/include/CL/sycl/detail/image_impl.hpp index d6a28ac26063f..285227c2d43e7 100644 --- a/sycl/include/CL/sycl/detail/image_impl.hpp +++ b/sycl/include/CL/sycl/detail/image_impl.hpp @@ -216,7 +216,12 @@ template class image_impl final : public SYCLMemObjT { size_t getSlicePitch() const { return MSlicePitch; } - ~image_impl() { BaseT::updateHostMemory(); } + ~image_impl() { + try { + BaseT::updateHostMemory(); + } catch (...) { + } + } private: vector_class getDevices(const ContextImplPtr Context); diff --git a/sycl/source/detail/accessor_impl.cpp b/sycl/source/detail/accessor_impl.cpp index bb8931392cf7e..96a7657bb27a5 100644 --- a/sycl/source/detail/accessor_impl.cpp +++ b/sycl/source/detail/accessor_impl.cpp @@ -15,8 +15,11 @@ namespace sycl { namespace detail { AccessorImplHost::~AccessorImplHost() { - if (MBlockedCmd) - detail::Scheduler::getInstance().releaseHostAccessor(this); + try { + if (MBlockedCmd) + detail::Scheduler::getInstance().releaseHostAccessor(this); + } catch (...) { + } } void addHostAccessorAndWait(Requirement *Req) { diff --git a/sycl/source/detail/scheduler/commands.cpp b/sycl/source/detail/scheduler/commands.cpp index 288081b14d9e8..cdae555c6452a 100644 --- a/sycl/source/detail/scheduler/commands.cpp +++ b/sycl/source/detail/scheduler/commands.cpp @@ -228,10 +228,11 @@ void Command::waitForEvents(QueueImplPtr Queue, } Command::Command(CommandType Type, QueueImplPtr Queue) - : MQueue(std::move(Queue)), MType(Type), MEnqueued(false) { + : MQueue(std::move(Queue)), MType(Type) { MEvent.reset(new detail::event_impl(MQueue)); MEvent->setCommand(this); MEvent->setContextImpl(detail::getSyclObjImpl(MQueue->get_context())); + MEnqueueStatus = EnqueueResultT::SyclEnqueueReady; #ifdef XPTI_ENABLE_INSTRUMENTATION if (!xptiTraceEnabled()) @@ -451,11 +452,11 @@ void Command::emitInstrumentation(uint16_t Type, const char *Txt) { bool Command::enqueue(EnqueueResultT &EnqueueResult, BlockingT Blocking) { // Exit if already enqueued - if (MEnqueued) + if (MEnqueueStatus == EnqueueResultT::SyclEnqueueSuccess) return true; // If the command is blocked from enqueueing - if (MIsBlockable && !MCanEnqueue) { + if (MIsBlockable && MEnqueueStatus == EnqueueResultT::SyclEnqueueBlocked) { // Exit if enqueue type is not blocking if (!Blocking) { EnqueueResult = EnqueueResultT(EnqueueResultT::SyclEnqueueBlocked, this); @@ -478,7 +479,7 @@ bool Command::enqueue(EnqueueResultT &EnqueueResult, BlockingT Blocking) { #endif // Wait if blocking - while (!MCanEnqueue) + while (MEnqueueStatus == EnqueueResultT::SyclEnqueueBlocked) ; #ifdef XPTI_ENABLE_INSTRUMENTATION emitInstrumentation(xpti::trace_barrier_end, Info.c_str()); @@ -488,13 +489,19 @@ bool Command::enqueue(EnqueueResultT &EnqueueResult, BlockingT Blocking) { std::lock_guard Lock(MEnqueueMtx); // Exit if the command is already enqueued - if (MEnqueued) + if (MEnqueueStatus == EnqueueResultT::SyclEnqueueSuccess) return true; #ifdef XPTI_ENABLE_INSTRUMENTATION emitInstrumentation(xpti::trace_task_begin, nullptr); #endif + if (MEnqueueStatus == EnqueueResultT::SyclEnqueueFailed) { + EnqueueResult = EnqueueResultT(EnqueueResultT::SyclEnqueueFailed, this); + return false; + } + + MEnqueueStatus = EnqueueResultT::SyclEnqueueFailed; cl_int Res = enqueueImp(); if (CL_SUCCESS != Res) @@ -503,14 +510,14 @@ bool Command::enqueue(EnqueueResultT &EnqueueResult, BlockingT Blocking) { else // Consider the command is successfully enqueued if return code is // CL_SUCCESS - MEnqueued = true; + MEnqueueStatus = EnqueueResultT::SyclEnqueueSuccess; // Emit this correlation signal before the task end emitEnqueuedEventSignal(MEvent->getHandleRef()); #ifdef XPTI_ENABLE_INSTRUMENTATION emitInstrumentation(xpti::trace_task_end, nullptr); #endif - return static_cast(MEnqueued); + return MEnqueueStatus == EnqueueResultT::SyclEnqueueSuccess; } void Command::resolveReleaseDependencies(std::set &DepList) { diff --git a/sycl/source/detail/scheduler/commands.hpp b/sycl/source/detail/scheduler/commands.hpp index 13763cc77a8d3..2a53aad6c9c4a 100644 --- a/sycl/source/detail/scheduler/commands.hpp +++ b/sycl/source/detail/scheduler/commands.hpp @@ -40,7 +40,12 @@ enum BlockingT { NON_BLOCKING = 0, BLOCKING }; // The struct represents the result of command enqueueing struct EnqueueResultT { - enum ResultT { SyclEnqueueSuccess, SyclEnqueueBlocked, SyclEnqueueFailed }; + enum ResultT { + SyclEnqueueReady, + SyclEnqueueSuccess, + SyclEnqueueBlocked, + SyclEnqueueFailed + }; EnqueueResultT(ResultT Result = SyclEnqueueSuccess, Command *Cmd = nullptr, cl_int ErrCode = CL_SUCCESS) : MResult(Result), MCmd(Cmd), MErrCode(ErrCode) {} @@ -110,7 +115,9 @@ class Command { bool isFinished(); - bool isEnqueued() const { return MEnqueued; } + bool isEnqueued() const { + return MEnqueueStatus == EnqueueResultT::SyclEnqueueSuccess; + } std::shared_ptr getQueue() const { return MQueue; } @@ -170,8 +177,6 @@ class Command { // The type of the command CommandType MType; - // Indicates whether the command is enqueued or not - std::atomic MEnqueued; // Mutex used to protect enqueueing from race conditions std::mutex MEnqueueMtx; @@ -182,13 +187,14 @@ class Command { std::unordered_set MUsers; // Indicates whether the command can be blocked from enqueueing bool MIsBlockable = false; - // Indicates whether the command is blocked from enqueueing - std::atomic MCanEnqueue; // Counts the number of memory objects this command is a leaf for unsigned MLeafCounter = 0; const char *MBlockReason = "Unknown"; + // Describes the status of a command + std::atomic MEnqueueStatus; + // All member variable defined here are needed for the SYCL instrumentation // layer. Do not guard these variables below with XPTI_ENABLE_INSTRUMENTATION // to ensure we have the same object layout when the macro in the library and diff --git a/sycl/source/detail/scheduler/graph_builder.cpp b/sycl/source/detail/scheduler/graph_builder.cpp index 146b52e169f35..887f2c3f95a11 100644 --- a/sycl/source/detail/scheduler/graph_builder.cpp +++ b/sycl/source/detail/scheduler/graph_builder.cpp @@ -362,7 +362,7 @@ Command *Scheduler::GraphBuilder::addHostAccessor(Requirement *Req, UpdateHostAccCmd->addUser(EmptyCmd); EmptyCmd->MIsBlockable = true; - EmptyCmd->MCanEnqueue = false; + EmptyCmd->MEnqueueStatus = EnqueueResultT::SyclEnqueueBlocked; EmptyCmd->MBlockReason = "A Buffer is locked by the host accessor"; updateLeaves({UpdateHostAccCmd}, Record, Req->MAccessMode); diff --git a/sycl/source/detail/scheduler/scheduler.cpp b/sycl/source/detail/scheduler/scheduler.cpp index 9a5f02fab02c8..69e2507765015 100644 --- a/sycl/source/detail/scheduler/scheduler.cpp +++ b/sycl/source/detail/scheduler/scheduler.cpp @@ -177,7 +177,7 @@ EventImplPtr Scheduler::addHostAccessor(Requirement *Req, } void Scheduler::releaseHostAccessor(Requirement *Req) { - Req->MBlockedCmd->MCanEnqueue = true; + Req->MBlockedCmd->MEnqueueStatus = EnqueueResultT::SyclEnqueueReady; MemObjRecord* Record = Req->MSYCLMemObj->MRecord.get(); auto EnqueueLeaves = [](CircularBuffer &Leaves) { for (Command *Cmd : Leaves) { diff --git a/sycl/test/scheduler/HandleException.cpp b/sycl/test/scheduler/HandleException.cpp new file mode 100644 index 0000000000000..0823380c3d587 --- /dev/null +++ b/sycl/test/scheduler/HandleException.cpp @@ -0,0 +1,50 @@ +// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple -I %sycl_source_dir %s -o %t.out +// RUN: %CPU_RUN_PLACEHOLDER %t.out +#include +#include + +using namespace cl::sycl; + +constexpr access::mode sycl_read = access::mode::read; +constexpr access::mode sycl_write = access::mode::write; + +constexpr unsigned MAX_WG_SIZE = 4; +constexpr unsigned SIZE = 5; +using ArrayType = std::array; + +class kernelCompute; + +// Return 'true' if an exception was thrown. +bool run_kernel(const uint wg_size) { + ArrayType index; + const unsigned N = index.size(); + { + buffer bufferIdx(index.data(), N); + queue deviceQueue; + try { + deviceQueue.submit([&](handler &cgh) { + auto accessorIdx = bufferIdx.get_access(cgh); + cgh.parallel_for( + nd_range<1>(range<1>(N), range<1>(wg_size)), + [=](nd_item<1> ID) [[cl::reqd_work_group_size(1, 1, MAX_WG_SIZE)]] { + (void)accessorIdx[ID.get_global_id(0)]; + }); + }); + } catch (nd_range_error &err) { + return true; + } catch (...) { + assert(!"Unknown exception was thrown"); + } + } + return false; +} + +int main() { + bool success_exception = run_kernel(MAX_WG_SIZE); + assert(!success_exception && + "Unexpected exception was thrown for success call"); + bool fail_exception = run_kernel(SIZE); + assert(fail_exception && "No exception was thrown"); + + return 0; +} diff --git a/sycl/unittests/scheduler/BlockedCommands.cpp b/sycl/unittests/scheduler/BlockedCommands.cpp index df93a2b5f9dcd..4854924dc207c 100644 --- a/sycl/unittests/scheduler/BlockedCommands.cpp +++ b/sycl/unittests/scheduler/BlockedCommands.cpp @@ -41,8 +41,8 @@ class TestScheduler : public detail::Scheduler { TEST_F(SchedulerTest, BlockedCommands) { MockCommand MockCmd(detail::getSyclObjImpl(MQueue)); + MockCmd.MEnqueueStatus = detail::EnqueueResultT::SyclEnqueueBlocked; MockCmd.MIsBlockable = true; - MockCmd.MCanEnqueue = false; MockCmd.MRetVal = CL_DEVICE_PARTITION_EQUALLY; detail::EnqueueResultT Res; @@ -52,7 +52,7 @@ TEST_F(SchedulerTest, BlockedCommands) { ASSERT_EQ(detail::EnqueueResultT::SyclEnqueueBlocked, Res.MResult) << "Result of enqueueing blocked command should be BLOCKED\n"; - MockCmd.MCanEnqueue = true; + MockCmd.MEnqueueStatus = detail::EnqueueResultT::SyclEnqueueReady; Res.MResult = detail::EnqueueResultT::SyclEnqueueSuccess; MockCmd.MRetVal = CL_DEVICE_PARTITION_EQUALLY; @@ -65,6 +65,7 @@ TEST_F(SchedulerTest, BlockedCommands) { ASSERT_EQ(&MockCmd, Res.MCmd) << "Expected different failed command.\n"; Res = detail::EnqueueResultT{}; + MockCmd.MEnqueueStatus = detail::EnqueueResultT::SyclEnqueueReady; MockCmd.MRetVal = CL_SUCCESS; Enqueued = TestScheduler::enqueueCommand(&MockCmd, Res, detail::BLOCKING); ASSERT_TRUE(Enqueued && diff --git a/sycl/unittests/scheduler/CMakeLists.txt b/sycl/unittests/scheduler/CMakeLists.txt index 3c630071a2398..3a3edfa68c05c 100644 --- a/sycl/unittests/scheduler/CMakeLists.txt +++ b/sycl/unittests/scheduler/CMakeLists.txt @@ -8,6 +8,7 @@ set(CMAKE_CXX_COMPILER ${clang}) add_sycl_unittest(SchedulerTests BlockedCommands.cpp + FailedCommands.cpp FinishedCmdCleanup.cpp LeafLimit.cpp MemObjCommandCleanup.cpp diff --git a/sycl/unittests/scheduler/FailedCommands.cpp b/sycl/unittests/scheduler/FailedCommands.cpp new file mode 100644 index 0000000000000..7b2e85d8bc8e5 --- /dev/null +++ b/sycl/unittests/scheduler/FailedCommands.cpp @@ -0,0 +1,61 @@ +//==----------- FailedCommands.cpp ---- Scheduler unit tests ---------------==// +// +// 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 "SchedulerTest.hpp" + +#include +#include +#include + +#include + +using namespace cl::sycl; + +class MockCommand : public detail::Command { +public: + MockCommand(detail::QueueImplPtr Queue) + : Command(detail::Command::ALLOCA, Queue) {} + void printDot(std::ostream &Stream) const override {} + void emitInstrumentationData() override {} + cl_int enqueueImp() override { return CL_SUCCESS; } +}; + +class TestScheduler : public detail::Scheduler { +public: + static bool enqueueCommand(detail::Command *Cmd, + detail::EnqueueResultT &EnqueueResult, + detail::BlockingT Blocking) { + return GraphProcessor::enqueueCommand(Cmd, EnqueueResult, Blocking); + } +}; + +TEST_F(SchedulerTest, FailedDependency) { + detail::Requirement MockReq(/*Offset*/ {0, 0, 0}, /*AccessRange*/ {1, 1, 1}, + /*MemoryRange*/ {1, 1, 1}, + access::mode::read_write, /*SYCLMemObjT*/ nullptr, + /*Dims*/ 1, /*ElementSize*/ 1); + MockCommand MDep(detail::getSyclObjImpl(MQueue)); + MockCommand MUser(detail::getSyclObjImpl(MQueue)); + MDep.addUser(&MUser); + MUser.addDep(detail::DepDesc{&MDep, &MockReq, nullptr}); + MUser.MEnqueueStatus = detail::EnqueueResultT::SyclEnqueueReady; + MDep.MEnqueueStatus = detail::EnqueueResultT::SyclEnqueueFailed; + + detail::EnqueueResultT Res; + bool Enqueued = + TestScheduler::enqueueCommand(&MUser, Res, detail::NON_BLOCKING); + + ASSERT_FALSE(Enqueued) << "Enqueue process must fail\n"; + ASSERT_EQ(Res.MCmd, &MDep) << "Wrong failed command\n"; + ASSERT_EQ(Res.MResult, detail::EnqueueResultT::SyclEnqueueFailed) + << "Enqueue process must fail\n"; + ASSERT_EQ(MUser.MEnqueueStatus, detail::EnqueueResultT::SyclEnqueueReady) + << "MUser shouldn't be marked as failed\n"; + ASSERT_EQ(MDep.MEnqueueStatus, detail::EnqueueResultT::SyclEnqueueFailed) + << "MDep should be marked as failed\n"; +} From ba6e119597ad4900435875f11d795980b30283e3 Mon Sep 17 00:00:00 2001 From: Ivan Karachun Date: Mon, 30 Mar 2020 16:58:57 +0300 Subject: [PATCH 2/2] [SYCL] Applied reviewers' comments. Signed-off-by: Ivan Karachun --- sycl/source/detail/scheduler/commands.cpp | 3 +++ sycl/source/detail/scheduler/commands.hpp | 2 +- sycl/source/detail/scheduler/graph_processor.cpp | 2 +- sycl/test/scheduler/HandleException.cpp | 2 +- sycl/unittests/scheduler/FailedCommands.cpp | 4 ++-- 5 files changed, 8 insertions(+), 5 deletions(-) diff --git a/sycl/source/detail/scheduler/commands.cpp b/sycl/source/detail/scheduler/commands.cpp index cdae555c6452a..758dcd6808a38 100644 --- a/sycl/source/detail/scheduler/commands.cpp +++ b/sycl/source/detail/scheduler/commands.cpp @@ -501,6 +501,9 @@ bool Command::enqueue(EnqueueResultT &EnqueueResult, BlockingT Blocking) { return false; } + // Command status set to "failed" beforehand, so this command + // has already been marked as "failed" if enqueueImp throws an exception. + // This will avoid execution of the same failed command twice. MEnqueueStatus = EnqueueResultT::SyclEnqueueFailed; cl_int Res = enqueueImp(); diff --git a/sycl/source/detail/scheduler/commands.hpp b/sycl/source/detail/scheduler/commands.hpp index 2a53aad6c9c4a..915163f512bce 100644 --- a/sycl/source/detail/scheduler/commands.hpp +++ b/sycl/source/detail/scheduler/commands.hpp @@ -115,7 +115,7 @@ class Command { bool isFinished(); - bool isEnqueued() const { + bool isSuccessfullyEnqueued() const { return MEnqueueStatus == EnqueueResultT::SyclEnqueueSuccess; } diff --git a/sycl/source/detail/scheduler/graph_processor.cpp b/sycl/source/detail/scheduler/graph_processor.cpp index 95f393afaff8f..08c8a67431376 100644 --- a/sycl/source/detail/scheduler/graph_processor.cpp +++ b/sycl/source/detail/scheduler/graph_processor.cpp @@ -55,7 +55,7 @@ void Scheduler::GraphProcessor::waitForEvent(EventImplPtr Event) { bool Scheduler::GraphProcessor::enqueueCommand(Command *Cmd, EnqueueResultT &EnqueueResult, BlockingT Blocking) { - if (!Cmd || Cmd->isEnqueued()) + if (!Cmd || Cmd->isSuccessfullyEnqueued()) return true; // Indicates whether dependency cannot be enqueued diff --git a/sycl/test/scheduler/HandleException.cpp b/sycl/test/scheduler/HandleException.cpp index 0823380c3d587..a9fbd3cc9d8d9 100644 --- a/sycl/test/scheduler/HandleException.cpp +++ b/sycl/test/scheduler/HandleException.cpp @@ -15,7 +15,7 @@ using ArrayType = std::array; class kernelCompute; // Return 'true' if an exception was thrown. -bool run_kernel(const uint wg_size) { +bool run_kernel(const unsigned wg_size) { ArrayType index; const unsigned N = index.size(); { diff --git a/sycl/unittests/scheduler/FailedCommands.cpp b/sycl/unittests/scheduler/FailedCommands.cpp index 7b2e85d8bc8e5..a30c1426debb8 100644 --- a/sycl/unittests/scheduler/FailedCommands.cpp +++ b/sycl/unittests/scheduler/FailedCommands.cpp @@ -25,7 +25,7 @@ class MockCommand : public detail::Command { cl_int enqueueImp() override { return CL_SUCCESS; } }; -class TestScheduler : public detail::Scheduler { +class MockScheduler : public detail::Scheduler { public: static bool enqueueCommand(detail::Command *Cmd, detail::EnqueueResultT &EnqueueResult, @@ -48,7 +48,7 @@ TEST_F(SchedulerTest, FailedDependency) { detail::EnqueueResultT Res; bool Enqueued = - TestScheduler::enqueueCommand(&MUser, Res, detail::NON_BLOCKING); + MockScheduler::enqueueCommand(&MUser, Res, detail::NON_BLOCKING); ASSERT_FALSE(Enqueued) << "Enqueue process must fail\n"; ASSERT_EQ(Res.MCmd, &MDep) << "Wrong failed command\n";