Skip to content

Commit ded4105

Browse files
author
Belton-Schure, Aidan
authored
SWDEV-515426 - Use RAII classes for comgr (#28)
Change-Id: I9f6005542cc88f1e16e22741dcc0ce904fdaa2b0
1 parent 376f23b commit ded4105

File tree

1 file changed

+131
-137
lines changed

1 file changed

+131
-137
lines changed

‎hipamd/src/hip_fatbin.cpp

+131-137
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,70 @@ THE SOFTWARE.
2929

3030
namespace hip {
3131

32+
namespace comgr_helper {
33+
34+
template <typename comgr_T> class ComgrUniqueHandle {
35+
public:
36+
ComgrUniqueHandle() = default;
37+
// constructor which takes ownership of a correctly initialzed handle
38+
ComgrUniqueHandle(comgr_T& handle) : comgr_obj_(handle) { handle = {0}; };
39+
40+
template <typename T = comgr_T,
41+
std::enable_if_t<std::is_same_v<T, amd_comgr_data_set_t> ||
42+
std::is_same_v<T, amd_comgr_action_info_t>,
43+
bool> = true>
44+
[[nodiscard]] amd_comgr_status_t Create() {
45+
if constexpr (std::is_same_v<T, amd_comgr_data_set_t>) {
46+
return amd::Comgr::create_data_set(&comgr_obj_);
47+
} else if constexpr (std::is_same_v<T, amd_comgr_action_info_t>) {
48+
return amd::Comgr::create_action_info(&comgr_obj_);
49+
}
50+
51+
// Unreachable code
52+
return AMD_COMGR_STATUS_SUCCESS;
53+
}
54+
55+
template <typename T = comgr_T,
56+
std::enable_if_t<std::is_same_v<T, amd_comgr_data_t>, bool> = true>
57+
[[nodiscard]] amd_comgr_status_t Create(amd_comgr_data_kind_t kind) {
58+
return amd::Comgr::create_data(kind, &comgr_obj_);
59+
}
60+
61+
~ComgrUniqueHandle() {
62+
if (comgr_obj_.handle != 0) {
63+
if constexpr (std::is_same_v<comgr_T, amd_comgr_data_set_t>) {
64+
amd::Comgr::destroy_data_set(comgr_obj_);
65+
} else if constexpr (std::is_same_v<comgr_T, amd_comgr_action_info_t>) {
66+
amd::Comgr::destroy_action_info(comgr_obj_);
67+
} else if constexpr (std::is_same_v<comgr_T, amd_comgr_data_t>) {
68+
amd::Comgr::release_data(comgr_obj_);
69+
}
70+
}
71+
}
72+
73+
// Delete all copy and move operators
74+
ComgrUniqueHandle(ComgrUniqueHandle&) = delete;
75+
ComgrUniqueHandle(ComgrUniqueHandle&&) = delete;
76+
ComgrUniqueHandle& operator=(ComgrUniqueHandle&) = delete;
77+
ComgrUniqueHandle& operator=(ComgrUniqueHandle&&) = delete;
78+
79+
// Method to access data
80+
comgr_T get() const {
81+
assert(comgr_obj_.handle != 0);
82+
return comgr_obj_;
83+
}
84+
85+
private:
86+
comgr_T comgr_obj_{0};
87+
};
88+
89+
90+
typedef ComgrUniqueHandle<amd_comgr_data_set_t> ComgrDataSetUniqueHandle;
91+
typedef ComgrUniqueHandle<amd_comgr_action_info_t> ComgrActionInfoUniqueHandle;
92+
typedef ComgrUniqueHandle<amd_comgr_data_t> ComgrDataUniqueHandle;
93+
94+
} // namespace comgr_helper
95+
3296
FatBinaryDeviceInfo::~FatBinaryDeviceInfo() {
3397
if (program_ != nullptr) {
3498
program_->unload();
@@ -309,81 +373,59 @@ hipError_t FatBinaryInfo::ExtractFatBinaryUsingCOMGR(const std::vector<hip::Devi
309373
} else {
310374
LogPrintfDebug("%s", "SPIRV isa found");
311375

312-
amd_comgr_data_set_t spirv_data_set, bc_data_set;
313-
amd_comgr_data_t spirv_data;
314-
amd_comgr_action_info_t action;
376+
comgr_helper::ComgrDataSetUniqueHandle spirv_data_set, bc_data_set;
377+
comgr_helper::ComgrDataUniqueHandle spirv_data;
378+
comgr_helper::ComgrActionInfoUniqueHandle action;
315379

316-
if (comgr_status = amd::Comgr::create_data_set(&spirv_data_set);
317-
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
380+
if (comgr_status = spirv_data_set.Create(); comgr_status != AMD_COMGR_STATUS_SUCCESS) {
318381
LogError("Failed to create SPIRV Data set");
319-
return hipErrorInvalidValue;
382+
break;
320383
}
321384

322-
if (comgr_status = amd::Comgr::create_data(AMD_COMGR_DATA_KIND_SPIRV, &spirv_data);
385+
if (comgr_status = spirv_data.Create(AMD_COMGR_DATA_KIND_SPIRV);
323386
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
324387
LogError("Failed to create SPIRV Data");
325-
amd::Comgr::destroy_data_set(spirv_data_set);
326-
return hipErrorInvalidValue;
388+
break;
327389
}
328390

329-
if (comgr_status = amd::Comgr::set_data(spirv_data, spirv_isa_handle->second.first /* size */,
330-
reinterpret_cast<char*>(const_cast<void*>(image_)) +
331-
spirv_isa_handle->second.second /* buffer */);
391+
if (comgr_status =
392+
amd::Comgr::set_data(spirv_data.get(), spirv_isa_handle->second.first /* size */,
393+
reinterpret_cast<char*>(const_cast<void*>(image_)) +
394+
spirv_isa_handle->second.second /* buffer */);
332395
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
333396
LogError("Failed to assign data in comgr");
334-
(void)amd::Comgr::release_data(spirv_data);
335-
(void)amd::Comgr::destroy_data_set(spirv_data_set);
336-
return hipErrorInvalidValue;
397+
break;
337398
}
338399

339-
if (comgr_status = amd::Comgr::set_data_name(spirv_data, "hip_code_object.spv");
400+
if (comgr_status = amd::Comgr::set_data_name(spirv_data.get(), "hip_code_object.spv");
340401
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
341402
LogError("Failed to set data name");
342-
(void)amd::Comgr::release_data(spirv_data);
343-
(void)amd::Comgr::destroy_data_set(spirv_data_set);
344-
return hipErrorInvalidValue;
403+
break;
345404
}
346405

347-
if (comgr_status = amd::Comgr::data_set_add(spirv_data_set, spirv_data);
406+
if (comgr_status = amd::Comgr::data_set_add(spirv_data_set.get(), spirv_data.get());
348407
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
349408
LogError("Failed to add spir data");
350-
(void)amd::Comgr::release_data(spirv_data);
351-
(void)amd::Comgr::destroy_data_set(spirv_data_set);
352-
return hipErrorInvalidValue;
409+
break;
353410
}
354411

355-
if (comgr_status = amd::Comgr::create_action_info(&action);
356-
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
412+
if (comgr_status = action.Create(); comgr_status != AMD_COMGR_STATUS_SUCCESS) {
357413
LogError("Failed to create action");
358-
(void)amd::Comgr::release_data(spirv_data);
359-
(void)amd::Comgr::destroy_data_set(spirv_data_set);
360-
return hipErrorInvalidValue;
414+
break;
361415
}
362416

363-
if (comgr_status = amd::Comgr::create_data_set(&bc_data_set);
364-
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
417+
if (comgr_status = bc_data_set.Create(); comgr_status != AMD_COMGR_STATUS_SUCCESS) {
365418
LogError("Failed to create bitcode data set");
366-
(void)amd::Comgr::destroy_action_info(action);
367-
(void)amd::Comgr::release_data(spirv_data);
368-
(void)amd::Comgr::destroy_data_set(spirv_data_set);
369-
return hipErrorInvalidValue;
419+
break;
370420
}
371421

372-
if (comgr_status = amd::Comgr::do_action(AMD_COMGR_ACTION_TRANSLATE_SPIRV_TO_BC, action,
373-
spirv_data_set, bc_data_set);
422+
if (comgr_status = amd::Comgr::do_action(AMD_COMGR_ACTION_TRANSLATE_SPIRV_TO_BC, action.get(),
423+
spirv_data_set.get(), bc_data_set.get());
374424
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
375425
LogError("Failed to compile to ll");
376-
(void)amd::Comgr::destroy_action_info(action);
377-
(void)amd::Comgr::release_data(spirv_data);
378-
(void)amd::Comgr::destroy_data_set(spirv_data_set);
379-
(void)amd::Comgr::destroy_data_set(bc_data_set);
380-
return hipErrorInvalidValue;
426+
break;
381427
}
382428

383-
(void)amd::Comgr::release_data(spirv_data);
384-
(void)amd::Comgr::destroy_data_set(spirv_data_set);
385-
(void)amd::Comgr::destroy_action_info(action);
386-
387429
// System might report multiple devices of same name, we do not want to recompile for all
388430
// these. store code objects after compiling them to reuse.
389431
std::unordered_map<std::string, std::pair<char*, size_t>> compiled_co;
@@ -402,160 +444,112 @@ hipError_t FatBinaryInfo::ExtractFatBinaryUsingCOMGR(const std::vector<hip::Devi
402444
}
403445

404446
LogPrintfInfo("Creating ISA for: %s from spirv", target_id.c_str());
405-
amd_comgr_action_info_t reloc_action;
447+
comgr_helper::ComgrActionInfoUniqueHandle reloc_action;
406448
std::string isa = "amdgcn-amd-amdhsa--" + target_id;
407-
if (comgr_status = amd::Comgr::create_action_info(&reloc_action);
408-
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
449+
if (comgr_status = reloc_action.Create(); comgr_status != AMD_COMGR_STATUS_SUCCESS) {
409450
LogError("Failed to create action");
410-
return hipErrorInvalidValue;
451+
break;
411452
}
412453

413454
// TODO: do this for all devices
414-
if (comgr_status = amd::Comgr::action_info_set_isa_name(reloc_action, isa.c_str());
455+
if (comgr_status = amd::Comgr::action_info_set_isa_name(reloc_action.get(), isa.c_str());
415456
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
416-
(void)amd::Comgr::destroy_action_info(reloc_action);
417-
(void)amd::Comgr::destroy_data_set(bc_data_set);
418457
LogError("Failed to set ISA name");
419-
return hipErrorInvalidValue;
458+
break;
420459
}
421460

422-
if (comgr_status = amd::Comgr::action_info_set_device_lib_linking(reloc_action, true);
461+
if (comgr_status = amd::Comgr::action_info_set_device_lib_linking(reloc_action.get(), true);
423462
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
424-
(void)amd::Comgr::destroy_action_info(reloc_action);
425-
(void)amd::Comgr::destroy_data_set(bc_data_set);
426463
LogError("Failed to set device lib linking");
427-
return hipErrorInvalidValue;
464+
break;
428465
}
429466

430467
if (comgr_status = amd::Comgr::action_info_set_option_list(
431-
reloc_action, nullptr /* options list */, 0 /* options size */);
468+
reloc_action.get(), nullptr /* options list */, 0 /* options size */);
432469
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
433-
(void)amd::Comgr::destroy_action_info(reloc_action);
434-
(void)amd::Comgr::destroy_data_set(bc_data_set);
435470
LogError("Failed to set option list");
436-
return hipErrorInvalidValue;
471+
break;
437472
}
438473

439-
amd_comgr_data_set_t reloc_data;
440-
if (comgr_status = amd::Comgr::create_data_set(&reloc_data);
441-
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
442-
(void)amd::Comgr::destroy_action_info(reloc_action);
443-
(void)amd::Comgr::destroy_data_set(bc_data_set);
474+
comgr_helper::ComgrDataSetUniqueHandle reloc_data;
475+
if (comgr_status = reloc_data.Create(); comgr_status != AMD_COMGR_STATUS_SUCCESS) {
444476
LogError("Failed to create reloc data set");
445-
return hipErrorInvalidValue;
477+
break;
446478
}
447479

448-
if (comgr_status = amd::Comgr::do_action(AMD_COMGR_ACTION_CODEGEN_BC_TO_RELOCATABLE,
449-
reloc_action, bc_data_set, reloc_data);
480+
if (comgr_status =
481+
amd::Comgr::do_action(AMD_COMGR_ACTION_CODEGEN_BC_TO_RELOCATABLE,
482+
reloc_action.get(), bc_data_set.get(), reloc_data.get());
450483
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
451484
LogError("Failed to compile to reloc");
452-
(void)amd::Comgr::destroy_action_info(reloc_action);
453-
(void)amd::Comgr::destroy_data_set(reloc_data);
454-
(void)amd::Comgr::destroy_data_set(bc_data_set);
455485
LogError("Failed to do action: codegen bc ot reloc");
456-
return hipErrorInvalidValue;
486+
break;
457487
}
458488

459-
amd_comgr_action_info_t exe_action;
460-
amd_comgr_data_set_t exe_output;
489+
comgr_helper::ComgrActionInfoUniqueHandle exe_action;
490+
comgr_helper::ComgrDataSetUniqueHandle exe_output;
461491

462-
if (comgr_status = amd::Comgr::create_action_info(&exe_action);
463-
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
492+
if (comgr_status = exe_action.Create(); comgr_status != AMD_COMGR_STATUS_SUCCESS) {
464493
LogError("Failed to create action");
465-
(void)amd::Comgr::destroy_action_info(reloc_action);
466-
(void)amd::Comgr::destroy_data_set(reloc_data);
467-
(void)amd::Comgr::destroy_data_set(bc_data_set);
468494
LogError("Failed to create exe action");
469-
return hipErrorInvalidValue;
495+
break;
470496
}
471497

472-
if (comgr_status =
473-
amd::Comgr::action_info_set_isa_name(exe_action, isa.c_str());
498+
if (comgr_status = amd::Comgr::action_info_set_isa_name(exe_action.get(), isa.c_str());
474499
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
475-
(void)amd::Comgr::destroy_action_info(exe_action);
476-
(void)amd::Comgr::destroy_action_info(reloc_action);
477-
(void)amd::Comgr::destroy_data_set(reloc_data);
478-
(void)amd::Comgr::destroy_data_set(bc_data_set);
479500
LogError("Failed to set exe action isa name");
480-
return hipErrorInvalidValue;
481501
}
482502

483-
if (comgr_status = amd::Comgr::create_data_set(&exe_output);
484-
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
485-
(void)amd::Comgr::destroy_action_info(exe_action);
486-
(void)amd::Comgr::destroy_action_info(reloc_action);
487-
(void)amd::Comgr::destroy_data_set(reloc_data);
488-
(void)amd::Comgr::destroy_data_set(bc_data_set);
503+
if (comgr_status = exe_output.Create(); comgr_status != AMD_COMGR_STATUS_SUCCESS) {
489504
LogError("Failed to create exe output");
490-
return hipErrorInvalidValue;
505+
break;
491506
}
492507

493-
if (comgr_status = amd::Comgr::do_action(AMD_COMGR_ACTION_LINK_RELOCATABLE_TO_EXECUTABLE,
494-
exe_action, reloc_data, exe_output);
508+
if (comgr_status =
509+
amd::Comgr::do_action(AMD_COMGR_ACTION_LINK_RELOCATABLE_TO_EXECUTABLE,
510+
exe_action.get(), reloc_data.get(), exe_output.get());
495511
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
496-
(void)amd::Comgr::destroy_action_info(exe_action);
497-
(void)amd::Comgr::destroy_action_info(reloc_action);
498-
(void)amd::Comgr::destroy_data_set(exe_output);
499-
(void)amd::Comgr::destroy_data_set(reloc_data);
500-
(void)amd::Comgr::destroy_data_set(bc_data_set);
501512
LogError("Failed to do action: reloc to exe");
502-
return hipErrorInvalidValue;
513+
break;
503514
}
504515

505-
amd_comgr_data_t exe_data;
506-
if (auto res = amd::Comgr::action_data_get_data(exe_output, AMD_COMGR_DATA_KIND_EXECUTABLE,
507-
0, &exe_data);
508-
res != AMD_COMGR_STATUS_SUCCESS) {
509-
(void)amd::Comgr::destroy_action_info(exe_action);
510-
(void)amd::Comgr::destroy_action_info(reloc_action);
511-
(void)amd::Comgr::destroy_data_set(exe_output);
512-
(void)amd::Comgr::destroy_data_set(reloc_data);
513-
(void)amd::Comgr::destroy_data_set(bc_data_set);
516+
amd_comgr_data_t exe_data_handle;
517+
if (comgr_status = amd::Comgr::action_data_get_data(
518+
exe_output.get(), AMD_COMGR_DATA_KIND_EXECUTABLE, 0, &exe_data_handle);
519+
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
514520
LogError("Failed to action get exe data");
515-
return hipErrorInvalidValue;
521+
break;
516522
}
523+
// Move ownership of exe_data_handle to exe_data
524+
comgr_helper::ComgrDataUniqueHandle exe_data(exe_data_handle);
517525

518526
size_t co_size;
519-
if (auto res = amd::Comgr::get_data(exe_data, &co_size, NULL);
520-
res != AMD_COMGR_STATUS_SUCCESS) {
521-
(void)amd::Comgr::destroy_action_info(exe_action);
522-
(void)amd::Comgr::destroy_action_info(reloc_action);
523-
(void)amd::Comgr::destroy_data_set(exe_output);
524-
(void)amd::Comgr::destroy_data_set(reloc_data);
525-
(void)amd::Comgr::destroy_data_set(bc_data_set);
527+
if (comgr_status = amd::Comgr::get_data(exe_data.get(), &co_size, NULL);
528+
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
526529
LogError("Failed to get exe size");
527-
return hipErrorInvalidValue;
530+
break;
528531
}
529532

530533
char* co = new char[co_size];
531-
if (auto res = amd::Comgr::get_data(exe_data, &co_size, co);
532-
res != AMD_COMGR_STATUS_SUCCESS) {
533-
(void)amd::Comgr::destroy_action_info(exe_action);
534-
(void)amd::Comgr::destroy_action_info(reloc_action);
535-
(void)amd::Comgr::destroy_data_set(exe_output);
536-
(void)amd::Comgr::destroy_data_set(reloc_data);
537-
(void)amd::Comgr::destroy_data_set(bc_data_set);
534+
if (comgr_status = amd::Comgr::get_data(exe_data.get(), &co_size, co);
535+
comgr_status != AMD_COMGR_STATUS_SUCCESS) {
538536
LogError("Failed to get exe data");
539-
return hipErrorInvalidValue;
537+
break;
540538
}
541539

542540
auto elf_size = CodeObject::ElfSize(co);
543541
fatbin_dev_info_[device->deviceId()] = new FatBinaryDeviceInfo(co, elf_size, 0);
544542
fatbin_dev_info_[device->deviceId()]->program_ = new amd::Program(*(device->asContext()));
545543
// Save the compiled code object
546544
compiled_co[target_id] = std::make_pair(co, elf_size);
547-
548-
// cleanup
549-
(void)amd::Comgr::release_data(exe_data);
550-
(void)amd::Comgr::destroy_action_info(exe_action);
551-
(void)amd::Comgr::destroy_action_info(reloc_action);
552-
(void)amd::Comgr::destroy_data_set(exe_output);
553-
(void)amd::Comgr::destroy_data_set(reloc_data);
554545
}
555-
(void)amd::Comgr::destroy_data_set(bc_data_set);
556546
}
557547
} while (0);
558548

549+
if (comgr_status != AMD_COMGR_STATUS_SUCCESS) {
550+
hip_status = hipErrorInvalidValue;
551+
}
552+
559553
// Clean up file and memory resouces if hip_status failed for some reason.
560554
if (hip_status != hipSuccess && hip_status != hipErrorInvalidKernelFile) {
561555
if (image_mapped_) {

0 commit comments

Comments
 (0)