From 70b78ac7fac99938644a404a1aeff275b7bf0fb8 Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Thu, 30 May 2019 17:53:11 -0400 Subject: [PATCH] Fix crash on pipeline cache merge after VkShaderModule destroyed. Set MTLFunction label immediately after creation from MVKShaderModule. Remove _shaderModule reference from MVKShaderLibrary & MVKShaderLibraryCache. No-op propogateDebugName() function in MVKShaderModule & MVKPipelineCache. --- .../MoltenVK/Commands/MVKCommandBuffer.mm | 2 +- MoltenVK/MoltenVK/GPUObjects/MVKDevice.mm | 2 +- MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h | 2 +- MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm | 8 --- .../MoltenVK/GPUObjects/MVKShaderModule.h | 15 +----- .../MoltenVK/GPUObjects/MVKShaderModule.mm | 52 +++---------------- 6 files changed, 11 insertions(+), 70 deletions(-) diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm b/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm index 7df3be2a..48fdab74 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm +++ b/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm @@ -177,7 +177,7 @@ void MVKCommandBuffer::clearPrefilledMTLCommandBuffer() { #pragma mark Construction -// Initializes this instance after it has been created retrieved from a pool. +// Initializes this instance after it has been created or retrieved from a pool. void MVKCommandBuffer::init(const VkCommandBufferAllocateInfo* pAllocateInfo) { _commandPool = (MVKCommandPool*)pAllocateInfo->commandPool; _isSecondary = (pAllocateInfo->level == VK_COMMAND_BUFFER_LEVEL_SECONDARY); diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKDevice.mm b/MoltenVK/MoltenVK/GPUObjects/MVKDevice.mm index aadd5fc1..a6c18f94 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKDevice.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKDevice.mm @@ -1839,7 +1839,7 @@ VkResult MVKDevice::createPipelines(VkPipelineCache pipelineCache, } // Create concrete implementations of the two variations of the mvkCreatePipelines() function -// that we will be using. This is required since the template definition is location in this +// that we will be using. This is required since the template definition is located in this // implementation file instead of in the header file. This is a realistic approach if the // universe of possible template implementation variations is small and known in advance. template VkResult MVKDevice::createPipelines(VkPipelineCache pipelineCache, diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h index 9e680505..f8c50842 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h @@ -365,7 +365,7 @@ public: ~MVKPipelineCache() override; protected: - void propogateDebugName() override; + void propogateDebugName() override {} MVKShaderLibraryCache* getShaderLibraryCache(MVKShaderModuleKey smKey); void readData(const VkPipelineCacheCreateInfo* pCreateInfo); void writeData(std::ostream& outstream, bool isCounting = false); diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm index 21976c60..abf1bddb 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm @@ -1268,20 +1268,12 @@ MVKComputePipeline::~MVKComputePipeline() { #pragma mark - #pragma mark MVKPipelineCache - -void MVKPipelineCache::propogateDebugName() { - lock_guard lock(_shaderCacheLock); - - for (auto& slPair : _shaderCache) { slPair.second->propogateDebugName(); } -} - // Return a shader library from the specified shader context sourced from the specified shader module. MVKShaderLibrary* MVKPipelineCache::getShaderLibrary(SPIRVToMSLConverterContext* pContext, MVKShaderModule* shaderModule) { lock_guard lock(_shaderCacheLock); bool wasAdded = false; MVKShaderLibraryCache* slCache = getShaderLibraryCache(shaderModule->getKey()); - slCache->setShaderModule(shaderModule); MVKShaderLibrary* shLib = slCache->getShaderLibrary(pContext, shaderModule, &wasAdded); if (wasAdded) { markDirty(); } return shLib; diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.h b/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.h index 1fd16704..bfd4d59e 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.h @@ -75,15 +75,11 @@ protected: friend MVKShaderLibraryCache; friend MVKShaderModule; - void propogateDebugName(); - NSString* getDebugName(); - void setShaderModule(MVKShaderModule* shaderModule); - MVKMTLFunction getMTLFunction(const VkSpecializationInfo* pSpecializationInfo); + MVKMTLFunction getMTLFunction(const VkSpecializationInfo* pSpecializationInfo, MVKShaderModule* shaderModule); void handleCompilationError(NSError* err, const char* opDesc); MTLFunctionConstant* getFunctionConstant(NSArray* mtlFCs, NSUInteger mtlFCID); MVKVulkanAPIDeviceObject* _owner; - MVKShaderModule* _shaderModule = nullptr; id _mtlLibrary; SPIRVEntryPoint _entryPoint; std::string _msl; @@ -111,11 +107,6 @@ public: MVKShaderLibrary* getShaderLibrary(SPIRVToMSLConverterContext* pContext, MVKShaderModule* shaderModule, bool* pWasAdded = nullptr); - /** - * Sets the shader module associated with this library cache. - * This is set after creation because libraries can be loaded from a pipeline cache. - */ - void setShaderModule(MVKShaderModule* shaderModule); MVKShaderLibraryCache(MVKVulkanAPIDeviceObject* owner) : _owner(owner) {}; @@ -126,7 +117,6 @@ protected: friend MVKPipelineCache; friend MVKShaderModule; - void propogateDebugName(); MVKShaderLibrary* findShaderLibrary(SPIRVToMSLConverterContext* pContext); MVKShaderLibrary* addShaderLibrary(SPIRVToMSLConverterContext* pContext, const std::string& mslSourceCode, @@ -134,7 +124,6 @@ protected: void merge(MVKShaderLibraryCache* other); MVKVulkanAPIDeviceObject* _owner; - MVKShaderModule* _shaderModule = nullptr; std::vector> _shaderLibraries; }; @@ -208,7 +197,7 @@ public: protected: friend MVKShaderCacheIterator; - void propogateDebugName() override; + void propogateDebugName() override {} MVKGLSLConversionShaderStage getMVKGLSLConversionShaderStage(SPIRVToMSLConverterContext* pContext); MVKShaderLibraryCache _shaderLibraryCache; diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm b/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm index 974a29dd..999b337c 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm @@ -31,16 +31,6 @@ const MVKMTLFunction MVKMTLFunctionNull = { nil, MTLSizeMake(1, 1, 1) }; #pragma mark - #pragma mark MVKShaderLibrary -void MVKShaderLibrary::propogateDebugName() { setLabelIfNotNil(_mtlLibrary, getDebugName()); } - -// First choice is to name after shader module if it exists, otherwise name after owner. -NSString* MVKShaderLibrary::getDebugName() { - NSString* dbName = _shaderModule ? _shaderModule-> getDebugName() : nil; - if (dbName) { return dbName; } - - return _owner ? _owner-> getDebugName() : nil; -} - // If the size of the workgroup dimension is specialized, extract it from the // specialization info, otherwise use the value specified in the SPIR-V shader code. static uint32_t getWorkgroupDimensionSize(const SPIRVWorkgroupSizeDimension& wgDim, const VkSpecializationInfo* pSpecInfo) { @@ -56,7 +46,7 @@ static uint32_t getWorkgroupDimensionSize(const SPIRVWorkgroupSizeDimension& wgD return wgDim.size; } -MVKMTLFunction MVKShaderLibrary::getMTLFunction(const VkSpecializationInfo* pSpecializationInfo) { +MVKMTLFunction MVKShaderLibrary::getMTLFunction(const VkSpecializationInfo* pSpecializationInfo, MVKShaderModule* shaderModule) { if ( !_mtlLibrary ) { return MVKMTLFunctionNull; } @@ -101,7 +91,10 @@ MVKMTLFunction MVKShaderLibrary::getMTLFunction(const VkSpecializationInfo* pSpe reportError(VK_ERROR_INVALID_SHADER_NV, "Shader module does not contain an entry point named '%s'.", mtlFuncName.UTF8String); } - setLabelIfNotNil(mtlFunc, getDebugName()); + // Set the debug name. First try name of shader module, otherwise try name of owner. + NSString* dbName = shaderModule-> getDebugName(); + if ( !dbName ) { dbName = _owner-> getDebugName(); } + setLabelIfNotNil(mtlFunc, dbName); return { mtlFunc, MTLSizeMake(getWorkgroupDimensionSize(_entryPoint.workgroupSize.width, pSpecializationInfo), getWorkgroupDimensionSize(_entryPoint.workgroupSize.height, pSpecializationInfo), @@ -116,18 +109,10 @@ MTLFunctionConstant* MVKShaderLibrary::getFunctionConstant(NSArraynewMTLLibrary(@(mslSourceCode.c_str())); // retained slc->destroy(); - propogateDebugName(); _entryPoint = entryPoint; _msl = mslSourceCode; @@ -148,7 +133,6 @@ MVKShaderLibrary::MVKShaderLibrary(MVKVulkanAPIDeviceObject* owner, handleCompilationError(err, "Compiled shader module creation"); [shdrData release]; } - propogateDebugName(); mvkDev->addActivityPerformance(mvkDev->_performanceStatistics.shaderCompilation.mslLoad, startTime); } @@ -156,7 +140,6 @@ MVKShaderLibrary::MVKShaderLibrary(MVKShaderLibrary& other) : _owner(other._owne _mtlLibrary = [other._mtlLibrary retain]; _entryPoint = other._entryPoint; _msl = other._msl; - setShaderModule(other._shaderModule); } // If err object is nil, the compilation succeeded without any warnings. @@ -183,10 +166,6 @@ MVKShaderLibrary::~MVKShaderLibrary() { #pragma mark - #pragma mark MVKShaderLibraryCache -void MVKShaderLibraryCache::propogateDebugName() { - for (auto& slPair : _shaderLibraries) { slPair.second->propogateDebugName(); } -} - MVKShaderLibrary* MVKShaderLibraryCache::getShaderLibrary(SPIRVToMSLConverterContext* pContext, MVKShaderModule* shaderModule, bool* pWasAdded) { @@ -221,7 +200,6 @@ MVKShaderLibrary* MVKShaderLibraryCache::addShaderLibrary(SPIRVToMSLConverterCon const string& mslSourceCode, const SPIRVEntryPoint& entryPoint) { MVKShaderLibrary* shLib = new MVKShaderLibrary(_owner, mslSourceCode, entryPoint); - shLib->setShaderModule(_shaderModule); _shaderLibraries.emplace_back(*pContext, shLib); return shLib; } @@ -236,13 +214,6 @@ void MVKShaderLibraryCache::merge(MVKShaderLibraryCache* other) { } } -void MVKShaderLibraryCache::setShaderModule(MVKShaderModule* shaderModule) { - if (shaderModule == _shaderModule) { return; } - - _shaderModule = shaderModule; - for (auto& slPair : _shaderLibraries) { slPair.second->setShaderModule(_shaderModule); } -} - MVKShaderLibraryCache::~MVKShaderLibraryCache() { for (auto& slPair : _shaderLibraries) { slPair.second->destroy(); } } @@ -251,13 +222,6 @@ MVKShaderLibraryCache::~MVKShaderLibraryCache() { #pragma mark - #pragma mark MVKShaderModule -void MVKShaderModule::propogateDebugName() { - lock_guard lock(_accessLock); - - _shaderLibraryCache.propogateDebugName(); - if (_defaultLibrary) { _defaultLibrary->propogateDebugName(); } -} - MVKMTLFunction MVKShaderModule::getMTLFunction(SPIRVToMSLConverterContext* pContext, const VkSpecializationInfo* pSpecializationInfo, MVKPipelineCache* pipelineCache) { @@ -276,7 +240,7 @@ MVKMTLFunction MVKShaderModule::getMTLFunction(SPIRVToMSLConverterContext* pCont pContext->markAllAttributesAndResourcesUsed(); } - return mvkLib ? mvkLib->getMTLFunction(pSpecializationInfo) : MVKMTLFunctionNull; + return mvkLib ? mvkLib->getMTLFunction(pSpecializationInfo, this) : MVKMTLFunctionNull; } bool MVKShaderModule::convert(SPIRVToMSLConverterContext* pContext) { @@ -335,8 +299,6 @@ MVKShaderModule::MVKShaderModule(MVKDevice* device, const VkShaderModuleCreateInfo* pCreateInfo) : MVKVulkanAPIDeviceObject(device), _shaderLibraryCache(this) { _defaultLibrary = nullptr; - _shaderLibraryCache.setShaderModule(this); - size_t codeSize = pCreateInfo->codeSize; @@ -374,7 +336,6 @@ MVKShaderModule::MVKShaderModule(MVKDevice* device, _spvConverter.setMSL(pMSLCode, nullptr); _defaultLibrary = new MVKShaderLibrary(this, _spvConverter.getMSL().c_str(), _spvConverter.getEntryPoint()); - _defaultLibrary->setShaderModule(this); break; } @@ -389,7 +350,6 @@ MVKShaderModule::MVKShaderModule(MVKDevice* device, _device->addActivityPerformance(_device->_performanceStatistics.shaderCompilation.hashShaderCode, startTime); _defaultLibrary = new MVKShaderLibrary(this, (void*)(pMSLCode), mslCodeLen); - _defaultLibrary->setShaderModule(this); break; }