Add ConvertUBOToPushConstants and corresponding tests

[hzqst]

anything else?

[TheMostDiligent]

This is the critical part of the push constants implementation, and is very complicated, so it needs to be very thoroughly tested.

I suggest two tests:
First, add tests to SPIRVShaderResourcesTest:

• Compile the shader with GLSLang/DXC
• Patch UBO to push constants
• Create SPIRVShaderResources and verify the push constants block

This, however, is not enough as the byte code may still be incorrect. So the second test will actually use the patched shader to draw an image:

• The test will use Vulkan directly (since we are testing low-level byte code patching logic) and should be added to Vulkan-specific folder
• Similar to other tests, it will render the reference two-triangle image
• The shader with UBO will be compiled to SPIRV and patched
  • Try to cover as many possible paths in the patching logic as possible (e.g. name refers to a variable or a struct, nested structs, propagate storage class, etc.)
• The PSO will be created in native Vulkan and used to render test image
• Test image will be compared to the reference

[TheMostDiligent]

You should set DILIGENT_NO_FORMAT_VALIDATION CMake option to NO. This will add format validation targets to the build. The build will fail if there any formatting issues.

[hzqst]

So the second test will actually use the patched shader to draw an image:

will do in the next pr, once this get merged.

[TheMostDiligent]

Can you add a few more tests that test more paths through the patching logic?
I don't know what exactly is need, looks like maybe nested structs?

[TheMostDiligent]

I'd suggest removing this namespace, and adding SpvOptimizerMessageConsumer and SpvTargetEnvFromSPIRV to the SPIRVTools.hpp

[hzqst]

SpvOptimizerMessageConsumer and SpvTargetEnvFromSPIRV should be internally used in SPIRVTools.cpp/ConvertUBOToPushConstant.cpp and not be exposed to other compile units where SPIRVTools.hpp might be included.

[hzqst]

Btw adding SpvOptimizerMessageConsumer and SpvTargetEnvFromSPIRV to the SPIRVTools.hpp leads to unused function warning on gcc

[TheMostDiligent]

Can you please check these AI review points:

Overall, this pass is conceptually correct and aligns well with how SPIRV-Tools handles storage-class fixes internally. It correctly:

• Locates a UBO by name
• Converts it from Uniform to PushConstant
• Updates pointer-producing instructions
• Removes Binding and DescriptorSet decorations
• Produces validator-friendly SPIR-V in the common case

That said, there are several correctness and robustness issues that should be addressed before relying on this pass long-term.

---

✅ What Looks Correct

• Storage class consistency

  • Both the OpTypePointer storage class and the OpVariable storage class operand are updated.
  • This is required by the SPIR-V validator and is a common source of bugs in custom passes.

• Decoration cleanup

  • Removing Binding and DescriptorSet is correct once the variable becomes a push constant.

• Pointer propagation

  • Handling of OpAccessChain, OpPtrAccessChain, OpPhi, OpSelect, OpCopyObject, etc. matches common pointer-flow patterns and typical SPIRV-Tools logic.

Potential issues

Issue 1: Analyses/managers may be stale after mutating IR

This pass changes the module in ways that invalidate cached analyses:

• Updates the variable result type (SetResultType)
• Updates the OpVariable storage class operand (SetInOperand)
• Creates/uses new pointer types (FindPointerToType)
• Removes decorations via DecorationManager

However, after these mutations the code continues using cached managers (DefUseManager, TypeManager, DecorationManager) without explicitly invalidating/rebuilding analyses. This can cause stale def-use chains or stale type/decoration views, leading to version-dependent or intermittent failures.

Why this matters

• ForEachUser(...) and subsequent propagation relies on correct def-use chains.
• Decoration removal and type queries may operate on outdated internal state if analyses are not refreshed after IR edits.

Suggested fix
After completing the storage class / type mutation of the target variable (and any type insertion/moves), invalidate analyses before walking users / removing decorations:

// After changing OpVariable and pointer types:
context()->InvalidateAnalyses(spvtools::opt::IRContext::kAnalysisAll);

// Reacquire managers if you store pointers/references to them
auto* def_use  = context()->get_def_use_mgr();
auto* type_mgr = context()->get_type_mgr();
auto* deco_mgr = context()->get_decoration_mgr();

