From e23fcf4343bfe2c2af75bdeb5e994909272608cd Mon Sep 17 00:00:00 2001 From: foreman Date: Mon, 27 Aug 2018 18:46:34 -0400 Subject: [PATCH] P4 to Git Change 1598666 by cpaquot@cpaquot-ocl-lc-lnx on 2018/08/27 18:36:44 SWDEV-145570 - [HIP] - Multithreading issues Add a lock per function so kernel parameters don't get overwritten Make execStack_ thread local and remove global lock use for it: The compiler uses the same thread to set it up and launch the function Affected files ... ... //depot/stg/opencl/drivers/opencl/api/hip/hip_internal.hpp#16 edit ... //depot/stg/opencl/drivers/opencl/api/hip/hip_module.cpp#18 edit ... //depot/stg/opencl/drivers/opencl/api/hip/hip_platform.cpp#19 edit [ROCm/clr commit: 556ef9c0d1cc565e5c8febd5492c6e2e56406710] --- projects/clr/hipamd/api/hip/hip_internal.hpp | 11 +++++++ projects/clr/hipamd/api/hip/hip_module.cpp | 8 +++-- projects/clr/hipamd/api/hip/hip_platform.cpp | 34 +++++++++++--------- 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/projects/clr/hipamd/api/hip/hip_internal.hpp b/projects/clr/hipamd/api/hip/hip_internal.hpp index 325f0b7cd3..a59ec04649 100644 --- a/projects/clr/hipamd/api/hip/hip_internal.hpp +++ b/projects/clr/hipamd/api/hip/hip_internal.hpp @@ -59,6 +59,17 @@ namespace hip { extern amd::HostQueue* getNullStream(); extern void syncStreams(); + + + struct Function { + amd::Kernel* function_; + amd::Monitor lock_; + + Function(amd::Kernel* f) : function_(f), lock_("function lock") {} + hipFunction_t asHipFunction() { return reinterpret_cast(this); } + + static Function* asFunction(hipFunction_t f) { return reinterpret_cast(f); } + }; }; extern std::vector g_devices; extern hipError_t ihipDeviceGetCount(int* count); diff --git a/projects/clr/hipamd/api/hip/hip_module.cpp b/projects/clr/hipamd/api/hip/hip_module.cpp index 9830a97dfb..389119c128 100644 --- a/projects/clr/hipamd/api/hip/hip_module.cpp +++ b/projects/clr/hipamd/api/hip/hip_module.cpp @@ -126,7 +126,8 @@ hipError_t hipModuleGetFunction(hipFunction_t *hfunc, hipModule_t hmod, const ch HIP_RETURN(hipErrorOutOfMemory); } - *hfunc = reinterpret_cast(as_cl(kernel)); + hip::Function* f = new hip::Function(kernel); + *hfunc = f->asHipFunction(); HIP_RETURN(hipSuccess); } @@ -151,9 +152,12 @@ hipError_t ihipModuleLaunchKernel(hipFunction_t f, sharedMemBytes, hStream, kernelParams, extra, startEvent, stopEvent); - amd::Kernel* kernel = as_amd(reinterpret_cast(f)); + hip::Function* function = hip::Function::asFunction(f); + amd::Kernel* kernel = function->function_; amd::Device* device = hip::getCurrentContext()->devices()[0]; + amd::ScopedLock lock(function->lock_); + hip::Event* eStart = reinterpret_cast(startEvent); hip::Event* eStop = reinterpret_cast(stopEvent); amd::HostQueue* queue; diff --git a/projects/clr/hipamd/api/hip/hip_platform.cpp b/projects/clr/hipamd/api/hip/hip_platform.cpp index ba0d50865d..c4c2e7d1cb 100644 --- a/projects/clr/hipamd/api/hip/hip_platform.cpp +++ b/projects/clr/hipamd/api/hip/hip_platform.cpp @@ -110,11 +110,12 @@ struct ihipExec_t { std::vector arguments_; }; +thread_local std::stack execStack_; + class PlatformState { amd::Monitor lock_; - - std::stack execStack_; - std::map functions_; +private: + std::unordered_map functions_; struct RegisteredVar { char* var; @@ -124,11 +125,16 @@ class PlatformState { bool constant; }; - std::map vars_; + std::unordered_map vars_; static PlatformState* platform_; PlatformState() : lock_("Guards global function map") {} + ~PlatformState() { + for (const auto it : functions_) { + delete it.second; + } + } public: static PlatformState& instance() { return *platform_; @@ -147,13 +153,14 @@ public: vars_.insert(std::make_pair(modules, rvar)); } - void registerFunction(const void* hostFunction, hipFunction_t func) { + void registerFunction(const void* hostFunction, amd::Kernel* func) { amd::ScopedLock lock(lock_); - functions_.insert(std::make_pair(hostFunction, func)); + hip::Function* f = new hip::Function(func); + functions_.insert(std::make_pair(hostFunction, f)); } - hipFunction_t getFunc(const void* hostFunction) { + hip::Function* getFunc(const void* hostFunction) { amd::ScopedLock lock(lock_); const auto it = functions_.find(hostFunction); if (it != functions_.cend()) { @@ -166,8 +173,6 @@ public: void setupArgument(const void *arg, size_t size, size_t offset) { - amd::ScopedLock lock(lock_); - auto& arguments = execStack_.top().arguments_; if (arguments.size() < offset + size) { @@ -181,12 +186,10 @@ public: dim3 blockDim, size_t sharedMem, hipStream_t stream) { - amd::ScopedLock lock(lock_); execStack_.push(ihipExec_t{gridDim, blockDim, sharedMem, stream}); } void popExec(ihipExec_t& exec) { - amd::ScopedLock lock(lock_); exec = std::move(execStack_.top()); execStack_.pop(); } @@ -215,7 +218,7 @@ extern "C" void __hipRegisterFunction( amd::Kernel* kernel = new amd::Kernel(*program, *symbol, deviceName); if (!kernel) return; - PlatformState::instance().registerFunction(hostFunction, reinterpret_cast(as_cl(kernel))); + PlatformState::instance().registerFunction(hostFunction, kernel); } // Registers a device-side global variable. @@ -274,7 +277,7 @@ extern "C" hipError_t hipLaunchByPtr(const void *hostFunction) { HIP_INIT_API(hostFunction); - hipFunction_t func = PlatformState::instance().getFunc(hostFunction); + hip::Function* func = PlatformState::instance().getFunc(hostFunction); if (func == nullptr) { HIP_RETURN(hipErrorUnknown); } @@ -282,13 +285,14 @@ extern "C" hipError_t hipLaunchByPtr(const void *hostFunction) ihipExec_t exec; PlatformState::instance().popExec(exec); + size_t size = exec.arguments_.size(); void *extra[] = { HIP_LAUNCH_PARAM_BUFFER_POINTER, &exec.arguments_[0], - HIP_LAUNCH_PARAM_BUFFER_SIZE, 0 /* FIXME: not needed, but should be correct*/, + HIP_LAUNCH_PARAM_BUFFER_SIZE, &size, HIP_LAUNCH_PARAM_END }; - HIP_RETURN(hipModuleLaunchKernel(func, + HIP_RETURN(hipModuleLaunchKernel(func->asHipFunction(), exec.gridDim_.x, exec.gridDim_.y, exec.gridDim_.z, exec.blockDim_.x, exec.blockDim_.y, exec.blockDim_.z, exec.sharedMem_, exec.hStream_, nullptr, extra));