Skip to content

Commit ddc4347

Browse files
KseniyaTikhomirovaKseniyaTikhomirova
authored
[SYCL] Add missed checks of ur call result in commands.cpp (#16748)
ur calls in enqueueImp should return error via return value (adapter->call_nocheck), not exception (adapter->call). ur_event_handle in corresponding event_impl also should be set only in case of successful call. Some functions have already had this logic. This PR fixes the rest. --------- Signed-off-by: Tikhomirova, Kseniya <[email protected]>
1 parent 1fd29f4 commit ddc4347

File tree

2 files changed

+127
-30
lines changed

2 files changed

+127
-30
lines changed

‎sycl/source/detail/scheduler/commands.cpp

Lines changed: 53 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -2525,7 +2525,7 @@ static ur_result_t SetKernelParamsAndLaunch(
25252525
property_list.size(), property_list.data(), RawEvents.size(),
25262526
RawEvents.empty() ? nullptr : &RawEvents[0],
25272527
OutEventImpl ? &UREvent : nullptr);
2528-
if (OutEventImpl) {
2528+
if ((Error == UR_RESULT_SUCCESS) && OutEventImpl) {
25292529
OutEventImpl->setHandle(UREvent);
25302530
}
25312531
return Error;
@@ -3421,15 +3421,21 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
34213421

34223422
ur_bool_t NativeCommandSupport = false;
34233423
assert(MQueue && "Native command should have an associated queue");
3424-
MQueue->getAdapter()->call<UrApiKind::urDeviceGetInfo>(
3424+
auto &Adapter = MQueue->getAdapter();
3425+
Adapter->call<UrApiKind::urDeviceGetInfo>(
34253426
detail::getSyclObjImpl(MQueue->get_device())->getHandleRef(),
34263427
UR_DEVICE_INFO_ENQUEUE_NATIVE_COMMAND_SUPPORT_EXP,
34273428
sizeof(NativeCommandSupport), &NativeCommandSupport, nullptr);
34283429
assert(NativeCommandSupport && "ext_codeplay_enqueue_native_command is not "
34293430
"supported on this device");
3430-
MQueue->getAdapter()->call<UrApiKind::urEnqueueNativeCommandExp>(
3431-
MQueue->getHandleRef(), InteropFreeFunc, &CustomOpData, ReqMems.size(),
3432-
ReqMems.data(), nullptr, RawEvents.size(), RawEvents.data(), Event);
3431+
if (auto Result =
3432+
Adapter->call_nocheck<UrApiKind::urEnqueueNativeCommandExp>(
3433+
MQueue->getHandleRef(), InteropFreeFunc, &CustomOpData,
3434+
ReqMems.size(), ReqMems.data(), nullptr, RawEvents.size(),
3435+
RawEvents.data(), Event);
3436+
Result != UR_RESULT_SUCCESS)
3437+
return Result;
3438+
34333439
SetEventHandleOrDiscard();
34343440
return UR_RESULT_SUCCESS;
34353441
}
@@ -3449,8 +3455,12 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
34493455
const AdapterPtr &Adapter = MQueue->getAdapter();
34503456
if (MEvent != nullptr)
34513457
MEvent->setHostEnqueueTime();
3452-
Adapter->call<UrApiKind::urEnqueueEventsWaitWithBarrierExt>(
3453-
MQueue->getHandleRef(), &Properties, 0, nullptr, Event);
3458+
if (auto Result =
3459+
Adapter->call_nocheck<UrApiKind::urEnqueueEventsWaitWithBarrierExt>(
3460+
MQueue->getHandleRef(), &Properties, 0, nullptr, Event);
3461+
Result != UR_RESULT_SUCCESS)
3462+
return Result;
3463+
34543464
SetEventHandleOrDiscard();
34553465
return UR_RESULT_SUCCESS;
34563466
}
@@ -3479,9 +3489,13 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
34793489
const AdapterPtr &Adapter = MQueue->getAdapter();
34803490
if (MEvent != nullptr)
34813491
MEvent->setHostEnqueueTime();
3482-
Adapter->call<UrApiKind::urEnqueueEventsWaitWithBarrierExt>(
3483-
MQueue->getHandleRef(), &Properties, UrEvents.size(), &UrEvents[0],
3484-
Event);
3492+
if (auto Result =
3493+
Adapter->call_nocheck<UrApiKind::urEnqueueEventsWaitWithBarrierExt>(
3494+
MQueue->getHandleRef(), &Properties, UrEvents.size(),
3495+
&UrEvents[0], Event);
3496+
Result != UR_RESULT_SUCCESS)
3497+
return Result;
3498+
34853499
SetEventHandleOrDiscard();
34863500
return UR_RESULT_SUCCESS;
34873501
}
@@ -3493,6 +3507,10 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
34933507
ur_event_handle_t *TimestampDeps = nullptr;
34943508
size_t NumTimestampDeps = 0;
34953509

