Added debug checks to verify that all resources in ShaderResouceCacheVk have been initialized

7 files changed, 69 insertions, 33 deletions

diff --git a/Graphics/GraphicsEngineVulkan/include/PipelineLayout.h b/Graphics/GraphicsEngineVulkan/include/PipelineLayout.h
index a875cb90..a5aec374 100644
--- a/Graphics/GraphicsEngineVulkan/include/PipelineLayout.h
+++ b/Graphics/GraphicsEngineVulkan/include/PipelineLayout.h
@@ -59,7 +59,7 @@ public:
 
     void AllocateResourceSlot(const SPIRVShaderResourceAttribs& ResAttribs, 
                               SHADER_RESOURCE_VARIABLE_TYPE     VariableType,
-                              VkSampler                         vkStaticSampler,
+                              VkSampler                         vkImmutableSampler,
                               SHADER_TYPE                       ShaderType, 
                               Uint32&                           DescriptorSet, 
                               Uint32&                           Binding,
@@ -189,7 +189,7 @@ private:
 
         void AllocateResourceSlot(const SPIRVShaderResourceAttribs& ResAttribs,
                                   SHADER_RESOURCE_VARIABLE_TYPE     VariableType,
-                                  VkSampler                         vkStaticSampler,
+                                  VkSampler                         vkImmutableSampler,
                                   SHADER_TYPE                       ShaderType,
                                   Uint32&                           DescriptorSet,
                                   Uint32&                           Binding, 
diff --git a/Graphics/GraphicsEngineVulkan/include/ShaderResourceCacheVk.h b/Graphics/GraphicsEngineVulkan/include/ShaderResourceCacheVk.h
index 0843939f..1bedbcac 100644
--- a/Graphics/GraphicsEngineVulkan/include/ShaderResourceCacheVk.h
+++ b/Graphics/GraphicsEngineVulkan/include/ShaderResourceCacheVk.h
@@ -49,6 +49,7 @@
 // Descriptor set for static and mutable resources is assigned during cache initialization
 // Descriptor set for dynamic resources is assigned at every draw call
 
+#include <vector>
 #include "DescriptorPoolManager.h"
 #include "SPIRVShaderResources.h"
 
@@ -171,6 +172,7 @@ public:
 #ifdef _DEBUG
     // Only for debug purposes: indicates what types of resources are stored in the cache
     DbgCacheContentType DbgGetContentType()const{return m_DbgContentType;}
+    void DbgVerifyResourceInitialization()const;
 #endif
 
     template<bool VerifyOnly>
@@ -192,7 +194,9 @@ private:
 
 #ifdef _DEBUG
     // Only for debug purposes: indicates what types of resources are stored in the cache
-    const DbgCacheContentType m_DbgContentType;
+    const DbgCacheContentType      m_DbgContentType;
+    // Debug array that stores flags indicating if resources in the cache have been initialized
+    std::vector<std::vector<bool>> m_DbgInitializedResources;
 #endif
 };
 
diff --git a/Graphics/GraphicsEngineVulkan/include/ShaderResourceLayoutVk.h b/Graphics/GraphicsEngineVulkan/include/ShaderResourceLayoutVk.h
index b9f0ea10..a33f9841 100644
--- a/Graphics/GraphicsEngineVulkan/include/ShaderResourceLayoutVk.h
+++ b/Graphics/GraphicsEngineVulkan/include/ShaderResourceLayoutVk.h
@@ -107,7 +107,7 @@ namespace Diligent
 {
 
 /// Diligent::ShaderResourceLayoutVk class
-// sizeof(ShaderResourceLayoutVk)==56 (MS compiler, x64) - TODO: verify
+// sizeof(ShaderResourceLayoutVk)==56 (MS compiler, x64)
 class ShaderResourceLayoutVk
 {
 public:
@@ -338,15 +338,16 @@ private:
         return reinterpret_cast<ImmutableSamplerPtrType*>(ResourceMemoryEnd)[n];
     }
 
-    const VulkanUtilities::VulkanLogicalDevice&         m_LogicalDevice;
-    std::unique_ptr<void, STDDeleterRawMem<void> >      m_ResourceBuffer;
+/* 0 */ const VulkanUtilities::VulkanLogicalDevice&         m_LogicalDevice;
+/* 8 */ std::unique_ptr<void, STDDeleterRawMem<void> >      m_ResourceBuffer;
 
-    // We must use shared_ptr to reference ShaderResources instance, because
-    // there may be multiple objects referencing the same set of resources
-    std::shared_ptr<const SPIRVShaderResources>         m_pResources;
+        // We must use shared_ptr to reference ShaderResources instance, because
+        // there may be multiple objects referencing the same set of resources
+/*24 */ std::shared_ptr<const SPIRVShaderResources>         m_pResources;
 
-    std::array<Uint16, SHADER_RESOURCE_VARIABLE_TYPE_NUM_TYPES+1>  m_NumResources = {};
-    Uint32 m_NumImmutableSamplers = 0;
+/*40 */ std::array<Uint16, SHADER_RESOURCE_VARIABLE_TYPE_NUM_TYPES+1>  m_NumResources = {};
+/*48 */ Uint32 m_NumImmutableSamplers = 0;
+/*56*/  // End of class
 };
 
 }
