From e5431676d4d0c7972f6f9c5af0d935ffa6ea67d5 Mon Sep 17 00:00:00 2001 From: Tony Tye Date: Sun, 10 Jan 2021 02:09:00 +0000 Subject: [PATCH] Add stronger checking - Add assertions to enforce that objects are of the correct kind and have been allocated. - Make destructors check if objects have been allocated before deleting. - Operations that require a non-NullDevice return failure if given a NullDevice. - Use static_cast rather than reinterpret_cast when cohersing from a base class to a derived class. Change-Id: I02ee0ea9d7982fd7ca29d49c9b02cfae111b7127 --- rocclr/device/device.cpp | 13 ++++++--- rocclr/device/gpu/gpuprogram.cpp | 11 +++++++- rocclr/device/gpu/gpuprogram.hpp | 2 ++ rocclr/device/pal/palkernel.cpp | 3 ++ rocclr/device/pal/palkernel.hpp | 5 +++- rocclr/device/pal/palprogram.cpp | 46 ++++++++++++++++++++++++++++--- rocclr/device/pal/palprogram.hpp | 2 ++ rocclr/device/rocm/rocdevice.cpp | 1 + rocclr/device/rocm/rocdevice.hpp | 2 +- rocclr/device/rocm/rocprogram.cpp | 14 ++++++++++ rocclr/device/rocm/rocprogram.hpp | 10 +++++-- 11 files changed, 96 insertions(+), 13 deletions(-) diff --git a/rocclr/device/device.cpp b/rocclr/device/device.cpp index daf91fa7a5..593a7f3e35 100644 --- a/rocclr/device/device.cpp +++ b/rocclr/device/device.cpp @@ -268,10 +268,14 @@ Device::Device() } Device::~Device() { - CondLog((vaCacheMap_ != nullptr) && (vaCacheMap_->size() != 0), - "Application didn't unmap all host memory!"); - delete vaCacheMap_; - delete vaCacheAccess_; + if (vaCacheMap_) { + CondLog(vaCacheMap_->size() != 0, "Application didn't unmap all host memory!"); + delete vaCacheMap_; + } + + if (vaCacheAccess_) { + delete vaCacheAccess_; + } // Destroy device settings if (settings_ != nullptr) { @@ -297,6 +301,7 @@ bool Device::ValidateComgr() { } bool Device::create() { + assert(!vaCacheAccess_ && !vaCacheMap_); vaCacheAccess_ = new amd::Monitor("VA Cache Ops Lock", true); if (nullptr == vaCacheAccess_) { return false; diff --git a/rocclr/device/gpu/gpuprogram.cpp b/rocclr/device/gpu/gpuprogram.cpp index 0e1cd6bd94..391fc3ddc4 100644 --- a/rocclr/device/gpu/gpuprogram.cpp +++ b/rocclr/device/gpu/gpuprogram.cpp @@ -1505,6 +1505,7 @@ HSAILProgram::HSAILProgram(Device& device, amd::Program& owner) maxScratchRegs_(0), executable_(NULL), loaderContext_(this) { + assert(device.isOnline()); machineTarget_ = gpuNullDevice().hwInfo()->targetName_; loader_ = amd::hsa::loader::Loader::Create(&loaderContext_); } @@ -1516,9 +1517,12 @@ HSAILProgram::HSAILProgram(NullDevice& device, amd::Program& owner) maxScratchRegs_(0), executable_(NULL), loaderContext_(this) { + assert(!device.isOnline()); isNull_ = true; machineTarget_ = gpuNullDevice().hwInfo()->targetName_; - loader_ = amd::hsa::loader::Loader::Create(&loaderContext_); + + // Cannot load onto a NullDevice. + loader_ = nullptr; } HSAILProgram::~HSAILProgram() { @@ -1736,6 +1740,11 @@ std::string HSAILProgram::hsailOptions() { } bool HSAILProgram::allocKernelTable() { + if (isNull()) { + // Cannot create a kernel table for offline devices. + return false; + } + uint size = kernels().size() * sizeof(size_t); kernels_ = new gpu::Memory(gpuDevice(), size); diff --git a/rocclr/device/gpu/gpuprogram.hpp b/rocclr/device/gpu/gpuprogram.hpp index 05f4569a5c..ad25f3e3b0 100644 --- a/rocclr/device/gpu/gpuprogram.hpp +++ b/rocclr/device/gpu/gpuprogram.hpp @@ -349,6 +349,7 @@ class Program : public NullProgram { //! Return a typecasted GPU device gpu::Device& gpuDevice() { + assert(!isNull()); return const_cast(static_cast(device())); } @@ -476,6 +477,7 @@ class HSAILProgram : public device::Program { //! Return a typecasted GPU device. The device must not be the NullDevice. gpu::Device& gpuDevice() { + assert(!isNull()); return const_cast(static_cast(device())); } diff --git a/rocclr/device/pal/palkernel.cpp b/rocclr/device/pal/palkernel.cpp index 7fcdb6b91f..bae85abc8a 100644 --- a/rocclr/device/pal/palkernel.cpp +++ b/rocclr/device/pal/palkernel.cpp @@ -68,6 +68,9 @@ void HSAILKernel::setWorkGroupInfo(const uint32_t privateSegmentSize, } bool HSAILKernel::setKernelCode(amd::hsa::loader::Symbol* sym, amd_kernel_code_t* akc) { + if (prog().isNull()) { + return false; + } if (!sym) { return false; } diff --git a/rocclr/device/pal/palkernel.hpp b/rocclr/device/pal/palkernel.hpp index 71b5373c03..0a432062bd 100644 --- a/rocclr/device/pal/palkernel.hpp +++ b/rocclr/device/pal/palkernel.hpp @@ -72,7 +72,10 @@ class HSAILKernel : public device::Kernel { const NullDevice& palNullDevice() const { return reinterpret_cast(dev_); } //! Returns PAL device object, associated with this kernel which must not be the null device. - const Device& palDevice() const { return reinterpret_cast(dev_); } + const Device& palDevice() const { + assert(dev_.isOnline()); + return reinterpret_cast(dev_); + } //! Returns HSA program associated with this kernel const HSAILProgram& prog() const; diff --git a/rocclr/device/pal/palprogram.cpp b/rocclr/device/pal/palprogram.cpp index 76eecb8690..954ff9396b 100644 --- a/rocclr/device/pal/palprogram.cpp +++ b/rocclr/device/pal/palprogram.cpp @@ -68,6 +68,11 @@ bool Segment::gpuAddressOffset(uint64_t offAddr, size_t* offset) { bool Segment::alloc(HSAILProgram& prog, amdgpu_hsa_elf_segment_t segment, size_t size, size_t align, bool zero) { + if (prog.isNull()) { + LogError("[OCL] cannot create a mem object on an offline device!"); + return false; + } + align = amd::alignUp(align, sizeof(uint32_t)); amd::Memory* amd_mem_obj = new (prog.palDevice().context()) @@ -178,6 +183,7 @@ HSAILProgram::HSAILProgram(Device& device, amd::Program& owner) maxScratchRegs_(0), executable_(nullptr), loaderContext_(this) { + assert(device.isOnline()); if (dev().asicRevision() == Pal::AsicRevision::Bristol) { machineTarget_ = Carrizo; } else { @@ -195,13 +201,15 @@ HSAILProgram::HSAILProgram(NullDevice& device, amd::Program& owner) maxScratchRegs_(0), executable_(nullptr), loaderContext_(this) { + assert(!device.isOnline()); isNull_ = true; if (dev().asicRevision() == Pal::AsicRevision::Bristol) { machineTarget_ = Carrizo; } else { machineTarget_ = dev().hwInfo()->machineTarget_; } - loader_ = amd::hsa::loader::Loader::Create(&loaderContext_); + // Cannot load onto a NullDevice. + loader_ = nullptr; } HSAILProgram::~HSAILProgram() { @@ -223,11 +231,15 @@ HSAILProgram::~HSAILProgram() { } #endif // defined(WITH_COMPILER_LIB) releaseClBinary(); - if (executable_ != nullptr) { + if (executable_) { loader_->DestroyExecutable(executable_); } - delete kernels_; - amd::hsa::loader::Loader::Destroy(loader_); + if (kernels_) { + delete kernels_; + } + if (loader_) { + amd::hsa::loader::Loader::Destroy(loader_); + } } @@ -242,6 +254,12 @@ inline static std::vector splitSpaceSeparatedString(char* str) { bool HSAILProgram::setKernels(amd::option::Options* options, void* binary, size_t binSize, amd::Os::FileDesc fdesc, size_t foffset, std::string uri) { #if defined(WITH_COMPILER_LIB) + // Stop compilation if it is an offline device - PAL runtime does not + // support ISA compiled offline + if (!device().isOnline()) { + return true; + } + // ACL_TYPE_CG stage is not performed for offline compilation hsa_agent_t agent; agent.handle = 1; @@ -324,6 +342,11 @@ bool HSAILProgram::setKernels(amd::option::Options* options, void* binary, size_ bool HSAILProgram::createBinary(amd::option::Options* options) { return true; } bool HSAILProgram::allocKernelTable() { + if (isNull()) { + // Cannot create a kernel table for offline devices. + return false; + } + uint size = kernels().size() * sizeof(size_t); kernels_ = new pal::Memory(palDevice(), size); @@ -381,6 +404,10 @@ bool HSAILProgram::saveBinaryAndSetType(type_t type) { } bool HSAILProgram::defineGlobalVar(const char* name, void* dptr) { + if (!device().isOnline()) { + return false; + } + hsa_status_t hsa_status = HSA_STATUS_SUCCESS; hsa_agent_t agent; @@ -396,6 +423,10 @@ bool HSAILProgram::defineGlobalVar(const char* name, void* dptr) { bool HSAILProgram::createGlobalVarObj(amd::Memory** amd_mem_obj, void** device_pptr, size_t* bytes, const char* global_name) const { + if (!device().isOnline()) { + return false; + } + uint32_t length = 0; size_t offset = 0; uint32_t flags = 0; @@ -607,6 +638,7 @@ void* PALHSALoaderContext::SegmentHostAddress(amdgpu_hsa_elf_segment_t segment, bool PALHSALoaderContext::SegmentFreeze(amdgpu_hsa_elf_segment_t segment, hsa_agent_t agent, void* seg, size_t size) { + assert(seg); if (program_->isNull()) { return true; } @@ -736,6 +768,12 @@ bool LightningProgram::createBinary(amd::option::Options* options) { bool LightningProgram::setKernels(amd::option::Options* options, void* binary, size_t binSize, amd::Os::FileDesc fdesc, size_t foffset, std::string uri) { #if defined(USE_COMGR_LIBRARY) + // Stop compilation if it is an offline device - PAL runtime does not + // support ISA compiled offline + if (!device().isOnline()) { + return true; + } + hsa_agent_t agent; agent.handle = 1; diff --git a/rocclr/device/pal/palprogram.hpp b/rocclr/device/pal/palprogram.hpp index 57aa9cc40e..58bd76ae4d 100644 --- a/rocclr/device/pal/palprogram.hpp +++ b/rocclr/device/pal/palprogram.hpp @@ -164,6 +164,7 @@ class HSAILProgram : public device::Program { //! Return a typecasted PAL device. The device must not be the NullDevice. pal::Device& palDevice() { + assert(!isNull()); return const_cast(static_cast(device())); } @@ -187,6 +188,7 @@ class HSAILProgram : public device::Program { //! Returns CPU address for a kernel uint64_t findHostKernelAddress(uint64_t devAddr) const { + assert(!isNull()); return loader_->FindHostAddress(devAddr); } diff --git a/rocclr/device/rocm/rocdevice.cpp b/rocclr/device/rocm/rocdevice.cpp index df452aeb1f..d6e9f101bc 100644 --- a/rocclr/device/rocm/rocdevice.cpp +++ b/rocclr/device/rocm/rocdevice.cpp @@ -598,6 +598,7 @@ bool Device::create() { } // Create HSA settings + assert(!settings_); settings_ = new Settings(); roc::Settings* hsaSettings = static_cast(settings_); if ((hsaSettings == nullptr) || diff --git a/rocclr/device/rocm/rocdevice.hpp b/rocclr/device/rocm/rocdevice.hpp index 39ffcfd84e..7e4847e625 100644 --- a/rocclr/device/rocm/rocdevice.hpp +++ b/rocclr/device/rocm/rocdevice.hpp @@ -122,7 +122,7 @@ class NullDevice : public amd::Device { Compiler* compiler() const { return compilerHandle_; } - const Settings& settings() const { return reinterpret_cast(*settings_); } + const Settings& settings() const { return static_cast(*settings_); } //! Construct an HSAIL program object from the ELF assuming it is valid virtual device::Program* createProgram(amd::Program& owner, amd::option::Options* options = nullptr); diff --git a/rocclr/device/rocm/rocprogram.cpp b/rocclr/device/rocm/rocprogram.cpp index cb766f9aba..6d4e0850c0 100644 --- a/rocclr/device/rocm/rocprogram.cpp +++ b/rocclr/device/rocm/rocprogram.cpp @@ -126,6 +126,10 @@ bool Program::initClBinary(char* binaryIn, size_t size) { bool Program::defineGlobalVar(const char* name, void* dptr) { + if (!device().isOnline()) { + return false; + } + hsa_status_t status = HSA_STATUS_SUCCESS; hsa_agent_t hsa_device = rocDevice().getBackendDevice(); @@ -141,6 +145,10 @@ bool Program::defineGlobalVar(const char* name, void* dptr) { bool Program::createGlobalVarObj(amd::Memory** amd_mem_obj, void** device_pptr, size_t* bytes, const char* global_name) const { + if (!device().isOnline()) { + return false; + } + hsa_status_t status = HSA_STATUS_SUCCESS; const roc::Device* roc_device = nullptr; hsa_agent_t hsa_device; @@ -470,6 +478,12 @@ bool LightningProgram::saveBinaryAndSetType(type_t type, void* rawBinary, size_t bool LightningProgram::setKernels(amd::option::Options* options, void* binary, size_t binSize, amd::Os::FileDesc fdesc, size_t foffset, std::string uri) { #if defined(USE_COMGR_LIBRARY) + // Stop compilation if it is an offline device - HSA runtime does not + // support ISA compiled offline + if (!device().isOnline()) { + return true; + } + // Find the size of global variables from the binary if (!FindGlobalVarSize(binary, binSize)) { buildLog_ += "Error: Cannot Global Var Sizes "; diff --git a/rocclr/device/rocm/rocprogram.hpp b/rocclr/device/rocm/rocprogram.hpp index f8f9691419..cc74aa6320 100644 --- a/rocclr/device/rocm/rocprogram.hpp +++ b/rocclr/device/rocm/rocprogram.hpp @@ -52,12 +52,18 @@ class Program : public device::Program { const NullDevice& rocNullDevice() const { return static_cast(device()); } //! Return a typecasted GPU device - const Device& rocDevice() const { return static_cast(device()); } + const Device& rocDevice() const { + assert(!isNull()); + return static_cast(device()); + } //! Returns the hsaBinary associated with the program hsa_agent_t hsaDevice() const { return rocNullDevice().getBackendDevice(); } - hsa_executable_t hsaExecutable() const { return hsaExecutable_; } + hsa_executable_t hsaExecutable() const { + assert(!isNull()); + return hsaExecutable_; + } virtual bool createGlobalVarObj(amd::Memory** amd_mem_obj, void** dptr, size_t* bytes, const char* globalName) const;