3510+
// TO DO - once the following WA removed: to change call to call_nocheck and
3511+
// return operation result to Command::enqueue (see other CG types). Set
3512+
// UREvent to EventImpl only for successful case.
3513+
34963514
// If the queue is not in-order, the implementation will need to first
34973515
// insert a marker event that the timestamp waits for.
34983516
ur_event_handle_t PreTimestampMarkerEvent{};
@@ -3581,15 +3599,18 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
35813599
static_cast<CGExecCommandBuffer *>(MCommandGroup.get());
35823600
if (MEvent != nullptr)
35833601
MEvent->setHostEnqueueTime();
3584-
ur_result_t Err =
3585-
MQueue->getAdapter()
3586-
->call_nocheck<UrApiKind::urCommandBufferEnqueueExp>(
3587-
CmdBufferCG->MCommandBuffer, MQueue->getHandleRef(),
3588-
RawEvents.size(), RawEvents.empty() ? nullptr : &RawEvents[0],
3589-
Event);
3602+
if (auto Result =
3603+
MQueue->getAdapter()
3604+
->call_nocheck<UrApiKind::urCommandBufferEnqueueExp>(
3605+
CmdBufferCG->MCommandBuffer, MQueue->getHandleRef(),
3606+
RawEvents.size(),
3607+
RawEvents.empty() ? nullptr : &RawEvents[0], Event);
3608+
Result != UR_RESULT_SUCCESS)
3609+
return Result;
3610+
35903611
SetEventHandleOrDiscard();
35913612

