Vulkan push constant support, based on #728

[hzqst]

HLSL prefix: [[vk::push_constant]] GLSL counterpart prefix: layout(push_constant)

Note that I added a new flag PIPELINE_RESOURCE_FLAG_VULKAN_PUSH_CONSTANT to indicate that a inline constant is a "vulkan push constant".

I don't find a way to avoid explicitly specifying because it is not possible to determine whether a inline constant is a vulkan push constant or not in PipelineResourceSignature creation (SPIRV is not available there).

As for legacy ResourceLayout, it's okay to have SHADER_VARIABLE_FLAG_INLINE_CONSTANTS without explicitly specifying vulkan stuffs for vulkan push constant, the flag PIPELINE_RESOURCE_FLAG_VULKAN_PUSH_CONSTANT will be automatically added to signature in InitDefaultSignature

Also added a new API SetPushConstants to IDeviceContextVk to expose the underlying API vkCmdPushConstants directly to user.

Tested with https://github.com/DiligentGraphics/DiligentCore/discussions/724, working fine so far, with either USE_STATIC_INLINE_CONSTANTS on / off, USE_VULKAN_PUSH_CONSTANT on / off, USE_PRS_TEST on / off.

overall doc: https://github.com/hzqst/DiligentCore/blob/vk_push_constants/doc/SetInlineConstants.md#vulkan-backend-implementation

~~TODO: the APITest on linux shows that InlineConstantTest image diffs from reference, further investigation needed. maybe issue with multiple dynamic-buffer-emulated inline constant?~~ fixed

[TheMostDiligent]

Emulating inline constants in Vulkan with buffers misses the point of push constants, which is a very fast and efficient way to make small amounts of data available to shaders.

As we discussed, Vulkan is a major challenge. PIPELINE_RESOURCE_FLAG_VULKAN_PUSH_CONSTANT should not be needed, it is confusing and requires special handling for one of the APIs, which is not what Diligent does. What Vulkan backend has to do is to patch the SPIRV to change the normal constant buffer into push constants block. This is tricky, but this will make Vulkan backend work seamlessly like others.

[hzqst]

Emulating inline constants in Vulkan with buffers misses the point of push constants, which is a very fast and efficient way to make small amounts of data available to shaders.

As we discussed, Vulkan is a major challenge. PIPELINE_RESOURCE_FLAG_VULKAN_PUSH_CONSTANT should not be needed, it is confusing and requires special handling for one of the APIs, which is not what Diligent does. What Vulkan backend has to do is to patch the SPIRV to change the normal constant buffer into push constants block. This is tricky, but this will make Vulkan backend work seamlessly like others.

I am going to promote the first inline constant declared in PRS a true vulkan pushconstant while leaving others emulated as a workaround.

[hzqst]

~~Well, have no idea why I got this on ImmutableSamplers2 test~~

[ RUN      ] PipelineResourceSignatureTest.ImmutableSamplers
[       OK ] PipelineResourceSignatureTest.ImmutableSamplers (133 ms)
[ RUN      ] PipelineResourceSignatureTest.ImmutableSamplers2
/home/runner/work/DiligentCore/DiligentCore/Tests/TestFramework/src/TestingEnvironment.cpp:66: Failure
Failed
Unexpected error

Diligent Engine: ERROR: Debug assertion failed in GetBindInfo(), file DeviceContextVkImpl.cpp, line 407:
Debug expression failed:
Type != PIPELINE_TYPE_INVALID
[  FAILED  ] PipelineResourceSignatureTest.ImmutableSamplers2 (130 ms)

It seems that The TEST_F(PipelineResourceSignatureTest, ImmutableSamplers2) creates a PipelineResourceSignature (pSignature3) with no resources, which causes its m_PipelineType to be set to PIPELINE_TYPE_INVALID. When CommitShaderResources() is called for this SRB, it calls GetBindInfo(PIPELINE_TYPE_INVALID), triggering the assertion VERIFY_EXPR(Type != PIPELINE_TYPE_INVALID) failure. I fixed this issue by skipping CommitShaderResources for PRS that has no resources bound on it.

[hzqst]

Another weird thing I've encountered is that pUncachedVS and pVS1 got same hash computation result on Vulkan in the first pass, which lead to PresentInCache=false, foundInPSO=true, and essentially fail the verification. I skipped the check by patching PresentInCache to true on Vulkan. I wonder if it is really necessary to have this verification.

[TheMostDiligent]

This test worked before, and it does not use inline constants, so it should not be affected by your changes

[hzqst]

