Fixed GLSL shader compilation to generate bindings and descriptor sets; updated SPIRV resource loading

7 files changed, 195 insertions, 91 deletions

diff --git a/Graphics/GLSLTools/include/SPIRVShaderResources.h b/Graphics/GLSLTools/include/SPIRVShaderResources.h
index 0b740b14..9d952a97 100644
--- a/Graphics/GLSLTools/include/SPIRVShaderResources.h
+++ b/Graphics/GLSLTools/include/SPIRVShaderResources.h
@@ -39,6 +39,12 @@
 #include "STDAllocator.h"
 #include "HashUtils.h"
 
+namespace spirv_cross
+{
+class Compiler;
+struct Resource;
+}
+
 namespace Diligent
 {
 
@@ -73,25 +79,19 @@ struct SPIRVShaderResourceAttribs
     };
 
     const String Name;
-    const Uint8 Binding;
-    const Uint8 Location;
+
+    const Uint16 Binding;
     const Uint16 ArraySize;
-    const ResourceType Type;
-    const SHADER_VARIABLE_TYPE VarType : 8;
-
-    SPIRVShaderResourceAttribs(String _Name,
-                               Uint8 _Binding,
-                               Uint8 _Location,
-                               Uint16 _ArraySize,
-                               ResourceType _Type,
-                               SHADER_VARIABLE_TYPE _VarType) :
-        Name(std::move(_Name)),
-        Binding(_Binding),
-        Location(_Location),
-        ArraySize(_ArraySize),
-        Type(_Type),
-        VarType(_VarType)
-    {}
+
+    // offset in SPIRV words (uint32_t) for a decoration which was originally declared in the SPIRV binary
+    const uint32_t BindingDecorationOffset;
+    const uint32_t DescriptorSetDecorationOffset;
+
+    const Uint8 DescriptorSet;
+    const ResourceType Type : 4;
+    const SHADER_VARIABLE_TYPE VarType : 4;
+
+    SPIRVShaderResourceAttribs(const spirv_cross::Compiler &Compiler, const spirv_cross::Resource &Res, ResourceType _Type, SHADER_VARIABLE_TYPE _VarType);
 
     String GetPrintName(Uint32 ArrayInd)const
     {
@@ -104,10 +104,10 @@ struct SPIRVShaderResourceAttribs
 
     bool IsCompatibleWith(const SPIRVShaderResourceAttribs& Attibs)const
     {
-        return Binding   == Attibs.Binding &&
-               Location  == Attibs.Location &&
-               ArraySize == Attibs.ArraySize && 
-               Type      == Attibs.Type;
+        return Binding       == Attibs.Binding &&
+               DescriptorSet == Attibs.DescriptorSet &&
+               ArraySize     == Attibs.ArraySize && 
+               Type          == Attibs.Type;
     }
 };
 
diff --git a/Graphics/GLSLTools/src/GLSL2SPIRV.cpp b/Graphics/GLSLTools/src/GLSL2SPIRV.cpp
index 69cadd2c..81f2009a 100644
--- a/Graphics/GLSLTools/src/GLSL2SPIRV.cpp
+++ b/Graphics/GLSLTools/src/GLSL2SPIRV.cpp
@@ -173,10 +173,102 @@ TBuiltInResource InitResources()
     return Resources;
 }
 