3592-
return Err;
3613+
return UR_RESULT_SUCCESS;
35933614
}
35943615
case CGType::CopyImage: {
35953616
CGCopyImage *Copy = (CGCopyImage *)MCommandGroup.get();
@@ -3614,11 +3635,11 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
36143635
const detail::AdapterPtr &Adapter = MQueue->getAdapter();
36153636
auto OptWaitValue = SemWait->getWaitValue();
36163637
uint64_t WaitValue = OptWaitValue.has_value() ? OptWaitValue.value() : 0;
3617-
Adapter->call<UrApiKind::urBindlessImagesWaitExternalSemaphoreExp>(
3618-
MQueue->getHandleRef(), SemWait->getExternalSemaphore(),
3619-
OptWaitValue.has_value(), WaitValue, 0, nullptr, nullptr);
36203638

3621-
return UR_RESULT_SUCCESS;
3639+
return Adapter
3640+
->call_nocheck<UrApiKind::urBindlessImagesWaitExternalSemaphoreExp>(
3641+
MQueue->getHandleRef(), SemWait->getExternalSemaphore(),
3642+
OptWaitValue.has_value(), WaitValue, 0, nullptr, nullptr);
36223643
}
36233644
case CGType::SemaphoreSignal: {
36243645
assert(MQueue &&
@@ -3628,11 +3649,10 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
36283649
auto OptSignalValue = SemSignal->getSignalValue();
36293650
uint64_t SignalValue =
36303651
OptSignalValue.has_value() ? OptSignalValue.value() : 0;
3631-
Adapter->call<UrApiKind::urBindlessImagesSignalExternalSemaphoreExp>(
3632-
MQueue->getHandleRef(), SemSignal->getExternalSemaphore(),
3633-
OptSignalValue.has_value(), SignalValue, 0, nullptr, nullptr);
3634-
3635-
return UR_RESULT_SUCCESS;
3652+
return Adapter
3653+
->call_nocheck<UrApiKind::urBindlessImagesSignalExternalSemaphoreExp>(
3654+
MQueue->getHandleRef(), SemSignal->getExternalSemaphore(),
3655+
OptSignalValue.has_value(), SignalValue, 0, nullptr, nullptr);
36363656
}
36373657
case CGType::None: {
36383658
if (RawEvents.empty()) {
@@ -3644,11 +3664,14 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
36443664
assert(MQueue && "Empty node should have an associated queue");
36453665
const detail::AdapterPtr &Adapter = MQueue->getAdapter();
36463666
ur_event_handle_t Event;
3647-
ur_result_t Result = Adapter->call_nocheck<UrApiKind::urEnqueueEventsWait>(
3648-
MQueue->getHandleRef(), RawEvents.size(),
3649-
RawEvents.size() ? &RawEvents[0] : nullptr, &Event);
3667+
if (auto Result = Adapter->call_nocheck<UrApiKind::urEnqueueEventsWait>(
3668+
MQueue->getHandleRef(), RawEvents.size(),
3669+
RawEvents.size() ? &RawEvents[0] : nullptr, &Event);
3670+
Result != UR_RESULT_SUCCESS)
3671+
return Result;
3672+
36503673
MEvent->setHandle(Event);
3651-
return Result;
3674+
return UR_RESULT_SUCCESS;
36523675
}
36533676
}
36543677
return UR_RESULT_ERROR_INVALID_OPERATION;

‎sycl/unittests/scheduler/FailedCommands.cpp

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,3 +86,77 @@ TEST_F(SchedulerTest, FailedCopyBackException) {
8686
&failingUrCall);
8787
RunWithFailedCommandsAndCheck(false, 1);
8888
}
89+
90+
bool DummyEventReturned = false;
91+
bool DummyEventReleaseAttempt = false;
92+
ur_event_handle_t DummyEvent = mock::createDummyHandle<ur_event_handle_t>();
93+
94+
inline ur_result_t failedEnqueueKernelLaunchWithDummy(void *pParams) {
95+
DummyEventReturned = true;
96+
auto params = *static_cast<ur_enqueue_kernel_launch_params_t *>(pParams);
97+
**params.pphEvent = DummyEvent;
98+
return UR_RESULT_ERROR_UNKNOWN;
99+
}
100+
101+
inline ur_result_t checkDummyInEventRelease(void *pParams) {
102+
auto params = static_cast<ur_event_handle_t>(pParams);
103+
DummyEventReleaseAttempt = params == DummyEvent;
104+
return UR_RESULT_SUCCESS;
105+
}
106+
107+
inline ur_result_t failedEnqueueBarrierWithDummy(void *pParams) {
108+
DummyEventReturned = true;
109+
auto params =
110+
*static_cast<ur_enqueue_events_wait_with_barrier_ext_params_t *>(pParams);
111+
**params.pphEvent = DummyEvent;
112+
return UR_RESULT_ERROR_UNKNOWN;
113+
}
114+
115+
// Checks that in case of failed command and "valid" event assigned to output
116+
// event var, RT ignores it and do not call release since its usage is undefined
117+
// behavior.
118+
TEST(FailedCommandsTest, CheckUREventReleaseWithKernel) {
119+
DummyEventReleaseAttempt = false;
120+
DummyEventReturned = false;
121+
sycl::unittest::UrMock<> Mock;
122+
mock::getCallbacks().set_before_callback("urEnqueueKernelLaunch",
123+
&failedEnqueueKernelLaunchWithDummy);
124+
mock::getCallbacks().set_before_callback("urEventRelease",
125+
&checkDummyInEventRelease);
126+
platform Plt = sycl::platform();
127+
queue Queue(context(Plt), default_selector_v);
128+
{
129+
try {
130+
Queue.submit(
131+
[&](sycl::handler &CGH) { CGH.single_task<TestKernel<1>>([]() {}); });
132+
} catch (...) {
133+
}
134+
}
135+
Queue.wait();
136+
ASSERT_TRUE(DummyEventReturned);
137+
ASSERT_FALSE(DummyEventReleaseAttempt);
138+
}
139+
140+
// Checks that in case of failed command and "valid" event assigned to output
141+
// event var, RT ignores it and do not call release since its usage is undefined
142+
// behavior.
143+
TEST(FailedCommandsTest, CheckUREventReleaseWithBarrier) {
144+
DummyEventReleaseAttempt = false;
145+
DummyEventReturned = false;
146+
sycl::unittest::UrMock<> Mock;
147+
mock::getCallbacks().set_before_callback("urEnqueueEventsWaitWithBarrierExt",
148+
&failedEnqueueBarrierWithDummy);
149+
mock::getCallbacks().set_before_callback("urEventRelease",
150+
&checkDummyInEventRelease);
151+
platform Plt = sycl::platform();
152+
queue Queue(context(Plt), default_selector_v);
153+
{
154+
try {
155+
Queue.submit([&](sycl::handler &CGH) { CGH.ext_oneapi_barrier(); });
156+
} catch (...) {
157+
}
158+
}
159+
Queue.wait();
160+
ASSERT_TRUE(DummyEventReturned);
161+
ASSERT_FALSE(DummyEventReleaseAttempt);
162+
}

0 commit comments

Comments
 (0)