This test worked before, and it does not use inline constants, so it should not be affected by your changes

That was me when submitting inline constant data in CommitShaderResources, accessing BindInfo.SetInfo[SRBIndex]; even before skipping empty PRS, my bad anyway.

[hzqst]

The implict added flags PIPELINE_RESOURCE_FLAG_VULKAN_PUSH_CONSTANT does the left job for me. This is what I am planning to do, any comment?

ConvertUniformBufferToPushConstant procedure:

std::vector<uint32_t> ConvertUniformBufferToPushConstant(
    spv_target_env target_env,
    const std::vector<uint32_t>& SPIRV,
    const std::string& block_name) 
{
    spvtools::Optimizer optimizer(target_env);
    
    optimizer.RegisterPass(
        CreatePassFromClass<ConvertUniformBufferToPushConstantPass>(block_name));
    
    std::vector<uint32_t> result;
    if (!optimizer.Run(SPIRV.data(), SPIRV.size(), &result)) {
        //throw error...
        return {};
    }
    
    return result;
}

[TheMostDiligent]

Yes, the first inline constants buffer defined in the signature should become push constants block in Vulkan. VULKAN_PUSH_CONSTANT constant is not needed and should be removed.

[hzqst]

Yes, the first inline constants buffer defined in the signature should become push constants block in Vulkan. VULKAN_PUSH_CONSTANT constant is not needed and should be removed.

But we need VULKAN_PUSH_CONSTANT at least as an internal flags to distinguish push constant from buffer-emulated inline constants when creating layout from signatures. especially when CreatePipelineResourceSignature invokes CreateSetLayouts under the hood in constructor.

[TheMostDiligent]

This flag is internal to Vulkan backend only and should be stored in PipelineResourceAttribsVk

[hzqst]

Well, two crucial problems:

1. Which DescriptorSet the mutable (SHADER_RESOURCE_VARIABLE_TYPE_MUTABLE) buffer-emulated inline constants should go with ? DESCRIPTOR_SET_ID_STATIC_MUTABLE or DESCRIPTOR_SET_ID_DYNAMIC ?

2. What if there are multiple PRSs that contains inline constants ?

RefCntAutoPtr<IPipelineResourceSignature> pPosSign;
RefCntAutoPtr<IPipelineResourceSignature> pColSign;

PipelineResourceSignatureDescX SignDesc{"Inline constants test"};
SignDesc
    .AddResource(SHADER_TYPE_VERTEX, "cb0_stat", 1u, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, SHADER_RESOURCE_VARIABLE_TYPE_STATIC, PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS)
    .AddResource(SHADER_TYPE_VERTEX, "cb0_mut", 1u, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, SHADER_RESOURCE_VARIABLE_TYPE_MUTABLE)
    .AddResource(SHADER_TYPE_VERTEX, "cb0_dyn", 1u, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, SHADER_RESOURCE_VARIABLE_TYPE_DYNAMIC)
    .AddResource(SHADER_TYPE_VERTEX, "tex0_stat", SHADER_RESOURCE_TYPE_TEXTURE_SRV, SHADER_RESOURCE_VARIABLE_TYPE_STATIC)
    .AddResource(SHADER_TYPE_VERTEX, "tex0_mut", SHADER_RESOURCE_TYPE_TEXTURE_SRV, SHADER_RESOURCE_VARIABLE_TYPE_MUTABLE)
    .AddResource(SHADER_TYPE_VERTEX, "tex0_dyn", SHADER_RESOURCE_TYPE_TEXTURE_SRV, SHADER_RESOURCE_VARIABLE_TYPE_DYNAMIC)

    .AddResource(SHADER_TYPE_VERTEX, "cbInlinePositions", kNumPosConstants, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, PosType, PIPELINE_RESOURCE_FLAG_INLINE_CONSTANTS);

if (NumSignatures == 2)
{
    pDevice->CreatePipelineResourceSignature(SignDesc, &pPosSign);
    ASSERT_TRUE(pPosSign);

    SignDesc.Clear();
    SignDesc.Name         = "Inline constants test 2";
    SignDesc.BindingIndex = 1;
}