At minimum, invalidate the analyses that are relied upon later in the pass (def-use, type, decoration), but kAnalysisAll is the safest option unless performance is a concern.

Issue 2: Reordering the newly created pointer type is fragile and unnecessary

The pass attempts to manually adjust type ordering by moving the newly created pointer type instruction using InsertAfter(pointee_type_inst).

This is risky for several reasons:

• FindPointerToType() may return an existing pointer type that is already in a valid location. Moving it can unintentionally reshuffle the IR and affect unrelated instructions.
• Manual instruction reordering in the types_values() section can break assumptions in cached analyses and other SPIRV-Tools utilities unless all analyses are invalidated and rebuilt afterward.
• SPIR-V does not require pointer types to appear in any specific relative order, as long as they are valid type declarations.

Suggested fix:

• Remove the pointer-type reordering logic entirely.
• If there is a proven driver or toolchain requirement that depends on type ordering, handle it explicitly and always invalidate and rebuild analyses after moving instructions.

In most cases, relying on SPIRV-Tools’ own type creation and placement behavior is the most stable and maintainable approach.

Issue 3: OpName-based lookup is ambiguous and may target the wrong variable

The pass identifies the target UBO by stopping at the first OpName instruction whose string matches the provided block name. This approach is inherently ambiguous and can select the wrong object.

Why this is problematic:

• Struct types and variables commonly share the same debug name.
• OpName strings are not guaranteed to be unique within a module.
• Linked or multi-entry-point modules may contain multiple objects with the same name.
• Debug names are optional and may be stripped or altered by earlier toolchains.

As a result, the pass may convert the wrong variable or type, leading to invalid or unintended SPIR-V output.

Suggested fix:

• Make the lookup deterministic instead of relying on the first matching OpName.
• Prefer selecting a Uniform-storage OpVariable whose pointee type is a Block-decorated struct.
• Alternatively, identify a variable that has both Binding and DescriptorSet decorations, as this reliably indicates a UBO.
• If starting from a named struct type, explicitly search for the unique uniform variable that references it.

Using semantic properties (storage class and decorations) rather than debug names will make the pass significantly more robust and predictable

Issue 4: Decoration removal is incomplete and may leave invalid metadata

The pass removes Binding and DescriptorSet decorations from the converted variable, which is required when switching a UBO to a push constant. However, this may not be sufficient in all cases.

Why this can be a problem:

• Some tools attach resource-related decorations at the struct type level rather than directly on the variable.
• Additional decorations (for example, access qualifiers or other resource metadata) may be meaningless or invalid for push constants.
• Leaving stale decorations can cause validation failures or subtle driver-specific issues.

Suggested fix:

• Clearly define which decorations are valid for push constants and remove any that are not applicable.
• Consider checking for and cleaning up decorations on both the variable and its pointee struct type.
• At minimum, document the assumptions about which decorations are expected to exist and which are intentionally left untouched.

This will make the transformation safer and easier to reason about when integrating shaders from different toolchains.

Issue 5: Pointer-type propagation is not comprehensive and may miss valid pointer flows

The pass updates the storage class for pointer-typed results produced by a limited set of opcodes (for example, access chains, phi/select, and copy-object). This covers common compiler output, but it is not guaranteed to cover all valid pointer flows that can occur in SPIR-V, especially across different frontends, optimization levels, or extensions.

Why this can be a problem:

• SPIR-V evolution and extensions can introduce additional pointer-producing instructions or patterns.
• Even within core SPIR-V, different compilers may express pointer flow through instructions not currently handled here.
• Missing a pointer-producing opcode means some downstream pointer-typed values may retain Uniform pointer types after the root variable becomes PushConstant, potentially causing type/storage-class mismatches and validator errors.

Suggested fix:

• Treat this as a correctness surface area: either broaden coverage to include all pointer-producing instructions relevant to your supported SPIR-V versions, or implement a more general propagation strategy.
• Add assertions or validation checks that detect remaining Uniform pointer types derived from the converted variable after the pass runs.
• Consider adding targeted tests with representative SPIR-V from multiple frontends (GLSLang, DXC, etc.) to ensure pointer propagation stays correct as inputs vary.