-std::vector<unsigned int> GLSLtoSPIRV(const SHADER_TYPE ShaderType, const char *ShaderSource) 
+class IoMapResolver : public glslang::TIoMapResolver
 {
-    std::vector<unsigned int> spirv;
+public:
+    // Should return true if the resulting/current binding would be okay.
+    // Basic idea is to do aliasing binding checks with this.
+    virtual bool validateBinding(EShLanguage stage, const char* name, const glslang::TType& type, bool is_live)
+    {
+        return true;
+    }
+
+    // Should return a value >= 0 if the current binding should be overridden.
+    // Return -1 if the current binding (including no binding) should be kept.
+    virtual int resolveBinding(EShLanguage stage, const char* name, const glslang::TType& type, bool is_live)
+    {
+        // We do not care about actual binding value here.
+        // We only need decoration to be present in SPIRV
+        return 0;
+    }
+
+    // Should return a value >= 0 if the current set should be overridden.
+    // Return -1 if the current set (including no set) should be kept.
+    virtual int resolveSet(EShLanguage stage, const char* name, const glslang::TType& type, bool is_live)
+    {
+        // We do not care about actual descriptor set value here.
+        // We only need decoration to be present in SPIRV
+        return 0;
+    }
+
+    // Should return a value >= 0 if the current location should be overridden.
+    // Return -1 if the current location (including no location) should be kept.
+    virtual int resolveUniformLocation(EShLanguage stage, const char* name, const glslang::TType& type, bool is_live)
+    {
+        return -1;
+    }
+
+    // Should return true if the resulting/current setup would be okay.
+    // Basic idea is to do aliasing checks and reject invalid semantic names.
+    virtual bool validateInOut(EShLanguage stage, const char* name, const glslang::TType& type, bool is_live)
+    {
+        return true;
+    }
+
+    // Should return a value >= 0 if the current location should be overridden.
+    // Return -1 if the current location (including no location) should be kept.
+    virtual int resolveInOutLocation(EShLanguage stage, const char* name, const glslang::TType& type, bool is_live)
+    {
+        return -1;
+    }
+
+    // Should return a value >= 0 if the current component index should be overridden.
+    // Return -1 if the current component index (including no index) should be kept.
+    virtual int resolveInOutComponent(EShLanguage stage, const char* name, const glslang::TType& type, bool is_live)
+    {
+        return -1;
+    }
+
+    // Should return a value >= 0 if the current color index should be overridden.
+    // Return -1 if the current color index (including no index) should be kept.
+    virtual int resolveInOutIndex(EShLanguage stage, const char* name, const glslang::TType& type, bool is_live)
+    {
+        return -1;
+    }
 
+    // Notification of a uniform variable
+    virtual void notifyBinding(EShLanguage stage, const char* name, const glslang::TType& type, bool is_live)
+    {
+    }
+
+    // Notification of a in or out variable
+    virtual void notifyInOut(EShLanguage stage, const char* name, const glslang::TType& type, bool is_live)
+    {
+    }
+
+    // Called by mapIO when it has finished the notify pass
+    virtual void endNotifications(EShLanguage stage)
+    {
+    }
+
+    // Called by mapIO when it starts its notify pass for the given stage
+    virtual void beginNotifications(EShLanguage stage)
+    {
+    }
+
+    // Called by mipIO when it starts its resolve pass for the given stage
+    virtual void beginResolve(EShLanguage stage)
+    {
+    }
+
+    // Called by mapIO when it has finished the resolve pass
+    virtual void endResolve(EShLanguage stage)
+    {
+    }
+};
+
+std::vector<unsigned int> GLSLtoSPIRV(const SHADER_TYPE ShaderType, const char *ShaderSource) 
+{
 #if PLATFORM_ANDROID
 
     // On Android, use shaderc instead.
@@ -187,6 +279,7 @@ std::vector<unsigned int> GLSLtoSPIRV(const SHADER_TYPE ShaderType, const char *
         LOGE("Error: Id=%d, Msg=%s", module.GetCompilationStatus(), module.GetErrorMessage().c_str());
         return false;
     }
+    std::vector<unsigned int> spirv;
     spirv.assign(module.cbegin(), module.cend());
 
 #else
@@ -202,25 +295,25 @@ std::vector<unsigned int> GLSLtoSPIRV(const SHADER_TYPE ShaderType, const char *
     Shader.setStrings(ShaderStrings, 1);
     
     Shader.setAutoMapBindings(true);
-    Shader.setAutoMapLocations(true);
     if (!Shader.parse(&Resources, 100, false, messages))
     {
         LOG_ERROR_MESSAGE("Failed to parse shader source: \n", Shader.getInfoLog(), '\n', Shader.getInfoDebugLog());
-        return std::move(spirv);
+        return {};
     }
 
     glslang::TProgram Program;
     Program.addShader(&Shader);
-
-    //
-    // Program-level processing...
-    //
     if (!Program.link(messages))
     {
         LOG_ERROR_MESSAGE("Failed to link program: \n", Shader.getInfoLog(), '\n', Shader.getInfoDebugLog());
-        return std::move(spirv);
+        return {};
     }
 
+    IoMapResolver Resovler;
+    // This step is essential to set bindings and descriptor sets
+    Program.mapIO(&Resovler);
+
+    std::vector<unsigned int> spirv;
     glslang::GlslangToSpv(*Program.getIntermediate(ShLang), spirv);
 #endif
 
diff --git a/Graphics/GLSLTools/src/SPIRVShaderResources.cpp b/Graphics/GLSLTools/src/SPIRVShaderResources.cpp
index 36fd9d4f..1e2a5865 100644
--- a/Graphics/GLSLTools/src/SPIRVShaderResources.cpp
+++ b/Graphics/GLSLTools/src/SPIRVShaderResources.cpp
@@ -27,11 +27,57 @@
 namespace Diligent
 {
 
+template<typename Type>
+Type GetResourceArraySize(const spirv_cross::Compiler &Compiler,
+                          const spirv_cross::Resource &Res)
+{
+    const auto& type = Compiler.get_type(Res.type_id);
+    uint32_t arrSize = !type.array.empty() ? type.array[0] : 1;
+    VERIFY(arrSize <= std::numeric_limits<Type>::max(), "Array size exceeds maximum representable value ", std::numeric_limits<Type>::max());
+    return static_cast<Type>(arrSize);
+}
+
+static uint32_t GetDecorationOffset(const spirv_cross::Compiler &Compiler,
+                                    const spirv_cross::Resource &Res,
+                                    spv::Decoration Decoration)
+{
+    VERIFY(Compiler.has_decoration(Res.id, Decoration), "Res \'", Res.name, "\' has no requested decoration");
+    uint32_t offset = 0;
+    auto declared = Compiler.get_binary_offset_for_decoration(Res.id, Decoration, offset);
+    VERIFY(declared, "Requested decoration is not declared");
+    return offset;
+}
+
+template<typename Type>
+static Type GetDecoration(const spirv_cross::Compiler &Compiler,
+                          const spirv_cross::Resource &Res,
+                          spv::Decoration Decoration)
+{
+    auto dec = Compiler.get_decoration(Res.id, Decoration);
+    VERIFY(dec <= std::numeric_limits<Type>::max(), "Decoration value exceeds maximum representable value ", std::numeric_limits<Type>::max());
+    return static_cast<Type>(dec);
+}
+
+SPIRVShaderResourceAttribs::SPIRVShaderResourceAttribs(const spirv_cross::Compiler &Compiler,
+                                                       const spirv_cross::Resource &Res, 
+                                                       ResourceType _Type, 
+                                                       SHADER_VARIABLE_TYPE _VarType) :
+    Name(Res.name),
+    Binding(GetDecoration<decltype(Binding)>(Compiler, Res, spv::DecorationBinding)),
+    ArraySize(GetResourceArraySize<decltype(ArraySize)>(Compiler, Res)),
+    BindingDecorationOffset(GetDecorationOffset(Compiler, Res, spv::Decoration::DecorationBinding)),
+    DescriptorSetDecorationOffset(GetDecorationOffset(Compiler, Res, spv::Decoration::DecorationDescriptorSet)),
+    DescriptorSet(GetDecoration<decltype(DescriptorSet)>(Compiler,Res, spv::DecorationDescriptorSet)),
+    Type(_Type),
+    VarType(_VarType)
+{
+}
+
 SPIRVShaderResources::SPIRVShaderResources(IMemoryAllocator &Allocator, SHADER_TYPE ShaderType, std::vector<uint32_t> spirv_binary) :
     m_MemoryBuffer(nullptr, STDDeleterRawMem<void>(Allocator)),
     m_ShaderType(ShaderType)
 {
-    spirv_cross::Compiler Compiler(std::move(spirv_binary));
+    spirv_cross::Compiler Compiler(spirv_binary);
 
     // The SPIR-V is now parsed, and we can perform reflection on it.
     spirv_cross::ShaderResources resources = Compiler.get_shader_resources();
@@ -46,68 +92,49 @@ SPIRVShaderResources::SPIRVShaderResources(IMemoryAllocator &Allocator, SHADER_T
                static_cast<Uint32>(resources.separate_samplers.size())
                );
 
+    Uint32 CurrUB = 0, CurrSB = 0, CurrImg = 0, CurrSmplImg = 0, CurrAC = 0, CurrSepImg = 0, CurrSepSmpl = 0;
     for (const auto &UB : resources.uniform_buffers)
     {
-        UB.name;
-        unsigned location = Compiler.get_decoration(UB.id, spv::DecorationLocation);
-        unsigned binding = Compiler.get_decoration(UB.id, spv::DecorationBinding);
-
-        int a=0;
+        new (&GetUB(CurrUB++)) SPIRVShaderResourceAttribs(Compiler, UB, SPIRVShaderResourceAttribs::ResourceType::UniformBuffer, SHADER_VARIABLE_TYPE_STATIC);
     }
 
     for (const auto &SB : resources.storage_buffers)
     {
-        SB.name;
-        unsigned location = Compiler.get_decoration(SB.id, spv::DecorationLocation);
-        unsigned binding = Compiler.get_decoration(SB.id, spv::DecorationBinding);
-
-        int a = 0;
+        new (&GetSB(CurrSB++)) SPIRVShaderResourceAttribs(Compiler, SB, SPIRVShaderResourceAttribs::ResourceType::StorageBuffer, SHADER_VARIABLE_TYPE_STATIC);
     }
 
     for (const auto &SmplImg : resources.sampled_images)
     {
-        SmplImg.name;
-        unsigned location = Compiler.get_decoration(SmplImg.id, spv::DecorationLocation);
-        unsigned binding = Compiler.get_decoration(SmplImg.id, spv::DecorationBinding);
-
-        int a = 0;
+        new (&GetSmplImg(CurrSmplImg++)) SPIRVShaderResourceAttribs(Compiler, SmplImg, SPIRVShaderResourceAttribs::ResourceType::SampledImage, SHADER_VARIABLE_TYPE_STATIC);
     }
 
     for (const auto &Img : resources.storage_images)
     {
-        Img.name;
-        unsigned location = Compiler.get_decoration(Img.id, spv::DecorationLocation);
-        unsigned binding = Compiler.get_decoration(Img.id, spv::DecorationBinding);
-
-        int a = 0;
+        new (&GetImg(CurrImg++)) SPIRVShaderResourceAttribs(Compiler, Img, SPIRVShaderResourceAttribs::ResourceType::StorageImage, SHADER_VARIABLE_TYPE_STATIC);
     }
 
     for (const auto &AC : resources.atomic_counters)
     {
-        AC.name;
-        unsigned location = Compiler.get_decoration(AC.id, spv::DecorationLocation);
-        unsigned binding = Compiler.get_decoration(AC.id, spv::DecorationBinding);
-
-        int a = 0;
+        new (&GetAC(CurrAC++)) SPIRVShaderResourceAttribs(Compiler, AC, SPIRVShaderResourceAttribs::ResourceType::AtomicCounter, SHADER_VARIABLE_TYPE_STATIC);
     }
 
     for (const auto &SepImg : resources.separate_images)
     {
-        SepImg.name;
-        unsigned location = Compiler.get_decoration(SepImg.id, spv::DecorationLocation);
-        unsigned binding = Compiler.get_decoration(SepImg.id, spv::DecorationBinding);
-
-        int a = 0;
+        new (&GetSepImg(CurrSepImg++)) SPIRVShaderResourceAttribs(Compiler, SepImg, SPIRVShaderResourceAttribs::ResourceType::SeparateImage, SHADER_VARIABLE_TYPE_STATIC);
     }
 
     for (const auto &SepSam : resources.separate_samplers)
     {
-        SepSam.name;
-        unsigned location = Compiler.get_decoration(SepSam.id, spv::DecorationLocation);
-        unsigned binding = Compiler.get_decoration(SepSam.id, spv::DecorationBinding);
-
-        int a = 0;
+        new (&GetSepSmpl(CurrSepSmpl++)) SPIRVShaderResourceAttribs(Compiler, SepSam, SPIRVShaderResourceAttribs::ResourceType::SeparateSampler, SHADER_VARIABLE_TYPE_STATIC);
     }
+
+    VERIFY_EXPR(CurrUB      == GetNumUBs());
+    VERIFY_EXPR(CurrSB      == GetNumSBs());
+    VERIFY_EXPR(CurrImg     == GetNumImgs());
+    VERIFY_EXPR(CurrSmplImg == GetNumSmplImgs());
+    VERIFY_EXPR(CurrAC      == GetNumACs());
+    VERIFY_EXPR(CurrSepImg  == GetNumSepImgs());
+    VERIFY_EXPR(CurrSepSmpl == GetNumSepSmpls());
 }
 
 SPIRVShaderResources::SPIRVShaderResources(IMemoryAllocator &Allocator,
diff --git a/Graphics/GraphicsEngineVulkan/include/PipelineLayout.h b/Graphics/GraphicsEngineVulkan/include/PipelineLayout.h
index efe4caef..625e1f3d 100644
--- a/Graphics/GraphicsEngineVulkan/include/PipelineLayout.h
+++ b/Graphics/GraphicsEngineVulkan/include/PipelineLayout.h
@@ -160,18 +160,6 @@ private:
         uint8_t m_ActiveSets = 0;
     };
 
-#if 0
-    static constexpr Uint8 InvalidRootTableIndex = static_cast<Uint8>(-1);
-    
-    // The array below contains array index of a CBV/SRV/UAV root table 
-    // in m_RootParams (NOT the Root Index!), for every variable type 
-    // (static, mutable, dynamic) and every shader type,
-    // or -1, if the table is not yet assigned to the combination
-    Uint8 m_SrvCbvUavRootTablesMap[SHADER_VARIABLE_TYPE_NUM_TYPES * 6];
-    // This array contains the same data for Sampler root table
-    Uint8 m_SamplerRootTablesMap[SHADER_VARIABLE_TYPE_NUM_TYPES * 6];
-#endif
-
     IMemoryAllocator &m_MemAllocator;
     DescriptorSetLayoutManager m_LayoutMgr;
 
diff --git a/Graphics/GraphicsEngineVulkan/include/PipelineStateVkImpl.h b/Graphics/GraphicsEngineVulkan/include/PipelineStateVkImpl.h
index 0ebfe80c..5ec8d260 100644
--- a/Graphics/GraphicsEngineVulkan/include/PipelineStateVkImpl.h
+++ b/Graphics/GraphicsEngineVulkan/include/PipelineStateVkImpl.h
@@ -33,6 +33,7 @@
 #include "ShaderResourceLayoutVk.h"
 #include "AdaptiveFixedBlockAllocator.h"
 #include "VulkanUtilities/VulkanObjectWrappers.h"
+#include "PipelineLayout.h"
 
 /// Namespace for the Direct3D11 implementation of the graphics engine
 namespace Diligent
@@ -127,8 +128,8 @@ private:
 #endif
     VulkanUtilities::RenderPassWrapper m_RenderPass;
     VulkanUtilities::PipelineWrapper m_Pipeline;
-    VulkanUtilities::PipelineLayoutWrapper m_PipelineLayout;
-
+    PipelineLayout m_PipelineLayout;
+    VulkanUtilities::PipelineLayoutWrapper m_TmpPipelineLayout;
 };
 
 }
diff --git a/Graphics/GraphicsEngineVulkan/src/PipelineLayout.cpp b/Graphics/GraphicsEngineVulkan/src/PipelineLayout.cpp
index ab69114e..c7a80ace 100644
--- a/Graphics/GraphicsEngineVulkan/src/PipelineLayout.cpp
+++ b/Graphics/GraphicsEngineVulkan/src/PipelineLayout.cpp
@@ -301,12 +301,6 @@ PipelineLayout::PipelineLayout() :
     m_StaticSamplers( STD_ALLOCATOR_RAW_MEM(StaticSamplerAttribs, GetRawAllocator(), "Allocator for vector<StaticSamplerAttribs>") )
     */
 {
-#if 0
-    for(size_t i=0; i < _countof(m_SrvCbvUavRootTablesMap); ++i)
-        m_SrvCbvUavRootTablesMap[i] = InvalidRootTableIndex;
-    for(size_t i=0; i < _countof(m_SamplerRootTablesMap); ++i)
-        m_SamplerRootTablesMap[i] = InvalidRootTableIndex;
-#endif
 }
 
 void PipelineLayout::Release(RenderDeviceVkImpl *pDeviceVkImpl)
diff --git a/Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp b/Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp
index 8fccd1e8..17f4bcda 100644
--- a/Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp
+++ b/Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp
@@ -144,7 +144,7 @@ void PipelineStateVkImpl::CreateRenderPass(const VulkanUtilities::VulkanLogicalD
 
 PipelineStateVkImpl :: PipelineStateVkImpl(IReferenceCounters *pRefCounters, RenderDeviceVkImpl *pDeviceVk, const PipelineStateDesc &PipelineDesc) : 
     TPipelineStateBase(pRefCounters, pDeviceVk, PipelineDesc)/*,
-    m_DummyVar(*this),
+    m_DummyVar(*this)
     m_ResourceCacheDataAllocator(GetRawAllocator(), PipelineDesc.SRBAllocationGranularity),
     m_pDefaultShaderResBinding(nullptr, STDDeleter<ShaderResourceBindingVkImpl, FixedBlockMemoryAllocator>(pDeviceVk->GetSRBAllocator()) )
 */
@@ -353,8 +353,8 @@ PipelineStateVkImpl :: PipelineStateVkImpl(IReferenceCounters *pRefCounters, Ren
         PipelineLayoutCI.pSetLayouts = nullptr;
         PipelineLayoutCI.pushConstantRangeCount = 0;
         PipelineLayoutCI.pPushConstantRanges = nullptr;
-        m_PipelineLayout = LogicalDevice.CreatePipelineLayout(PipelineLayoutCI);
-        PipelineCI.layout = m_PipelineLayout;
+        m_TmpPipelineLayout = LogicalDevice.CreatePipelineLayout(PipelineLayoutCI);
+        PipelineCI.layout = m_TmpPipelineLayout;
 
         PipelineCI.renderPass = m_RenderPass;
         PipelineCI.subpass = 0;
@@ -423,7 +423,8 @@ PipelineStateVkImpl::~PipelineStateVkImpl()
     pDeviceVkImpl->GetFramebufferCache().OnDestroyRenderPass(m_RenderPass);
     pDeviceVkImpl->SafeReleaseVkObject(std::move(m_RenderPass));
     pDeviceVkImpl->SafeReleaseVkObject(std::move(m_Pipeline));
-    pDeviceVkImpl->SafeReleaseVkObject(std::move(m_PipelineLayout));
+    m_PipelineLayout.Release(pDeviceVkImpl);
+    //pDeviceVkImpl->SafeReleaseVkObject(std::move(m_TmpPipelineLayout));
 
 #if 0
     auto &ShaderResLayoutAllocator = GetRawAllocator();