SignDesc.AddResource(SHADER_TYPE_VERTEX, "cb1_stat", 1u, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, SHADER_RESOURCE_VARIABLE_TYPE_STATIC, PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS)
    .AddResource(SHADER_TYPE_VERTEX, "cb1_mut", 1u, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, SHADER_RESOURCE_VARIABLE_TYPE_MUTABLE)
    .AddResource(SHADER_TYPE_VERTEX, "cb1_dyn", 1u, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, SHADER_RESOURCE_VARIABLE_TYPE_DYNAMIC)
    .AddResource(SHADER_TYPE_VERTEX, "tex1_stat", SHADER_RESOURCE_TYPE_TEXTURE_SRV, SHADER_RESOURCE_VARIABLE_TYPE_STATIC)
    .AddResource(SHADER_TYPE_VERTEX, "tex1_mut", SHADER_RESOURCE_TYPE_TEXTURE_SRV, SHADER_RESOURCE_VARIABLE_TYPE_MUTABLE)
    .AddResource(SHADER_TYPE_VERTEX, "tex1_dyn", SHADER_RESOURCE_TYPE_TEXTURE_SRV, SHADER_RESOURCE_VARIABLE_TYPE_DYNAMIC)

    .AddResource(SHADER_TYPE_VS_PS, "cbInlineColors", kNumColConstants, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, ColType, PIPELINE_RESOURCE_FLAG_INLINE_CONSTANTS)

    .AddResource(SHADER_TYPE_VERTEX, "cb2_stat", 1u, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, SHADER_RESOURCE_VARIABLE_TYPE_STATIC, PIPELINE_RESOURCE_FLAG_NO_DYNAMIC_BUFFERS)
    .AddResource(SHADER_TYPE_VERTEX, "cb2_mut", 1u, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, SHADER_RESOURCE_VARIABLE_TYPE_MUTABLE)
    .AddResource(SHADER_TYPE_VERTEX, "cb2_dyn", 1u, SHADER_RESOURCE_TYPE_CONSTANT_BUFFER, SHADER_RESOURCE_VARIABLE_TYPE_DYNAMIC)
    .AddResource(SHADER_TYPE_VERTEX, "tex2_stat", SHADER_RESOURCE_TYPE_TEXTURE_SRV, SHADER_RESOURCE_VARIABLE_TYPE_STATIC)
    .AddResource(SHADER_TYPE_VERTEX, "tex2_mut", SHADER_RESOURCE_TYPE_TEXTURE_SRV, SHADER_RESOURCE_VARIABLE_TYPE_MUTABLE)
    .AddResource(SHADER_TYPE_VERTEX, "tex2_dyn", SHADER_RESOURCE_TYPE_TEXTURE_SRV, SHADER_RESOURCE_VARIABLE_TYPE_DYNAMIC);

We have no idea how many inline constants there are that actually being used by a PSO, until CreateGraphicsPipelineState, thus no way to enforce a true vulkan push constant implicitly (cuz we need to skip descriptor set allocation for push constant and create proper layout in PRS creation).

workflow: PRS1 creation (create layout) -> PRS2 creation (create layout) -> PSO creation (now we finally knows how many inline constants there are in the current pipeline)

This is what I am planning to do:

PRS Creation:
  All inline constants -> Alloc DescriptorSet + SRBCache + Buffer

PSO Creation:
  Loop through ShaderResource -> Find a PushConstant
    found -> mark as push constant
    not found -> promote the first one in inline constants to a true push constant -> call PatchShaderConvertUniformBufferToPushConstant

Runtime:
  UpdateInlineConstantBuffers:
    if ( push constant )
      -> vkCmdPushConstants
    else
      -> Map/Unmap emulated buffer

[TheMostDiligent]

Which DescriptorSet the mutable (SHADER_RESOURCE_VARIABLE_TYPE_MUTABLE) buffer-emulated inline constants should go with ? DESCRIPTOR_SET_ID_STATIC_MUTABLE or DESCRIPTOR_SET_ID_DYNAMIC ?