This reduces the risk of “works for my shaders” behavior and makes the pass more resilient to future SPIR-V patterns.

[hzqst]

Issue 1: Analyses/managers may be stale after mutating IR

This pass changes the module in ways that invalidate cached analyses:

No need to fix:

SuccessWithChange leads to InvalidateAnalysesExceptFor(GetPreservedAnalyses()) which automatically handles analysis invalidation:

1. The Pass::Run() method automatically calls InvalidateAnalysesExceptFor(GetPreservedAnalyses()) when a pass returns SuccessWithChange (see pass.cpp lines 40-42).

2. The current implementation correctly returns kAnalysisNone from GetPreservedAnalyses(), which means all analyses will be invalidated after the pass completes.

3. During pass execution, using context()->UpdateDefUse(inst) incrementally updates def-use chains, and FindPointerToType() properly updates related analyses.

4. Reference implementations in SPIRV-Tools (e.g., fix_storage_class.cpp, private_to_local_pass.cpp) follow the same pattern without manual invalidation calls.

[hzqst]

Issue 2: Reordering the newly created pointer type is fragile and unnecessary

reordering logic has been removed.

[hzqst]

Issue 3: OpName-based lookup is ambiguous and may target the wrong variable

addressed by looping through all resources with matched OpName(s) and applying block storage class verification & decoration verification on each resource:

for (uint32_t named_id : candidate_ids)
{
//......

//Only resource with StorageClass::Uniform matched
if (storage_class != spv::StorageClass::Uniform)
    continue;

//Only resource with BlockDecoration matched
if (HasBlockDecoration(pointee_type_id))

btw it's not legit to have two resources with same resource name but different resource type in the global scope in HLSL/GLSL. not really have to worry about "Struct types and variables commonly share the same debug name."

[hzqst]

Issue 4: Decoration removal is incomplete and may leave invalid metadata

No need to fix:

1. Binding and DescriptorSet decorations are UBO-specific and must be removed (correctly handled).

2. Block decoration on the struct type is required for both UBOs and Push Constants, so it should remain (correctly preserved).

3. Other decorations (e.g., Offset, MatrixStride, ArrayStride, ColMajor, RowMajor) are valid for both UBOs and Push Constants according to the SPIR-V specification, so they should remain.

4. Push Constants and UBOs share most decoration semantics - the main difference is the storage class and the absence of Binding/DescriptorSet for push constants.

[hzqst]

Issue 5: Pointer-type propagation is not comprehensive and may miss valid pointer flows

addressed by expanding opcode coverage and adding defensive assertions:

1. explicit handling for OpFunctionCall

   • Returns false with a comment explaining that function call results cannot be safely updated without inlining

2. explicit handling for additional opcodes

   • OpImageTexelPointer, OpBitcast, OpVariable

3. Added UNEXPECTED assertion in default case

The current impl now follows the pattern used in SPIRV-Tools/source/opt/fix_storage_class.cpp, which is the reference implementation for storage class propagation.

[hzqst]

Can you add a few more tests that test more paths through the patching logic? I don't know what exactly is need, looks like maybe nested structs?

nested structs test added:

// Test for nested constant buffers

struct InnerConstant_t
{
    float4 g_Color;
};

struct InnerConstant2_t
{
    float4 g_MaterialParams;
};

struct MergedConstant_t
{
    InnerConstant_t InnerConstant;
    InnerConstant2_t InnerConstant2;
};

cbuffer CB3
{
    MergedConstant_t g_MergedConstant;
};

cbuffer CB4
{
    InnerConstant_t g_InnerConstant;
    InnerConstant2_t g_InnerConstant2;
};

[TheMostDiligent]

Do the tests cover all paths through Process()?

[hzqst]

Do the tests cover all paths through Process()?

dym also with failure paths ?

[TheMostDiligent]

For a missing uniform - yes, would be nice to have.
Are there other failure paths for a valid SPIRV?

[hzqst]

For a missing uniform - yes, would be nice to have. Are there other failure paths for a valid SPIRV?

Additional tests with InvalidBlockName / InvalidResourceType added: Patching non-existing uniform buffer block or non-uniform-buffer-resource leads to error now.