diff --git a/Graphics/GraphicsEngineVulkan/src/PipelineLayout.cpp b/Graphics/GraphicsEngineVulkan/src/PipelineLayout.cpp
index ef7fd8ec..d8383418 100644
--- a/Graphics/GraphicsEngineVulkan/src/PipelineLayout.cpp
+++ b/Graphics/GraphicsEngineVulkan/src/PipelineLayout.cpp
@@ -316,7 +316,7 @@ size_t PipelineLayout::DescriptorSetLayoutManager::GetHash()const
 
 void PipelineLayout::DescriptorSetLayoutManager::AllocateResourceSlot(const SPIRVShaderResourceAttribs& ResAttribs,
                                                                       SHADER_RESOURCE_VARIABLE_TYPE     VariableType,
-                                                                      VkSampler                         vkStaticSampler,
+                                                                      VkSampler                         vkImmutableSampler,
                                                                       SHADER_TYPE                       ShaderType,
                                                                       Uint32&                           DescriptorSet,
                                                                       Uint32&                           Binding,
@@ -336,15 +336,15 @@ void PipelineLayout::DescriptorSetLayoutManager::AllocateResourceSlot(const SPIR
     VkBinding.descriptorCount = ResAttribs.ArraySize;
     // There are no limitations on what combinations of stages can use a descriptor binding (13.2.1)
     VkBinding.stageFlags = ShaderTypeToVkShaderStageFlagBit(ShaderType);
-    if (vkStaticSampler != VK_NULL_HANDLE)
+    if (vkImmutableSampler != VK_NULL_HANDLE)
     {
         // If descriptorType is VK_DESCRIPTOR_TYPE_SAMPLER or VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER, and 
         // descriptorCount is not 0 and pImmutableSamplers is not NULL, pImmutableSamplers must be a valid pointer 
         // to an array of descriptorCount valid VkSampler handles (13.2.1)
-        auto *pStaticSamplers = reinterpret_cast<VkSampler*>(ALLOCATE(m_MemAllocator, "Memory buffer for immutable samplers", sizeof(VkSampler) * VkBinding.descriptorCount));
+        auto *pImmutableSamplers = reinterpret_cast<VkSampler*>(ALLOCATE(m_MemAllocator, "Memory buffer for immutable samplers", sizeof(VkSampler) * VkBinding.descriptorCount));
         for(uint32_t s=0; s < VkBinding.descriptorCount; ++s)
-            pStaticSamplers[s] = vkStaticSampler;
-        VkBinding.pImmutableSamplers = pStaticSamplers;
+            pImmutableSamplers[s] = vkImmutableSampler;
+        VkBinding.pImmutableSamplers = pImmutableSamplers;
     }
     else
         VkBinding.pImmutableSamplers = nullptr;
@@ -366,7 +366,7 @@ void PipelineLayout::Release(RenderDeviceVkImpl *pDeviceVkImpl, Uint64 CommandQu
 
 void PipelineLayout::AllocateResourceSlot(const SPIRVShaderResourceAttribs& ResAttribs,
                                           SHADER_RESOURCE_VARIABLE_TYPE     VariableType,
-                                          VkSampler                         vkStaticSampler,
+                                          VkSampler                         vkImmutableSampler,
                                           SHADER_TYPE                       ShaderType,
                                           Uint32&                           DescriptorSet, // Output parameter
                                           Uint32&                           Binding, // Output parameter
@@ -374,9 +374,9 @@ void PipelineLayout::AllocateResourceSlot(const SPIRVShaderResourceAttribs& ResA
                                           std::vector<uint32_t>&            SPIRV)
 {
     VERIFY( (ResAttribs.Type == SPIRVShaderResourceAttribs::ResourceType::SampledImage || 
-             ResAttribs.Type == SPIRVShaderResourceAttribs::ResourceType::SeparateSampler) || vkStaticSampler == VK_NULL_HANDLE,
+             ResAttribs.Type == SPIRVShaderResourceAttribs::ResourceType::SeparateSampler) || vkImmutableSampler == VK_NULL_HANDLE,
             "Immutable sampler should only be specified for combined image samplers or separate samplers");
-    m_LayoutMgr.AllocateResourceSlot(ResAttribs, VariableType,vkStaticSampler, ShaderType, DescriptorSet, Binding, OffsetInCache);
+    m_LayoutMgr.AllocateResourceSlot(ResAttribs, VariableType ,vkImmutableSampler, ShaderType, DescriptorSet, Binding, OffsetInCache);
     SPIRV[ResAttribs.BindingDecorationOffset] = Binding;
     SPIRV[ResAttribs.DescriptorSetDecorationOffset] = DescriptorSet;
 }
diff --git a/Graphics/GraphicsEngineVulkan/src/ShaderResourceBindingVkImpl.cpp b/Graphics/GraphicsEngineVulkan/src/ShaderResourceBindingVkImpl.cpp
index 54b09c69..0193bb55 100644
--- a/Graphics/GraphicsEngineVulkan/src/ShaderResourceBindingVkImpl.cpp
+++ b/Graphics/GraphicsEngineVulkan/src/ShaderResourceBindingVkImpl.cpp
@@ -65,6 +65,10 @@ ShaderResourceBindingVkImpl::ShaderResourceBindingVkImpl(IReferenceCounters* pRe
         const SHADER_RESOURCE_VARIABLE_TYPE VarTypes[] = {SHADER_RESOURCE_VARIABLE_TYPE_MUTABLE, SHADER_RESOURCE_VARIABLE_TYPE_DYNAMIC};
         new (m_pShaderVarMgrs + s) ShaderVariableManagerVk(*this, SrcLayout, VarDataAllocator, VarTypes, _countof(VarTypes), m_ShaderResourceCache);
     }
+#ifdef _DEBUG
+    m_ShaderResourceCache.DbgVerifyResourceInitialization();
+#endif
+
 }
 
 ShaderResourceBindingVkImpl::~ShaderResourceBindingVkImpl()
diff --git a/Graphics/GraphicsEngineVulkan/src/ShaderResourceCacheVk.cpp b/Graphics/GraphicsEngineVulkan/src/ShaderResourceCacheVk.cpp
index 47d098f9..2bfa922e 100644
--- a/Graphics/GraphicsEngineVulkan/src/ShaderResourceCacheVk.cpp
+++ b/Graphics/GraphicsEngineVulkan/src/ShaderResourceCacheVk.cpp
@@ -57,13 +57,16 @@ void ShaderResourceCacheVk::InitializeSets(IMemoryAllocator& MemAllocator, Uint3
     //  Ns = m_NumSets
 
     VERIFY(m_pAllocator == nullptr && m_pMemory == nullptr, "Cache already initialized");
-    m_pAllocator = &MemAllocator;
-    m_NumSets = NumSets;
+    m_pAllocator     = &MemAllocator;
+    m_NumSets        = NumSets;
     m_TotalResources = 0;
     for (Uint32 t=0; t < NumSets; ++t)
         m_TotalResources += SetSizes[t];
     auto MemorySize = NumSets * sizeof(DescriptorSet) + m_TotalResources * sizeof(Resource);
     VERIFY_EXPR(MemorySize == GetRequiredMemorySize(NumSets, SetSizes));
+#ifdef _DEBUG
+    m_DbgInitializedResources.resize(m_NumSets);
+#endif
     if (MemorySize > 0)
     {
         m_pMemory = ALLOCATE( *m_pAllocator, "Memory for shader resource cache data", MemorySize);
@@ -73,6 +76,9 @@ void ShaderResourceCacheVk::InitializeSets(IMemoryAllocator& MemAllocator, Uint3
         {
             new(&GetDescriptorSet(t)) DescriptorSet(SetSizes[t], SetSizes[t] > 0 ? pCurrResPtr : nullptr);
             pCurrResPtr += SetSizes[t];
+#ifdef _DEBUG
+            m_DbgInitializedResources[t].resize(SetSizes[t]);
+#endif
         }
         VERIFY_EXPR((char*)pCurrResPtr == (char*)m_pMemory + MemorySize);
     }
@@ -82,9 +88,25 @@ void ShaderResourceCacheVk::InitializeResources(Uint32 Set, Uint32 Offset, Uint3
 {
     auto& DescrSet = GetDescriptorSet(Set);
     for (Uint32 res = 0; res < ArraySize; ++res)
+    {
         new(&DescrSet.GetResource(Offset + res)) Resource{Type};
+#ifdef _DEBUG
+        m_DbgInitializedResources[Set][Offset + res] = true;
+#endif
+    }
 }
 
+#ifdef _DEBUG
+void ShaderResourceCacheVk::DbgVerifyResourceInitialization()const
+{
+    for (const auto &SetFlags : m_DbgInitializedResources)
+    {
+        for (auto ResInitialized : SetFlags)
+            VERIFY(ResInitialized, "Not all resources in the cache have been initialized. This is a bug.");
+    }
+}
+#endif
+
 ShaderResourceCacheVk::~ShaderResourceCacheVk()
 {
     if (m_pMemory)
diff --git a/Graphics/GraphicsEngineVulkan/src/ShaderResourceLayoutVk.cpp b/Graphics/GraphicsEngineVulkan/src/ShaderResourceLayoutVk.cpp
index bc6c0a54..3e1aba42 100644
--- a/Graphics/GraphicsEngineVulkan/src/ShaderResourceLayoutVk.cpp
+++ b/Graphics/GraphicsEngineVulkan/src/ShaderResourceLayoutVk.cpp
@@ -93,9 +93,8 @@ static SHADER_RESOURCE_VARIABLE_TYPE GetShaderVariableType(SHADER_TYPE
 
 ShaderResourceLayoutVk::~ShaderResourceLayoutVk()
 {
-    auto* Resources = reinterpret_cast<VkResource*>(m_ResourceBuffer.get());
     for (Uint32 r=0; r < GetTotalResourceCount(); ++r)
-        Resources[r].~VkResource();
+        GetResource(r).~VkResource();
 
     for(Uint32 s=0; s < m_NumImmutableSamplers; ++s)
         GetImmutableSampler(s).~ImmutableSamplerPtrType();
@@ -116,14 +115,17 @@ void ShaderResourceLayoutVk::AllocateMemory(std::shared_ptr<const SPIRVShaderRes
     const Uint32 AllowedTypeBits = GetAllowedTypeBits(AllowedVarTypes, NumAllowedTypes);
     const auto ShaderType = m_pResources->GetShaderType();
     const auto* CombinedSamplerSuffix = m_pResources->GetCombinedSamplerSuffix();
-    // Count number of resources to allocate all needed memory
+    // Count the number of resources to allocate all needed memory
     m_pResources->ProcessResources(
         [&](const SPIRVShaderResourceAttribs& ResAttribs, Uint32)
         {
             auto VarType = GetShaderVariableType(ShaderType, ResAttribs, ResourceLayoutDesc, CombinedSamplerSuffix);
             if (IsAllowedType(VarType, AllowedTypeBits))
             {
-                VERIFY( Uint32{m_NumResources[VarType]} + 1 <= Uint32{std::numeric_limits<Uint16>::max()}, "Number of resources exceeds max representable value");
+                // For immutable separate samplers we still allocate VkResource instances, but they are never used
+                // or exposed to the app
+
+                VERIFY( Uint32{m_NumResources[VarType]} + 1 <= Uint32{std::numeric_limits<Uint16>::max()}, "Number of resources exceeds maximum representable value");
                 ++m_NumResources[VarType];
             }
         }
@@ -134,11 +136,11 @@ void ShaderResourceLayoutVk::AllocateMemory(std::shared_ptr<const SPIRVShaderRes
     {
         TotalResources += m_NumResources[VarType];
     }
-    VERIFY(TotalResources <= Uint32{std::numeric_limits<Uint16>::max()}, "Total number of resources exceeds Uint16 max representable value" );
+    VERIFY(TotalResources <= Uint32{std::numeric_limits<Uint16>::max()}, "Total number of resources exceeds Uint16 maximum representable value" );
     m_NumResources[SHADER_RESOURCE_VARIABLE_TYPE_NUM_TYPES] = static_cast<Uint16>(TotalResources);
 
     m_NumImmutableSamplers = 0;
-    for(Uint32 s=0; s < ResourceLayoutDesc.NumStaticSamplers; ++s)
+    for (Uint32 s=0; s < ResourceLayoutDesc.NumStaticSamplers; ++s)
     {
         const auto& StSamDesc = ResourceLayoutDesc.StaticSamplers[s];
         if ((StSamDesc.ShaderStages & ShaderType) != 0)
@@ -152,7 +154,7 @@ void ShaderResourceLayoutVk::AllocateMemory(std::shared_ptr<const SPIRVShaderRes
 
     auto* pRawMem = ALLOCATE(Allocator, "Raw memory buffer for shader resource layout resources", MemSize);
     m_ResourceBuffer = std::unique_ptr<void, STDDeleterRawMem<void> >(pRawMem, Allocator);
-    for(Uint32 s=0; s < m_NumImmutableSamplers; ++s)
+    for (Uint32 s=0; s < m_NumImmutableSamplers; ++s)
     {
         // We need to initialize immutable samplers
         auto& UninitializedImmutableSampler = GetImmutableSampler(s);
@@ -189,10 +191,10 @@ void ShaderResourceLayoutVk::InitializeStaticResourceLayout(std::shared_ptr<cons
             {
                 // Only search for the immutable sampler for combined image samplers and separate samplers
                 SrcImmutableSamplerInd = FindImmutableSampler(ShaderType, ResourceLayoutDesc, Attribs, CombinedSamplerSuffix);
+                // For immutable separate samplers we allocate VkResource instances, but it is never used
+                // or exposed to the app
             }
 
-            //TODO: how to handle static samplers?
-
             Uint32 Binding       = Attribs.Type;
             Uint32 DescriptorSet = 0;
             Uint32 CacheOffset   = StaticResCacheSize;
@@ -218,6 +220,9 @@ void ShaderResourceLayoutVk::InitializeStaticResourceLayout(std::shared_ptr<cons
 
     StaticResourceCache.InitializeSets(GetRawAllocator(), 1, &StaticResCacheSize);
     InitializeResourceMemoryInCache(StaticResourceCache);
+#ifdef _DEBUG
+    StaticResourceCache.DbgVerifyResourceInitialization();
+#endif
 }
 
 void ShaderResourceLayoutVk::Initialize(IRenderDevice*                               pRenderDevice,
@@ -362,8 +367,8 @@ void ShaderResourceLayoutVk::Initialize(IRenderDevice*
         
         auto& ShaderSPIRV = SPIRVs[ShaderInd];
         PipelineLayout.AllocateResourceSlot(Attribs, VarType, vkImmutableSampler, Resources.GetShaderType(), DescriptorSet, Binding, CacheOffset, ShaderSPIRV);
-        VERIFY(DescriptorSet <= std::numeric_limits<decltype(VkResource::DescriptorSet)>::max(), "Descriptor set (", DescriptorSet, ") excceeds max representable value");
-        VERIFY(Binding <= std::numeric_limits<decltype(VkResource::Binding)>::max(), "Binding (", Binding, ") excceeds max representable value");
+        VERIFY(DescriptorSet <= std::numeric_limits<decltype(VkResource::DescriptorSet)>::max(), "Descriptor set (", DescriptorSet, ") excceeds maximum representable value");
+        VERIFY(Binding       <= std::numeric_limits<decltype(VkResource::Binding)>      ::max(), "Binding (",        Binding,       ") excceeds maximum representable value");
 
 #ifdef _DEBUG
         // Verify that bindings and cache offsets monotonically increase in every descriptor set
@@ -904,7 +909,7 @@ void ShaderResourceLayoutVk::InitializeStaticResources(const ShaderResourceLayou
             
             auto DstOffset = DstRes.CacheOffset + ArrInd;
             IDeviceObject* pCachedResource = DstResourceCache.GetDescriptorSet(DstRes.DescriptorSet).GetResource(DstOffset).pObject;
-            if(pCachedResource != pObject)
+            if (pCachedResource != pObject)
             {
                 VERIFY(pCachedResource == nullptr, "Static resource has already been initialized, and the resource to be assigned from the shader does not match previously assigned resource");
                 DstRes.BindResource(pObject, ArrInd, DstResourceCache);