Skip to content

Commit 834e127

Browse files
jchlandajchlanda
authored
[SYCL][NFC] Make UR_CHECK_ERROR a void return macro (#11100)
`UR_CHECK_ERROR` was designed to return `ur_result_t`, however in practice it was guaranteed to only ever return `UR_RESULT_SUCCESS`, as other paths would either terminate, abort or throw. This in turns leads to poor quality/error prone code, as the codebase was littered with: * statements not checking the return value - depending on the compiler generating a warning, * extra check on the return which was only ever going to be true. Some care was required, as the codebase has a habit of accumulating err codes across branches, so depending on the use case the initial value of `ur_result_t Result`s had to be set accordingly (now that `UR_CHECK_ERROR` does not return).
1 parent cca22fa commit 834e127

File tree

10 files changed

+107
-113
lines changed

10 files changed

+107
-113
lines changed

‎common.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,10 @@ ur_result_t mapErrorUR(hipError_t Result) {
2828
}
2929
}
3030

31-
ur_result_t checkErrorUR(hipError_t Result, const char *Function, int Line,
32-
const char *File) {
31+
void checkErrorUR(hipError_t Result, const char *Function, int Line,
32+
const char *File) {
3333
if (Result == hipSuccess) {
34-
return UR_RESULT_SUCCESS;
34+
return;
3535
}
3636

3737
if (std::getenv("SYCL_PI_SUPPRESS_ERROR_MESSAGE") == nullptr ||
@@ -56,10 +56,10 @@ ur_result_t checkErrorUR(hipError_t Result, const char *Function, int Line,
5656
throw mapErrorUR(Result);
5757
}
5858

59-
ur_result_t checkErrorUR(ur_result_t Result, const char *Function, int Line,
60-
const char *File) {
59+
void checkErrorUR(ur_result_t Result, const char *Function, int Line,
60+
const char *File) {
6161
if (Result == UR_RESULT_SUCCESS) {
62-
return UR_RESULT_SUCCESS;
62+
return;
6363
}
6464

6565
if (std::getenv("SYCL_PI_SUPPRESS_ERROR_MESSAGE") == nullptr ||

‎common.hpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,10 @@ typedef hipArray *hipCUarray;
6767

6868
ur_result_t mapErrorUR(hipError_t Result);
6969

70-
ur_result_t checkErrorUR(hipError_t Result, const char *Function, int Line,
71-
const char *File);
72-
ur_result_t checkErrorUR(ur_result_t Result, const char *Function, int Line,
73-
const char *File);
70+
void checkErrorUR(hipError_t Result, const char *Function, int Line,
71+
const char *File);
72+
void checkErrorUR(ur_result_t Result, const char *Function, int Line,
73+
const char *File);
7474

7575
#define UR_CHECK_ERROR(result) \
7676
checkErrorUR(result, __func__, __LINE__, __FILE__)

‎enqueue.cpp

Lines changed: 45 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,8 @@ ur_result_t enqueueEventsWait(ur_queue_handle_t CommandQueue,
4949
if (Event->getStream() == Stream) {
5050
return UR_RESULT_SUCCESS;
5151
} else {
52-
return UR_CHECK_ERROR(hipStreamWaitEvent(Stream, Event->get(), 0));
52+
UR_CHECK_ERROR(hipStreamWaitEvent(Stream, Event->get(), 0));
53+
return UR_RESULT_SUCCESS;
5354
}
5455
});
5556

@@ -109,7 +110,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemBufferWrite(
109110
UR_CHECK_ERROR(RetImplEvent->start());
110111
}
111112

112-
Result = UR_CHECK_ERROR(
113+
UR_CHECK_ERROR(
113114
hipMemcpyHtoDAsync(hBuffer->Mem.BufferMem.getWithOffset(offset),
114115
const_cast<void *>(pSrc), size, HIPStream));
115116

@@ -118,7 +119,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemBufferWrite(
118119
}
119120

120121
if (blockingWrite) {
121-
Result = UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
122+
UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
122123
}
123124

124125
if (phEvent) {
@@ -155,15 +156,15 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemBufferRead(
155156
UR_CHECK_ERROR(RetImplEvent->start());
156157
}
157158

158-
Result = UR_CHECK_ERROR(hipMemcpyDtoHAsync(
159+
UR_CHECK_ERROR(hipMemcpyDtoHAsync(
159160
pDst, hBuffer->Mem.BufferMem.getWithOffset(offset), size, HIPStream));
160161

161162
if (phEvent) {
162163
UR_CHECK_ERROR(RetImplEvent->record());
163164
}
164165

165166
if (blockingRead) {
166-
Result = UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
167+
UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
167168
}
168169

169170
if (phEvent) {
@@ -309,7 +310,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueKernelLaunch(
309310

310311
if (LocalMemSzPtr) {
311312
int DeviceMaxLocalMem = 0;
312-
Result = UR_CHECK_ERROR(hipDeviceGetAttribute(
313+
UR_CHECK_ERROR(hipDeviceGetAttribute(
313314
&DeviceMaxLocalMem, hipDeviceAttributeMaxSharedMemoryPerBlock,
314315
HIPDev));
315316

@@ -322,11 +323,11 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueKernelLaunch(
322323
UR_RESULT_ERROR_ADAPTER_SPECIFIC);
323324
return UR_RESULT_ERROR_ADAPTER_SPECIFIC;
324325
}
325-
Result = UR_CHECK_ERROR(hipFuncSetAttribute(
326+
UR_CHECK_ERROR(hipFuncSetAttribute(
326327
HIPFunc, hipFuncAttributeMaxDynamicSharedMemorySize, EnvVal));
327328
}
328329

329-
Result = UR_CHECK_ERROR(hipModuleLaunchKernel(
330+
UR_CHECK_ERROR(hipModuleLaunchKernel(
330331
HIPFunc, BlocksPerGrid[0], BlocksPerGrid[1], BlocksPerGrid[2],
331332
ThreadsPerBlock[0], ThreadsPerBlock[1], ThreadsPerBlock[2],
332333
hKernel->getLocalSize(), HIPStream, ArgIndices.data(), nullptr));
@@ -405,13 +406,13 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueEventsWaitWithBarrier(
405406
Event->getComputeStreamToken())) {
406407
return UR_RESULT_SUCCESS;
407408
} else {
408-
return UR_CHECK_ERROR(
409-
hipStreamWaitEvent(HIPStream, Event->get(), 0));
409+
UR_CHECK_ERROR(hipStreamWaitEvent(HIPStream, Event->get(), 0));
410+
return UR_RESULT_SUCCESS;
410411
}
411412
});
412413
}
413414

414-
Result = UR_CHECK_ERROR(hipEventRecord(hQueue->BarrierEvent, HIPStream));
415+
UR_CHECK_ERROR(hipEventRecord(hQueue->BarrierEvent, HIPStream));
415416
for (unsigned int i = 0; i < hQueue->ComputeAppliedBarrier.size(); i++) {
416417
hQueue->ComputeAppliedBarrier[i] = false;
417418
}
@@ -487,7 +488,8 @@ static ur_result_t commonEnqueueMemBufferCopyRect(
487488
Params.dstPitch = DstRowPitch;
488489
Params.dstHeight = DstSlicePitch / DstRowPitch;
489490

490-
return UR_CHECK_ERROR(hipDrvMemcpy3DAsync(&Params, HipStream));
491+
UR_CHECK_ERROR(hipDrvMemcpy3DAsync(&Params, HipStream));
492+
return UR_RESULT_SUCCESS;
491493
}
492494

493495
UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemBufferReadRect(
@@ -546,7 +548,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemBufferReadRect(
546548
}
547549

548550
if (blockingRead) {
549-
Result = UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
551+
UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
550552
}
551553

552554
if (phEvent) {
@@ -593,7 +595,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemBufferWriteRect(
593595
}
594596

595597
if (blockingWrite) {
596-
Result = UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
598+
UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
597599
}
598600

599601
if (phEvent) {
@@ -638,7 +640,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemBufferCopy(
638640
auto Src = hBufferSrc->Mem.BufferMem.getWithOffset(srcOffset);
639641
auto Dst = hBufferDst->Mem.BufferMem.getWithOffset(dstOffset);
640642

641-
Result = UR_CHECK_ERROR(hipMemcpyDtoDAsync(Dst, Src, size, Stream));
643+
UR_CHECK_ERROR(hipMemcpyDtoDAsync(Dst, Src, size, Stream));
642644

643645
if (phEvent) {
644646
UR_CHECK_ERROR(RetImplEvent->record());
@@ -713,10 +715,7 @@ ur_result_t commonMemSetLargePattern(hipStream_t Stream, uint32_t PatternSize,
713715

714716
// Get 4-byte chunk of the pattern and call hipMemsetD32Async
715717
auto Value = *(static_cast<const uint32_t *>(pPattern));
716-
auto Result = UR_CHECK_ERROR(hipMemsetD32Async(Ptr, Value, Count32, Stream));
717-
if (Result != UR_RESULT_SUCCESS) {
718-
return Result;
719-
}
718+
UR_CHECK_ERROR(hipMemsetD32Async(Ptr, Value, Count32, Stream));
720719
for (auto step = 4u; step < NumberOfSteps; ++step) {
721720
// take 1 byte of the pattern
722721
Value = *(static_cast<const uint8_t *>(pPattern) + step);
@@ -726,11 +725,8 @@ ur_result_t commonMemSetLargePattern(hipStream_t Stream, uint32_t PatternSize,
726725
(step * sizeof(uint8_t)));
727726

728727
// set all of the pattern chunks
729-
Result = UR_CHECK_ERROR(hipMemset2DAsync(OffsetPtr, Pitch, Value,
730-
sizeof(uint8_t), Height, Stream));
731-
if (Result != UR_RESULT_SUCCESS) {
732-
return Result;
733-
}
728+
UR_CHECK_ERROR(hipMemset2DAsync(OffsetPtr, Pitch, Value, sizeof(uint8_t),
729+
Height, Stream));
734730
}
735731
return UR_RESULT_SUCCESS;
736732
}
@@ -764,7 +760,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemBufferFill(
764760
ScopedContext Active(hQueue->getDevice());
765761

766762
auto Stream = hQueue->getNextTransferStream();
767-
ur_result_t Result;
763+
ur_result_t Result = UR_RESULT_SUCCESS;
768764
if (phEventWaitList) {
769765
Result = enqueueEventsWait(hQueue, Stream, numEventsInWaitList,
770766
phEventWaitList);
@@ -784,17 +780,17 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemBufferFill(
784780
switch (patternSize) {
785781
case 1: {
786782
auto Value = *static_cast<const uint8_t *>(pPattern);
787-
Result = UR_CHECK_ERROR(hipMemsetD8Async(DstDevice, Value, N, Stream));
783+
UR_CHECK_ERROR(hipMemsetD8Async(DstDevice, Value, N, Stream));
788784
break;
789785
}
790786
case 2: {
791787
auto Value = *static_cast<const uint16_t *>(pPattern);
792-
Result = UR_CHECK_ERROR(hipMemsetD16Async(DstDevice, Value, N, Stream));
788+
UR_CHECK_ERROR(hipMemsetD16Async(DstDevice, Value, N, Stream));
793789
break;
794790
}
795791
case 4: {
796792
auto Value = *static_cast<const uint32_t *>(pPattern);
797-
Result = UR_CHECK_ERROR(hipMemsetD32Async(DstDevice, Value, N, Stream));
793+
UR_CHECK_ERROR(hipMemsetD32Async(DstDevice, Value, N, Stream));
798794
break;
799795
}
800796

@@ -855,7 +851,8 @@ static ur_result_t commonEnqueueMemImageNDCopy(
855851
}
856852
CpyDesc.WidthInBytes = Region[0];
857853
CpyDesc.Height = Region[1];
858-
return UR_CHECK_ERROR(hipMemcpyParam2DAsync(&CpyDesc, HipStream));
854+
UR_CHECK_ERROR(hipMemcpyParam2DAsync(&CpyDesc, HipStream));
855+
return UR_RESULT_SUCCESS;
859856
}
860857

861858
if (ImgType == UR_MEM_TYPE_IMAGE3D) {
@@ -884,8 +881,8 @@ static ur_result_t commonEnqueueMemImageNDCopy(
884881
CpyDesc.WidthInBytes = Region[0];
885882
CpyDesc.Height = Region[1];
886883
CpyDesc.Depth = Region[2];
887-
return UR_CHECK_ERROR(hipDrvMemcpy3DAsync(&CpyDesc, HipStream));
888-
return UR_RESULT_ERROR_UNKNOWN;
884+
UR_CHECK_ERROR(hipDrvMemcpy3DAsync(&CpyDesc, HipStream));
885+
return UR_RESULT_SUCCESS;
889886
}
890887

891888
return UR_RESULT_ERROR_INVALID_VALUE;
@@ -948,7 +945,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueMemImageRead(
948945
}
949946

950947
if (blockingRead) {
951-
Result = UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
948+
UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
952949
}
953950
} catch (ur_result_t Err) {
954951
return Err;
@@ -1243,19 +1240,19 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueUSMFill(
12431240
auto N = size / patternSize;
12441241
switch (patternSize) {
12451242
case 1:
1246-
Result = UR_CHECK_ERROR(
1247-
hipMemsetD8Async(reinterpret_cast<hipDeviceptr_t>(ptr),
1248-
*(const uint8_t *)pPattern & 0xFF, N, HIPStream));
1243+
UR_CHECK_ERROR(hipMemsetD8Async(reinterpret_cast<hipDeviceptr_t>(ptr),
1244+
*(const uint8_t *)pPattern & 0xFF, N,
1245+
HIPStream));
12491246
break;
12501247
case 2:
1251-
Result = UR_CHECK_ERROR(hipMemsetD16Async(
1252-
reinterpret_cast<hipDeviceptr_t>(ptr),
1253-
*(const uint16_t *)pPattern & 0xFFFF, N, HIPStream));
1248+
UR_CHECK_ERROR(hipMemsetD16Async(reinterpret_cast<hipDeviceptr_t>(ptr),
1249+
*(const uint16_t *)pPattern & 0xFFFF, N,
1250+
HIPStream));
12541251
break;
12551252
case 4:
1256-
Result = UR_CHECK_ERROR(hipMemsetD32Async(
1257-
reinterpret_cast<hipDeviceptr_t>(ptr),
1258-
*(const uint32_t *)pPattern & 0xFFFFFFFF, N, HIPStream));
1253+
UR_CHECK_ERROR(hipMemsetD32Async(reinterpret_cast<hipDeviceptr_t>(ptr),
1254+
*(const uint32_t *)pPattern & 0xFFFFFFFF,
1255+
N, HIPStream));
12591256
break;
12601257

12611258
default:
@@ -1265,7 +1262,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueUSMFill(
12651262
}
12661263

12671264
if (phEvent) {
1268-
Result = UR_CHECK_ERROR(EventPtr->record());
1265+
UR_CHECK_ERROR(EventPtr->record());
12691266
*phEvent = EventPtr.release();
12701267
}
12711268
} catch (ur_result_t Err) {
@@ -1294,13 +1291,13 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueUSMMemcpy(
12941291
UR_COMMAND_USM_MEMCPY, hQueue, HIPStream));
12951292
UR_CHECK_ERROR(EventPtr->start());
12961293
}
1297-
Result = UR_CHECK_ERROR(
1294+
UR_CHECK_ERROR(
12981295
hipMemcpyAsync(pDst, pSrc, size, hipMemcpyDefault, HIPStream));
12991296
if (phEvent) {
13001297
UR_CHECK_ERROR(EventPtr->record());
13011298
}
13021299
if (blocking) {
1303-
Result = UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
1300+
UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
13041301
}
13051302
if (phEvent) {
13061303
*phEvent = EventPtr.release();
@@ -1367,7 +1364,7 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueUSMPrefetch(
13671364
UR_COMMAND_USM_PREFETCH, hQueue, HIPStream));
13681365
UR_CHECK_ERROR(EventPtr->start());
13691366
}
1370-
Result = UR_CHECK_ERROR(
1367+
UR_CHECK_ERROR(
13711368
hipMemPrefetchAsync(pMem, size, hQueue->getDevice()->get(), HIPStream));
13721369
if (phEvent) {
13731370
UR_CHECK_ERROR(EventPtr->record());
@@ -1439,16 +1436,15 @@ UR_APIEXPORT ur_result_t UR_APICALL urEnqueueUSMMemcpy2D(
14391436
UR_CHECK_ERROR(RetImplEvent->start());
14401437
}
14411438

1442-
Result =
1443-
UR_CHECK_ERROR(hipMemcpy2DAsync(pDst, dstPitch, pSrc, srcPitch, width,
1444-
height, hipMemcpyDefault, HIPStream));
1439+
UR_CHECK_ERROR(hipMemcpy2DAsync(pDst, dstPitch, pSrc, srcPitch, width,
1440+
height, hipMemcpyDefault, HIPStream));
14451441

14461442
if (phEvent) {
14471443
UR_CHECK_ERROR(RetImplEvent->record());
14481444
*phEvent = RetImplEvent.release();
14491445
}
14501446
if (blocking) {
1451-
Result = UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
1447+
UR_CHECK_ERROR(hipStreamSynchronize(HIPStream));
14521448
}
14531449
} catch (ur_result_t Err) {
14541450
Result = Err;

‎event.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,8 @@ ur_result_t ur_event_handle_t_::record() {
144144
detail::ur::die(
145145
"Unrecoverable program state reached in event identifier overflow");
146146
}
147-
Result = UR_CHECK_ERROR(hipEventRecord(EvEnd, Stream));
147+
UR_CHECK_ERROR(hipEventRecord(EvEnd, Stream));
148+
Result = UR_RESULT_SUCCESS;
148149
} catch (ur_result_t Error) {
149150
Result = Error;
150151
}
@@ -157,9 +158,9 @@ ur_result_t ur_event_handle_t_::record() {
157158
}
158159

159160
ur_result_t ur_event_handle_t_::wait() {
160-
ur_result_t Result;
161+
ur_result_t Result = UR_RESULT_SUCCESS;
161162
try {
162-
Result = UR_CHECK_ERROR(hipEventSynchronize(EvEnd));
163+
UR_CHECK_ERROR(hipEventSynchronize(EvEnd));
163164
HasBeenWaitedOn = true;
164165
} catch (ur_result_t Error) {
165166
Result = Error;

‎kernel.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ urKernelCreate(ur_program_handle_t hProgram, const char *pKernelName,
2020
ScopedContext Active(hProgram->getContext()->getDevice());
2121

2222
hipFunction_t HIPFunc;
23-
Result = UR_CHECK_ERROR(
23+
UR_CHECK_ERROR(
2424
hipModuleGetFunction(&HIPFunc, hProgram->get(), pKernelName));
2525

2626
std::string KernelNameWoffset = std::string(pKernelName) + "_with_offset";
@@ -32,7 +32,7 @@ urKernelCreate(ur_program_handle_t hProgram, const char *pKernelName,
3232
if (OffsetRes == hipErrorNotFound) {
3333
HIPFuncWithOffsetParam = nullptr;
3434
} else {
35-
Result = UR_CHECK_ERROR(OffsetRes);
35+
UR_CHECK_ERROR(OffsetRes);
3636
}
3737
RetKernel = std::unique_ptr<ur_kernel_handle_t_>(
3838
new ur_kernel_handle_t_{HIPFunc, HIPFuncWithOffsetParam, pKernelName,

0 commit comments

Comments
 (0)