Final changes to enable inline constants in Vulkan #750

hzqst · 2026-01-02T04:24:53Z

No description provided.

Graphics/GraphicsEngineVulkan/src/PipelineLayoutVk.cpp

Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp

+                        Uint32 ArraySize = Attribs.ArraySize;
+                        if (Flags & PIPELINE_RESOURCE_FLAG_INLINE_CONSTANTS)
+                        {
+                            VERIFY(Flags == PIPELINE_RESOURCE_FLAG_INLINE_CONSTANTS, "INLINE_CONSTANTS flag cannot be combined with other flags.");


Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp

TheMostDiligent · 2026-01-02T12:07:54Z

Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp

+                ResCI.ShaderType                  = pShader->GetDesc().ShaderType;
+                ResCI.Name                        = pShader->GetDesc().Name;
+                ResCI.CombinedSamplerSuffix       = pShader->GetDesc().UseCombinedTextureSamplers ? pShader->GetDesc().CombinedSamplerSuffix : nullptr;
+                ResCI.LoadShaderStageInputs       = pShader->GetDesc().ShaderType == SHADER_TYPE_VERTEX;


Loading shader stage inputs is not needed.
It is used to patch Vertex Shader inputs, which is done when the shader is created.
They are already correct in the original SPIRV, and are the same in patched.

It is used to patch Vertex Shader inputs, which is done when the shader is created.

it can be a problem when ShaderVkImpl is asynchronously loaded, SPIRVShaderResources is not available at that time in ShaderVkImpl::Initialize. The ArchiverTest said that no SPIRVShaderResources is created when shader is created.

btw isn't shader patching done in RemapOrVerifyShaderResources?

There is an error somewhere. Asynchronous shaders must be created first before the pipeline is created. Querying anything from the shader while it is compiled is an error.

Well, seems that after commenting out deferred SPIRVShaderResource creation everything works fine, also with ShaderResource initialized with pShader->GetShaderResource()

I haven't checked this code yet, but based on comment, this block is certainly not needed. InitPipelineLayout is only called after all asynchronous shader work is complete.

Still a problem with Async PSO too

Reflection is only stripped from the final SPIRV after all patching, push constants conversion including

nvm I skipped the validation in 34af2e6. it works fine for stripped_reflection one now.

I refactored PipelineStateVkImpl::ShaderStageInfo, but now when I look at it I don't think ShaderResources are needed in ShaderStageInfo at all.
The problem is that right now, PatchShaderConvertUniformBufferToPushConstant is called separately from RemapOrVerifyShaderResources, but it should be part of RemapOrVerifyShaderResources.
RemapOrVerifyShaderResources should both patch bindings and push constants, if necessary.
If needed, it will created new resources and add them to pDvpShaderResources. If pDvpShaderResources is null, new resources are not needed.

I refactored PipelineStateVkImpl::ShaderStageInfo, but now when I look at it I don't think ShaderResources are needed in ShaderStageInfo at all. The problem is that right now, PatchShaderConvertUniformBufferToPushConstant is called separately from RemapOrVerifyShaderResources, but it should be part of RemapOrVerifyShaderResources. RemapOrVerifyShaderResources should both patch bindings and push constants, if necessary. If needed, it will created new resources and add them to pDvpShaderResources. If pDvpShaderResources is null, new resources are not needed.

but pDvpShaderResources is a DILIGENT_DEVELOPMENT-only variable which is not available in production build.

Yes, which is why in production build there is no even need to create new shader resources after patching SPIRV.

TheMostDiligent · 2026-01-02T12:08:22Z

Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp

+                ResCI.Name                        = pShader->GetDesc().Name;
+                ResCI.CombinedSamplerSuffix       = pShader->GetDesc().UseCombinedTextureSamplers ? pShader->GetDesc().CombinedSamplerSuffix : nullptr;
+                ResCI.LoadShaderStageInputs       = pShader->GetDesc().ShaderType == SHADER_TYPE_VERTEX;
+                ResCI.LoadUniformBufferReflection = true; //LoadConstantBufferReflection;


Why is uniform buffer reflection needed?

we just leave it false or?

Graphics/GraphicsEngineVulkan/include/PipelineLayoutVk.hpp

Graphics/GraphicsEngineVulkan/src/PipelineLayoutVk.cpp

TheMostDiligent

Can you submit changes to PipelineLayoutVk.cpp/hpp as a separate PR - they are clean and isolated.

hzqst · 2026-01-02T14:47:45Z

Seems that we have PipelineResourceSignatureVkImpl::HasInlineConstants failed and causing m_PushConstantInfo to be left empty. what do we need next?

    bool HasInlineConstants() const
    {
        // TODO
        return false;
    }

TheMostDiligent · 2026-01-03T00:53:07Z

Seems that we have PipelineResourceSignatureVkImpl::HasInlineConstants failed and causing m_PushConstantInfo to be left empty. what do we need next?

This is a simple function that helps to avoid looping through all resources for all signatures even if there are no inline constants

Graphics/GraphicsEngineVulkan/include/ShaderResourceCacheVk.hpp

Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp

TheMostDiligent · 2026-01-06T05:31:12Z

Take a look at how much simpler I made the inline constant initialization in the resource cache.
Few and simpler changes are always preferable as they are easier to understand, and less code to maintain.

hzqst · 2026-01-06T15:58:26Z

Can you please do rebases instead of merges? Linearizing history after merge is very inconvenient

will do next time

TheMostDiligent · 2026-01-06T17:28:09Z

Tests/DiligentCoreAPITest/src/InlineConstantsTest.cpp

+
+float4 main(in PSInput PSIn) : SV_Target
+{
+    return PSIn.Color;


Why PS is not using the push constants block? It should to verify that the stages are properly merged.

Why PS is not using the push constants block? It should to verify that the stages are properly merged.

That's what I mentioned before:

If we have [[vk::push_constant]] ConstantBuffer<PushConstants_t> PushConstants; declared both in VS and PS, they will be allocated as two resources instead of one, in PipelineStateVkImpl::GetDefaultResourceSignatureDesc.

Calling pPSO->GetStaticVariableByName(SHADER_TYPE_PIXEL, "PushConstants")->SetInlineConstants does nothing under the hood as data never get committed in m_CommandBuffer.PushConstants(Layout.GetVkPipelineLayout(), PushConstantInfo.vkRange, pPushConstantData);. The PushConstantInfo.vkRange goes with nothing more than VK_SHADER_STAGE_VERTEX_BIT.

I suggust ensure that [[vk::push_constant]] ConstantBuffer<PushConstants_t> PushConstants; has exact the same size in both VS and PS first, if they are both found in VS and PS.

We merge PCInfo.vkRange.stageFlags with ShaderTypesToVkShaderStageFlags(SHADER_TYPE_PIXEL) in the later pass in PipelineLayoutVk::GetPushConstantInfo then.

PS inline constants need to be merged into VS inline constants in PipelineStateVkImpl::GetDefaultResourceSignatureDesc as an identical resource also.

TheMostDiligent · 2026-01-06T17:29:18Z

@hzqst Don't rebase on inline_constants yet - the tests will break due to incorrect memory size for SRB allocator.

TheMostDiligent · 2026-01-07T05:42:21Z

I am going to rebase and overwrite your branch - give me a few minutes.
You will need to submit the last change again - it is not yet working anyway.

TheMostDiligent · 2026-01-07T05:57:13Z

OK, done

hzqst · 2026-01-07T08:18:11Z

@TheMostDiligent we are now error out with 9aad908

If we have [[vk::push_constant]] ConstantBuffer<PushConstants_t> PushConstants; declared both in VS and PS, they will be allocated as two resources instead of one, in PipelineStateVkImpl::GetDefaultResourceSignatureDesc.

Calling pPSO->GetStaticVariableByName(SHADER_TYPE_PIXEL, "PushConstants")->SetInlineConstants does nothing under the hood as data never get committed in m_CommandBuffer.PushConstants(Layout.GetVkPipelineLayout(), PushConstantInfo.vkRange, pPushConstantData);. The PushConstantInfo.vkRange goes with nothing more than VK_SHADER_STAGE_VERTEX_BIT.

I suggust ensure that [[vk::push_constant]] ConstantBuffer<PushConstants_t> PushConstants; has exact the same size in both VS and PS first, if they are both found in VS and PS.

We should probably merge PCInfo.vkRange.stageFlags with ShaderTypesToVkShaderStageFlags(SHADER_TYPE_PIXEL) later in PipelineLayoutVk::GetPushConstantInfo.

PS inline constants as a PRSDesc need to be merged into VS inline constants in PipelineStateVkImpl::GetDefaultResourceSignatureDesc also.

TheMostDiligent · 2026-01-07T14:35:13Z

I suggust ensure that [[vk::push_constant]] ConstantBuffer<PushConstants_t> PushConstants; has exact the same size in both VS and PS first, if they are both found in VS and PS.

Yes, this is what we need to do.
Instead of break, the loop should check if PCInfo is not empty already, and if so:

Check if the name and size matches, then add the shader stage flag
Otherwise throw an error

Can you work on this in a separate PR?
I want to finish merging this one.

TheMostDiligent · 2026-01-07T14:36:26Z

I will also remove the test from this branch.

hzqst · 2026-01-07T14:38:49Z

I suggust ensure that [[vk::push_constant]] ConstantBuffer<PushConstants_t> PushConstants; has exact the same size in both VS and PS first, if they are both found in VS and PS.

Yes, this is what we need to do. Instead of break, the loop should check if PCInfo is not empty already, and if so:

Check if the name and size matches, then add the shader stage flag

Otherwise throw an error

Can you work on this in a separate PR? I want to finish merging this one.

base on DiligentGraphics:inline_constants or hzqst:inline_constants_747_fix?

TheMostDiligent · 2026-01-07T14:55:01Z

On hzqst:inline_constants_747_fix, DiligentGraphics:inline_constants don't have full implementation yet.

TheMostDiligent · 2026-01-07T15:15:25Z

Can you please create another PR?
I am working on merging this one.

hzqst · 2026-01-07T15:16:04Z

Done in b0e30b5

Next problem: what we gonna do with PushConstantInfo::SignatureIndex and PushConstantInfo::ResourceIndex? we have two ResourceIndex now, if merge happens.

TheMostDiligent · 2026-01-07T16:15:55Z

Graphics/GraphicsEngineVulkan/src/DeviceContextVkImpl.cpp

+
+    const Uint32                           SRBIndex   = pResBindingVkImpl->GetBindingIndex();
+    const PipelineResourceSignatureVkImpl* pSignature = pResBindingVkImpl->GetSignature();
+
+    // PRS has no resources and thus PIPELINE_TYPE_INVALID ?
+    PIPELINE_TYPE SRBPipelineType = pResBindingVkImpl->GetPipelineType();
+    if (SRBPipelineType == PIPELINE_TYPE_INVALID)
+        return;
+
+    ResourceBindInfo&                    BindInfo = GetBindInfo(SRBPipelineType);
+    ResourceBindInfo::DescriptorSetInfo& SetInfo  = BindInfo.SetInfo[SRBIndex];
+
+    // Always bind the SRB, even if it has no descriptor sets (e.g., only push constants)
+    // The resource cache is needed to store push constant data
+    BindInfo.Set(SRBIndex, pResBindingVkImpl);
+
+    // If there are no descriptor sets, we're done (SRB only contains push constants)
+    // Clear the stale flag since there are no descriptor sets to commit
    if (ResourceCache.GetNumDescriptorSets() == 0)
    {
-        // Ignore SRBs that contain no resources
+        // Clear the stale bit for this SRB since it has no descriptor sets to commit
+        // The SRB is still bound and ResourceCaches[SRBIndex] is set, so push constants can be accessed
+        const auto SRBBit = static_cast<ResourceBindInfo::SRBMaskType>(1u << SRBIndex);
+        BindInfo.StaleSRBMask &= ~SRBBit;


Why are all these changed needed?
If I revert all of them, everything works fine.

Based on comments, it looks like this is remained from your earlier versions, since now all inline constants get descriptor sets, this does not make sense:
// Always bind the SRB, even if it has no descriptor sets (e.g., only push constants)
Also,

PIPELINE_TYPE SRBPipelineType = pResBindingVkImpl->GetPipelineType(); if (SRBPipelineType == PIPELINE_TYPE_INVALID) return;

Should never happen.

…ffers and GetInlineConstantBuffers

…retrieval

…stants in CommitInlineConstants

hzqst marked this pull request as ready for review January 2, 2026 04:27

hzqst requested a review from TheMostDiligent as a code owner January 2, 2026 04:27

hzqst changed the title ~~Fix issues from https://github.com/DiligentGraphics/DiligentCore/pull/747~~ Fix issues from #747 Jan 2, 2026

github-advanced-security bot found potential problems Jan 2, 2026

View reviewed changes

Graphics/GraphicsEngineVulkan/src/PipelineLayoutVk.cpp Fixed Show fixed Hide fixed

Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Jan 2, 2026

View reviewed changes

Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp Fixed Show fixed Hide fixed

Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp Fixed Show fixed Hide fixed

TheMostDiligent reviewed Jan 2, 2026

View reviewed changes

Graphics/GraphicsEngineVulkan/include/PipelineLayoutVk.hpp Outdated Show resolved Hide resolved

TheMostDiligent reviewed Jan 2, 2026

View reviewed changes

Graphics/GraphicsEngineVulkan/src/PipelineLayoutVk.cpp Outdated Show resolved Hide resolved

TheMostDiligent reviewed Jan 2, 2026

View reviewed changes

TheMostDiligent force-pushed the inline_constants_747_fix branch 3 times, most recently from e405c1f to 7be6864 Compare January 3, 2026 00:50

TheMostDiligent force-pushed the inline_constants_747_fix branch 4 times, most recently from a022722 to 09a471d Compare January 4, 2026 02:10

TheMostDiligent force-pushed the inline_constants branch from 9f79a40 to 75a4449 Compare January 4, 2026 04:44

TheMostDiligent force-pushed the inline_constants_747_fix branch 2 times, most recently from 7a5328a to 1c5f655 Compare January 4, 2026 16:46

TheMostDiligent requested changes Jan 4, 2026

View reviewed changes

Graphics/GraphicsEngineVulkan/include/ShaderResourceCacheVk.hpp Outdated Show resolved Hide resolved

hzqst added a commit to hzqst/DiligentCore that referenced this pull request Jan 5, 2026

merge DiligentGraphics#750

1aca225

TheMostDiligent reviewed Jan 6, 2026

View reviewed changes

Graphics/GraphicsEngineVulkan/src/PipelineStateVkImpl.cpp Outdated Show resolved Hide resolved

TheMostDiligent force-pushed the inline_constants_747_fix branch 3 times, most recently from 43e3d98 to bd9769e Compare January 6, 2026 05:23

TheMostDiligent force-pushed the inline_constants_747_fix branch from a36c137 to 293f205 Compare January 6, 2026 17:27

TheMostDiligent reviewed Jan 6, 2026

View reviewed changes

TheMostDiligent force-pushed the inline_constants_747_fix branch 3 times, most recently from efe8f86 to fbe3f5d Compare January 7, 2026 03:59

TheMostDiligent force-pushed the inline_constants_747_fix branch from 43ec87a to 3159a82 Compare January 7, 2026 05:56

TheMostDiligent force-pushed the inline_constants_747_fix branch from 9aad908 to 9e12690 Compare January 7, 2026 14:48

TheMostDiligent force-pushed the inline_constants_747_fix branch 3 times, most recently from 9e00a88 to 7b500ee Compare January 7, 2026 16:06

TheMostDiligent reviewed Jan 7, 2026

View reviewed changes

TheMostDiligent force-pushed the inline_constants_747_fix branch from 7b500ee to ba44fc8 Compare January 7, 2026 16:26

hzqst and others added 5 commits January 7, 2026 08:28

DeviceContextVk: enable inline constants

d444f82

PipelineResourceSignatureVkImpl: remove unused GetNumInlineConstantBu…

23c49fa

…ffers and GetInlineConstantBuffers

PipelineResourceSignatureVkImpl: simplify UpdateInlineConstantBuffers

f8d2a37

InlineConstantBufferAttribsVk: add SRBCacheOffset to facilitate data …

00d63c4

…retrieval

PipelineResourceSignatureVkImpl: handle emulated buffers and push con…

7cac8db

…stants in CommitInlineConstants

TheMostDiligent force-pushed the inline_constants_747_fix branch from ba44fc8 to 7cac8db Compare January 7, 2026 16:28

TheMostDiligent merged commit 7cac8db into DiligentGraphics:inline_constants Jan 7, 2026
37 checks passed

Final changes to enable inline constants in Vulkan #750

Final changes to enable inline constants in Vulkan #750

Conversation

hzqst commented Jan 2, 2026

Uh oh!

Uh oh!

Uh oh!

Check warning

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzqst Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheMostDiligent left a comment

Choose a reason for hiding this comment

Uh oh!

hzqst commented Jan 2, 2026

Uh oh!

TheMostDiligent commented Jan 3, 2026

Uh oh!

Uh oh!

Uh oh!

TheMostDiligent commented Jan 6, 2026

Uh oh!

hzqst commented Jan 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzqst Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheMostDiligent commented Jan 6, 2026

Uh oh!

TheMostDiligent commented Jan 7, 2026

Uh oh!

TheMostDiligent commented Jan 7, 2026

Uh oh!

hzqst commented Jan 7, 2026

Uh oh!

TheMostDiligent commented Jan 7, 2026

Uh oh!

TheMostDiligent commented Jan 7, 2026

Uh oh!

hzqst commented Jan 7, 2026

Uh oh!

TheMostDiligent commented Jan 7, 2026

Uh oh!

TheMostDiligent commented Jan 7, 2026

Uh oh!

hzqst commented Jan 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hzqst Jan 2, 2026 •

edited

Loading

hzqst Jan 7, 2026 •

edited

Loading