From 22fafb8e38439f203a70988e6455b2d2ce13bd43 Mon Sep 17 00:00:00 2001 From: assiduous Date: Fri, 29 Jan 2021 20:36:32 -0800 Subject: Pipeline resource signature impl: improved validation of PipelineResourceDesc.Flags parameter --- .../src/PipelineResourceSignatureBase.cpp | 84 ++++++++++++++++++---- .../src/PipelineResourceSignatureVkImpl.cpp | 48 +++++++++---- .../src/PipelineStateVkImpl.cpp | 3 + 3 files changed, 106 insertions(+), 29 deletions(-) (limited to 'Graphics') diff --git a/Graphics/GraphicsEngine/src/PipelineResourceSignatureBase.cpp b/Graphics/GraphicsEngine/src/PipelineResourceSignatureBase.cpp index ebe45d02..b6a5cfb8 100644 --- a/Graphics/GraphicsEngine/src/PipelineResourceSignatureBase.cpp +++ b/Graphics/GraphicsEngine/src/PipelineResourceSignatureBase.cpp @@ -68,20 +68,76 @@ void ValidatePipelineResourceSignatureDesc(const PipelineResourceSignatureDesc& } UsedStages |= Res.ShaderStages; - if ((Res.Flags & PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS) && - (Res.ResourceType != SHADER_RESOURCE_TYPE_CONSTANT_BUFFER && - Res.ResourceType != SHADER_RESOURCE_TYPE_BUFFER_UAV && - Res.ResourceType != SHADER_RESOURCE_TYPE_BUFFER_SRV)) - LOG_PRS_ERROR_AND_THROW("Desc.Resources[", i, "].Flags must not contain PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS if ResourceType is not buffer"); - - if ((Res.Flags & PIPELINE_RESOURCE_FLAG_COMBINED_SAMPLER) && - Res.ResourceType != SHADER_RESOURCE_TYPE_TEXTURE_SRV) - LOG_PRS_ERROR_AND_THROW("Desc.Resources[", i, "].Flags must not contain PIPELINE_RESOURCE_FLAG_COMBINED_SAMPLER if ResourceType is not SHADER_RESOURCE_TYPE_TEXTURE_SRV"); - - if ((Res.Flags & PIPELINE_RESOURCE_FLAG_FORMATTED_BUFFER) && - (Res.ResourceType != SHADER_RESOURCE_TYPE_BUFFER_UAV && - Res.ResourceType != SHADER_RESOURCE_TYPE_BUFFER_SRV)) - LOG_PRS_ERROR_AND_THROW("Desc.Resources[", i, "].Flags must not contain PIPELINE_RESOURCE_FLAG_FORMATTED_BUFFER if ResourceType is not buffer"); + static_assert(SHADER_RESOURCE_TYPE_LAST == 8, "Please add the new resource type to the switch below"); + switch (Res.ResourceType) + { + case SHADER_RESOURCE_TYPE_CONSTANT_BUFFER: + if ((Res.Flags & ~PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS) != 0) + { + LOG_PRS_ERROR_AND_THROW("Incorrect Desc.Resources[", i, "].Flags (", GetPipelineResourceFlagsString(Res.Flags), + "): NO_DYNAMIC_BUFFERS is the only valid flag for a constant buffer."); + } + break; + + case SHADER_RESOURCE_TYPE_TEXTURE_SRV: + if ((Res.Flags & ~PIPELINE_RESOURCE_FLAG_COMBINED_SAMPLER) != 0) + { + LOG_PRS_ERROR_AND_THROW("Incorrect Desc.Resources[", i, "].Flags (", GetPipelineResourceFlagsString(Res.Flags), + "): COMBINED_SAMPLER is the only valid flag for a texture SRV."); + } + break; + + case SHADER_RESOURCE_TYPE_BUFFER_SRV: + if ((Res.Flags & ~(PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS | PIPELINE_RESOURCE_FLAG_FORMATTED_BUFFER)) != 0) + { + LOG_PRS_ERROR_AND_THROW("Incorrect Desc.Resources[", i, "].Flags (", GetPipelineResourceFlagsString(Res.Flags), + "): NO_DYNAMIC_BUFFERS and FORMATTED_BUFFER are the only valid flags for a buffer SRV."); + } + break; + + case SHADER_RESOURCE_TYPE_TEXTURE_UAV: + if (Res.Flags != PIPELINE_RESOURCE_FLAG_UNKNOWN) + { + LOG_PRS_ERROR_AND_THROW("Incorrect Desc.Resources[", i, "].Flags (", GetPipelineResourceFlagsString(Res.Flags), + "): UNKNOWN is the only valid flag for a texture UAV."); + } + break; + + case SHADER_RESOURCE_TYPE_BUFFER_UAV: + if ((Res.Flags & ~(PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS | PIPELINE_RESOURCE_FLAG_FORMATTED_BUFFER)) != 0) + { + LOG_PRS_ERROR_AND_THROW("Incorrect Desc.Resources[", i, "].Flags (", GetPipelineResourceFlagsString(Res.Flags), + "): NO_DYNAMIC_BUFFERS and FORMATTED_BUFFER are the only valid flags for a buffer UAV."); + } + break; + + case SHADER_RESOURCE_TYPE_SAMPLER: + if (Res.Flags != PIPELINE_RESOURCE_FLAG_UNKNOWN) + { + LOG_PRS_ERROR_AND_THROW("Incorrect Desc.Resources[", i, "].Flags (", GetPipelineResourceFlagsString(Res.Flags), + "): UNKNOWN is the only valid flag for a sampler."); + } + break; + + case SHADER_RESOURCE_TYPE_INPUT_ATTACHMENT: + if (Res.Flags != PIPELINE_RESOURCE_FLAG_UNKNOWN) + { + LOG_PRS_ERROR_AND_THROW("Incorrect Desc.Resources[", i, "].Flags (", GetPipelineResourceFlagsString(Res.Flags), + "): UNKNOWN is the only valid flag for an input attachment."); + } + break; + + case SHADER_RESOURCE_TYPE_ACCEL_STRUCT: + if (Res.Flags != PIPELINE_RESOURCE_FLAG_UNKNOWN) + { + LOG_PRS_ERROR_AND_THROW("Incorrect Desc.Resources[", i, "].Flags (", GetPipelineResourceFlagsString(Res.Flags), + "): UNKNOWN is the only valid flag for an acceleration structure."); + } + break; + + default: + UNEXPECTED("Unexpected resource type"); + } } if (Desc.UseCombinedTextureSamplers) diff --git a/Graphics/GraphicsEngineVulkan/src/PipelineResourceSignatureVkImpl.cpp b/Graphics/GraphicsEngineVulkan/src/PipelineResourceSignatureVkImpl.cpp index 8d9714df..775f9df5 100644 --- a/Graphics/GraphicsEngineVulkan/src/PipelineResourceSignatureVkImpl.cpp +++ b/Graphics/GraphicsEngineVulkan/src/PipelineResourceSignatureVkImpl.cpp @@ -106,37 +106,53 @@ DescriptorType GetDescriptorType(const PipelineResourceDesc& Res) switch (Res.ResourceType) { case SHADER_RESOURCE_TYPE_CONSTANT_BUFFER: - VERIFY_EXPR((Res.Flags & ~PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS) == 0); + VERIFY((Res.Flags & ~PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS) == 0, + "NO_DYNAMIC_BUFFERS is the only valid flag allowed for constant buffers. " + "This error should've been caught by ValidatePipelineResourceSignatureDesc."); return WithDynamicOffset ? DescriptorType::UniformBufferDynamic : DescriptorType::UniformBuffer; - case SHADER_RESOURCE_TYPE_BUFFER_UAV: - VERIFY_EXPR((Res.Flags & ~(PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS | PIPELINE_RESOURCE_FLAG_FORMATTED_BUFFER)) == 0); - return UseTexelBuffer ? DescriptorType::StorageTexelBuffer : - (WithDynamicOffset ? DescriptorType::StorageBufferDynamic : DescriptorType::StorageBuffer); - case SHADER_RESOURCE_TYPE_TEXTURE_SRV: - VERIFY_EXPR((Res.Flags & ~PIPELINE_RESOURCE_FLAG_COMBINED_SAMPLER) == 0); + VERIFY((Res.Flags & ~PIPELINE_RESOURCE_FLAG_COMBINED_SAMPLER) == 0, + "COMBINED_SAMPLER is the only valid flag for a texture SRV. " + "This error should've been caught by ValidatePipelineResourceSignatureDesc."); return CombinedSampler ? DescriptorType::CombinedImageSampler : DescriptorType::SeparateImage; case SHADER_RESOURCE_TYPE_BUFFER_SRV: - VERIFY_EXPR((Res.Flags & ~(PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS | PIPELINE_RESOURCE_FLAG_FORMATTED_BUFFER)) == 0); + VERIFY((Res.Flags & ~(PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS | PIPELINE_RESOURCE_FLAG_FORMATTED_BUFFER)) == 0, + "NO_DYNAMIC_BUFFERS and FORMATTED_BUFFER are the only valid flags for a buffer SRV. " + "This error should've been caught by ValidatePipelineResourceSignatureDesc."); return UseTexelBuffer ? DescriptorType::UniformTexelBuffer : (WithDynamicOffset ? DescriptorType::StorageBufferDynamic_ReadOnly : DescriptorType::StorageBuffer_ReadOnly); case SHADER_RESOURCE_TYPE_TEXTURE_UAV: - VERIFY_EXPR(Res.Flags == PIPELINE_RESOURCE_FLAG_UNKNOWN); + VERIFY(Res.Flags == PIPELINE_RESOURCE_FLAG_UNKNOWN, + "UNKNOWN is the only valid flag for a texture UAV. " + "This error should've been caught by ValidatePipelineResourceSignatureDesc."); return DescriptorType::StorageImage; + case SHADER_RESOURCE_TYPE_BUFFER_UAV: + VERIFY((Res.Flags & ~(PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS | PIPELINE_RESOURCE_FLAG_FORMATTED_BUFFER)) == 0, + "NO_DYNAMIC_BUFFERS and FORMATTED_BUFFER are the only valid flags for a buffer UAV. " + "This should've been caught by ValidatePipelineResourceSignatureDesc."); + return UseTexelBuffer ? DescriptorType::StorageTexelBuffer : + (WithDynamicOffset ? DescriptorType::StorageBufferDynamic : DescriptorType::StorageBuffer); + case SHADER_RESOURCE_TYPE_SAMPLER: - VERIFY_EXPR(Res.Flags == PIPELINE_RESOURCE_FLAG_UNKNOWN); + VERIFY(Res.Flags == PIPELINE_RESOURCE_FLAG_UNKNOWN, + "UNKNOWN is the only valid flag for a sampler. " + "This error should've been caught by ValidatePipelineResourceSignatureDesc."); return DescriptorType::Sampler; case SHADER_RESOURCE_TYPE_INPUT_ATTACHMENT: - VERIFY_EXPR(Res.Flags == PIPELINE_RESOURCE_FLAG_UNKNOWN); + VERIFY(Res.Flags == PIPELINE_RESOURCE_FLAG_UNKNOWN, + "UNKNOWN is the only valid flag for an input attachment. " + "This error should've been caught by ValidatePipelineResourceSignatureDesc."); return DescriptorType::InputAttachment; case SHADER_RESOURCE_TYPE_ACCEL_STRUCT: - VERIFY_EXPR(Res.Flags == PIPELINE_RESOURCE_FLAG_UNKNOWN); + VERIFY(Res.Flags == PIPELINE_RESOURCE_FLAG_UNKNOWN, + "UNKNOWN is the only valid flag for an acceleration structure. " + "This error should've been caught by ValidatePipelineResourceSignatureDesc."); return DescriptorType::AccelerationStructure; default: @@ -1681,13 +1697,15 @@ bool PipelineResourceSignatureVkImpl::DvpValidateCommittedResource(const SPIRVSh const auto CacheType = ResourceCache.GetContentType(); const auto CacheOffset = ResAttribs.CacheOffset(CacheType); + VERIFY_EXPR(SPIRVAttribs.ArraySize <= ResAttribs.ArraySize); + switch (ResAttribs.GetDescriptorType()) { case DescriptorType::UniformBuffer: case DescriptorType::UniformBufferDynamic: { VERIFY_EXPR(ResInfo.ResourceType == SHADER_RESOURCE_TYPE_CONSTANT_BUFFER); - for (Uint32 i = 0; i < ResAttribs.ArraySize; ++i) + for (Uint32 i = 0; i < SPIRVAttribs.ArraySize; ++i) { const auto& Res = DescrSetResources.GetResource(CacheOffset + i); @@ -1719,7 +1737,7 @@ bool PipelineResourceSignatureVkImpl::DvpValidateCommittedResource(const SPIRVSh case DescriptorType::StorageBufferDynamic_ReadOnly: { VERIFY_EXPR(ResInfo.ResourceType == SHADER_RESOURCE_TYPE_BUFFER_UAV || ResInfo.ResourceType == SHADER_RESOURCE_TYPE_BUFFER_SRV); - for (Uint32 i = 0; i < ResAttribs.ArraySize; ++i) + for (Uint32 i = 0; i < SPIRVAttribs.ArraySize; ++i) { const auto& Res = DescrSetResources.GetResource(CacheOffset + i); @@ -1766,7 +1784,7 @@ bool PipelineResourceSignatureVkImpl::DvpValidateCommittedResource(const SPIRVSh case DescriptorType::CombinedImageSampler: { VERIFY_EXPR(ResInfo.ResourceType == SHADER_RESOURCE_TYPE_TEXTURE_SRV || ResInfo.ResourceType == SHADER_RESOURCE_TYPE_TEXTURE_UAV); - for (Uint32 i = 0; i < ResAttribs.ArraySize; ++i) + for (Uint32 i = 0; i < SPIRVAttribs.ArraySize; ++i) { const auto& Res = DescrSetResources.GetResource(CacheOffset + i); // When can use raw cast here because the dynamic type is verified when the resource diff --git a/Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp b/Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp index c7e81769..f1bbfdaa 100644 --- a/Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp +++ b/Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp @@ -813,7 +813,10 @@ void PipelineStateVkImpl::InitPipelineLayout(const PipelineStateCreateInfo& Crea if (Resources.size()) { + String SignName = String{"Implicit signature for PSO '"} + m_Desc.Name + '\''; + PipelineResourceSignatureDesc ResSignDesc; + ResSignDesc.Name = SignName.c_str(); ResSignDesc.Resources = Resources.data(); ResSignDesc.NumResources = static_cast(Resources.size()); ResSignDesc.ImmutableSamplers = LayoutDesc.ImmutableSamplers; -- cgit v1.2.3