Always mutable, because the emulated buffer does not change and needs to be bound only once. The variable type for inline constant does not really matter, but for consistency with other variables, static inline constants (though they don't really make sense) are set through PRS.

We have no idea how many inline constants there are that actually being used by a PSO, until CreateGraphicsPipelineState, thus no way to enforce a true vulkan push constant implicitly (cuz we need to skip descriptor set allocation for push constant and create proper layout in PRS creation).

This may be a real problem. The descriptor layout defined by PRS must not be changed when PSO is created. Looks like scenario with multiple PRS with inline constants has to be disallowed in Vulkan.

[hzqst]

Another problem found on patched shader:

//HashUtils.hpp
template <typename HasherType>
void HashShaderBytecode(HasherType& Hasher, IShader* pShader)
{
    if (pShader == nullptr)
        return;

    const void* pBytecode = nullptr;
    Uint64      Size      = 0;
    pShader->GetBytecode(&pBytecode, Size);
    VERIFY_EXPR(pBytecode != nullptr && Size != 0);
    Hasher.UpdateRaw(pBytecode, static_cast<size_t>(Size));
}

//RenderStateCacheImpl.cpp
        RefCntAutoPtr<IPipelineState> pPSO;
        CreatePSOFromCache(pCache, pData != nullptr, pVS1, pPS1, &pPSO);
        ASSERT_NE(pPSO, nullptr);
        ASSERT_EQ(pPSO->GetStatus(), PIPELINE_STATE_STATUS_READY);
        EXPECT_TRUE(pRefPSO->IsCompatibleWith(pPSO));
        EXPECT_TRUE(pPSO->IsCompatibleWith(pRefPSO));

        VerifyPSOFromCache(pPSO, nullptr);
        VerifyPSOFromCache(pPSO, pRefSRB);

        {
            RefCntAutoPtr<IPipelineState> pPSO2;
            CreatePSOFromCache(pCache, true, pVS1, pPS1, &pPSO2);
            EXPECT_EQ(pPSO, pPSO2);
        }

The RenderStateCacheImpl -> XXH128Hash diffs between CreatePSOFromCache(pCache, pData != nullptr, pVS1, pPS1, &pPSO); and CreatePSOFromCache(pCache, true, pVS1, pPS1, &pPSO2); since GetBytecode always returns patched shader. SPIRV bytes are known to be manipulated after first call to CreatePSOFromCache.

This causes a test failure in EXPECT_EQ(pPSO, pPSO2);

My sugguestion is to return unpached shader bytecode in GetByteCode.

[TheMostDiligent]

Bytecode is always patched when PSO is created. Decorating a buffer is inline constants is just one of the steps and should not change the logic.

[hzqst]

Bytecode is always patched when PSO is created. Decorating a buffer is inline constants is just one of the steps and should not change the logic.

That was me manipulating ShaderVkImpl::m_SPIRV which ruined everything. ShaderVkImpl::m_SPIRV should not be modified since pipeline creation as it can be shared between mutiple pipelines. I am going with ShaderStages[i].Shaders now.

Following changes were applied:

1. Flags VULKAN_PUSH_CONSTANT removed .
2. Promote first inline constant declared in pipeline to a true push constant if there is no push constant declared in SPIR-V.
3. Added ShaderResource to PipelineStateVkImpl::ShaderStageInfo, to have a chance to update it in PatchShaderConvertUniformBufferToPushConstant, where we reconstruct ShaderResource from patched SPIRV.
4. Buffer-emulated inline constants design aligned to D3D11 one
5. PatchShaderConvertUniformBufferToPushConstant utilizes a custom opt pass ConvertUBOToPushConstantPass, ThirdParty/SPIRV-Tools need to be merged.

[TheMostDiligent]

Why does ConvertUBOToPushConstantPass need to be in SPIRV-Tools? Keeping a fork of third-party dependencies is highly undesirable and creates a lot of problems.

[hzqst]

Why does ConvertUBOToPushConstantPass need to be in SPIRV-Tools? Keeping a fork of third-party dependencies is highly undesirable and creates a lot of problems.

cuz it depends on : source/opt/pass.h source/opt/ir_context.h source/opt/type_manager.h source/opt/decoration_manager.h which are SPIRV-Tools's private headers

some odd shenanigans need to done to our CMakeLists.txt to access SPIRV-Tools's private headers

[hzqst]

Following changes were applied:

1. ConvertUBOToPushConstantPass has been ported to SPIRVTools.cpp
2. PatchShaderConvertUBOToPushConstant: now error out when DILIGENT_NO_HLSL is defined (SPIRVTools is not available at that time). the first inline constant in the input SPIRV should be with layout(push_constant) explicitly.

[TheMostDiligent]

Looks like everything works now - this is really nice. 👍 Thanks for working on this!

Only the size of this PR got out of hands - merging change this big is very difficult. I suggest we break it into parts and merge in steps. The first thing I suggest is adding support for push constants to SPIRVShaderResources. We will need to add tests that will compile a simple shader with each resource type, create SPIRVShaderResources from byte code and test that the expected resource is properly reflected.

[TheMostDiligent]

#733 is merged - this is very good, thank you. One thing is that ideally we would want to also test DXC. Did you try it?

The next step is to add SPIRV patching to convert uniform block to push constants. I see it requires SPIRV-Tools, which means it will not be possible to use push constants if GLSLang is disabled. This should be OK as it is expected that the app provides fully ready bytecode in this case e.g. from archiver.