Chasing the shaders linkage errors

[bishopnator]

In my HLMS implementation when I am writing new parts of the shaders, I am struggling with identifying different kind of errors which occur in the parsed and compiled shaders. The most annoying is to hunt the linkage errors why are reported in D3D11 just after the DrawXXX calls (e.g. DrawIndexedInstanced.

I identified 2 problems and/or possible improvements for the ogre-next. The D3D11RenderSystem is missing in several methods the check for the errors - in my case the D3D11RenderSystem::render and D3D11RenderSystem::renderEmulated. If those calls "accumulate" the errors, they are not reported immediately and later when I e.g. map a buffer, I see the exception that vertex-pixel shader linkage fails, which doesn't make sense at this call of mapping a buffer - you can imagine, that those kind of errors are popping in ogre "randomly" - just when mDevice->isError() is called first after the failed DrawXXX call in D3D11 device. If the error checks could cause a performance problem, at least it is possible to wrap the code in #ifdef DEBUG / #endif macros.

Code: Select all

    //---------------------------------------------------------------------
    void D3D11RenderSystem::_renderEmulated( const CbDrawCallIndexed *cmd )
    {
        ID3D11DeviceContextN *deviceContext = mDevice.GetImmediateContext();

    CbDrawIndexed *drawCmd = reinterpret_cast<CbDrawIndexed *>( mSwIndirectBufferPtr +
                                                                (size_t)cmd->indirectBufferOffset );

    for( uint32 i = cmd->numDraws; i--; )
    {
        assert( !mDevice.isError() ); // <------------- THIS IS NEW
        deviceContext->DrawIndexedInstanced( drawCmd->primCount, drawCmd->instanceCount,
                                             drawCmd->firstVertexIndex, drawCmd->baseVertex,
                                             drawCmd->baseInstance );
        if( mDevice.isError() )  // <------------- THIS IS NEW
        {
            String errorDescription = mDevice.getErrorDescription();
            OGRE_EXCEPT( Exception::ERR_RENDERINGAPI_ERROR,
                         "D3D11 device failed to render DrawIndexedInstanced\nError Description:" +
                             errorDescription,
                         "D3D11RenderSystem::_renderEmulated" );
        }
        ++drawCmd;
    }
}

Another improvement could be placed in Hlms::compileShaderCode where the compiled shaders could be immediately checked for the correctness of output/input between the shader stages. At least for the D3D!! there is a D3DReflect function which could help with the analysis.

Following functions are generated by AI so take is just as untested example:

Code: Select all

#include <d3d11shader.h>
#include <d3dcompiler.h>
#include <string>
#include <iostream>
#include <unordered_map>

// Link against d3dcompiler.lib
#pragma comment(lib, "d3dcompiler.lib")

enum class ShaderSignatureType {
    Standard,     // VS->HS, HS->DS (Control Points), DS->GS, GS->PS
    PatchConstant // HS->DS (Patch Constants mapping)
};

bool VerifyPipelineStageLinkage(ID3DBlob* previousStageBlob, ID3DBlob* nextStageBlob, ShaderSignatureType sigType)
{
    if (!previousStageBlob || !nextStageBlob) return false;

ID3D11ShaderReflection* prevReflection = nullptr;
ID3D11ShaderReflection* nextReflection = nullptr;

D3DReflect(previousStageBlob->GetBufferPointer(), previousStageBlob->GetBufferSize(), 
           IID_ID3D11ShaderReflection, (void**)&prevReflection);
D3DReflect(nextStageBlob->GetBufferPointer(), nextStageBlob->GetBufferSize(), 
           IID_ID3D11ShaderReflection, (void**)&nextReflection);

D3D11_SHADER_DESC prevDesc;
D3D11_SHADER_DESC nextDesc;
prevReflection->GetDesc(&prevDesc);
nextReflection->GetDesc(&nextDesc);

struct InputSlot {
    std::string semanticName;
    UINT semanticIndex;
    BYTE mask; 
};
std::unordered_map<UINT, InputSlot> nextInputRegisterMap;

// 1. Map out target stage inputs based on validation type
UINT nextParamCount = (sigType == ShaderSignatureType::PatchConstant) ? nextDesc.PatchConstantParameters : nextDesc.InputParameters;

for (UINT i = 0; i < nextParamCount; ++i)
{
    D3D11_SIGNATURE_PARAMETER_DESC paramDesc;
    if (sigType == ShaderSignatureType::PatchConstant) {
        nextReflection->GetPatchConstantParameterDesc(i, &paramDesc);
    } else {
        nextReflection->GetInputParameterDesc(i, &paramDesc);
    }

    // Filter out system-generated variables like SV_TessFactor, SV_InsideTessFactor, SV_InstanceID, etc.
    // Hardware auto-generates these; the previous shader doesn't explicitly write them to a register.
    if (std::string(paramDesc.SemanticName).find("SV_") == 0 && 
        std::string(paramDesc.SemanticName) != "SV_Position") 
    {
        continue; 
    }

    InputSlot slot;
    slot.semanticName = paramDesc.SemanticName;
    slot.semanticIndex = paramDesc.SemanticIndex;
    slot.mask = paramDesc.Mask; 
    
    nextInputRegisterMap[paramDesc.Register] = slot;
}

// 2. Validate against output signatures of the previous stage
bool isLinkageValid = true;
UINT prevParamCount = (sigType == ShaderSignatureType::PatchConstant) ? prevDesc.PatchConstantParameters : prevDesc.OutputParameters;

for (UINT i = 0; i < prevParamCount; ++i)
{
    D3D11_SIGNATURE_PARAMETER_DESC paramDesc;
    if (sigType == ShaderSignatureType::PatchConstant) {
        prevReflection->GetPatchConstantParameterDesc(i, &paramDesc);
    } else {
        prevReflection->GetOutputParameterDesc(i, &paramDesc);
    }

    auto it = nextInputRegisterMap.find(paramDesc.Register);
    if (it != nextInputRegisterMap.end())
    {
        InputSlot& nextInput = it->second;

        // Rule A: Semantics must match
        if (nextInput.semanticName != paramDesc.SemanticName || 
            nextInput.semanticIndex != paramDesc.SemanticIndex)
        {
            std::cout << "Linkage Error [" << (sigType == ShaderSignatureType::PatchConstant ? "PatchConstant" : "Standard") << "]: "
                      << "Register " << paramDesc.Register << " semantic mismatch. "
                      << "Out: " << paramDesc.SemanticName << paramDesc.SemanticIndex
                      << " | In: " << nextInput.semanticName << nextInput.semanticIndex << std::endl;
            isLinkageValid = false;
        }

        // Rule B: Component verification (e.g., matching a float4 to a float4)
        if ((nextInput.mask & paramDesc.Mask) != nextInput.mask)
        {
            std::cout << "Linkage Error [" << (sigType == ShaderSignatureType::PatchConstant ? "PatchConstant" : "Standard") << "]: "
                      << "Component mask mismatch at Register " << paramDesc.Register 
                      << ". Next stage reads components missing from previous stage output." << std::endl;
            isLinkageValid = false;
        }

        nextInputRegisterMap.erase(it);
    }
}

// 3. Confirm all requested inputs were satisfied
if (!nextInputRegisterMap.empty())
{
    for (auto& pair : nextInputRegisterMap)
    {
        std::cout << "Linkage Error [" << (sigType == ShaderSignatureType::PatchConstant ? "PatchConstant" : "Standard") << "]: "
                  << "Next stage requires input " << pair.second.semanticName << pair.second.semanticIndex 
                  << " at Register " << pair.first << " but it is unwritten by the previous stage." << std::endl;
    }
    isLinkageValid = false;
}

prevReflection->Release();
nextReflection->Release();

return isLinkageValid;
}

And then the usage:

Code: Select all

bool VerifyFullPipeline(ID3DBlob* vs, ID3DBlob* hs, ID3DBlob* ds, ID3DBlob* gs, ID3DBlob* ps)
{
    bool valid = true;

if (hs && ds) {
    // 1. VS -> HS (Control Point interface)
    if (!VerifyPipelineStageLinkage(vs, hs, ShaderSignatureType::Standard)) valid = false;

    // 2. HS -> DS (Control Point interface)
    if (!VerifyPipelineStageLinkage(hs, ds, ShaderSignatureType::Standard)) valid = false;

    // 3. HS -> DS (Patch Constant function data structure)
    if (!VerifyPipelineStageLinkage(hs, ds, ShaderSignatureType::PatchConstant)) valid = false;

    // 4. DS -> Next Stage (GS or PS)
    ID3DBlob* nextStage = gs ? gs : ps;
    if (nextStage && !VerifyPipelineStageLinkage(ds, nextStage, ShaderSignatureType::Standard)) valid = false;
} 
else {
    // Standard Pipeline fallback (No tessellation active)
    ID3DBlob* nextStage = gs ? gs : ps;
    if (nextStage && !VerifyPipelineStageLinkage(vs, nextStage, ShaderSignatureType::Standard)) valid = false;
}

// 5. GS -> PS if Geometry Shader is used
if (gs && ps) {
    if (!VerifyPipelineStageLinkage(gs, ps, ShaderSignatureType::Standard)) valid = false;
}

return valid;
}

If something similar could be implemented also for GLSL and Vulkan, it could be possible to integrate the functionality with virtual methods in the RenderSystem. This moves the reporting of the errors directly to the parsing instead of reporting it much later during the command buffer execution.

What are the thoughts about it? I can try to create a pull request if it sounds like a good idea to have something like this in the ogre.

[dark_sylinc]

Regarding:

Code: Select all

for( uint32 i = cmd->numDraws; i--; )
    {
assert( !mDevice.isError() );

This should be moved up and use OGRE_ASSERT_MEDIUM:

Code: Select all

OGRE_ASSERT_MEDIUM( !mDevice.isError() );
for( uint32 i = cmd->numDraws; i--; )
    {

This is because the next bit of code already excepts if the device is on error. As for the other bit of code, I think it should only run on MEDIUM and wrapped in ogre_unlikely:

Code: Select all

#if OGRE_DEBUG_MODE >= OGRE_DEBUG_MEDIUM
        if( ogre_unlikely( mDevice.isError() ) )  // <------------- THIS IS NEW
        {
            String errorDescription = mDevice.getErrorDescription();
            OGRE_EXCEPT( Exception::ERR_RENDERINGAPI_ERROR,
                         "D3D11 device failed to render DrawIndexedInstanced\nError Description:" +
                             errorDescription,
                         "D3D11RenderSystem::_renderEmulated" );
        }
#endif

I can't comment on the AI generated code (I'm being lazy, sorry. Also I have a flu right now).

[bishopnator]

Hopefully get better soon. I will try to prepare the MR with the suggested changes and I will test the code from AI. For OpenGL and Vulkan it seems to be much easier as there is a direct function(s) to check the linking and I expect that it is somewhere in render system already integrated. I have to find out, how "far" from the HLMS parsing/compiling it is.

[dark_sylinc]

The only risk about verifying the linkage earlier are both false positives and false negatives. Sometimes the setups can get really complex which is why we just let it error when it's render time, as at that point we have everything.

[bishopnator]

False negative wouldn't be a big deal as later during the rendering the error will be popped by the RS. False positive is more problematic as it would feel like a bug in Ogre. It would be great to filter out (and ignore) very complex cases and just do simple verification. I am not sure in which cases it could be false positive. I would like to just "purely" verify that the output from one stage is compatible with input of the next stage (so all the input data could be mapped from the output of the previous stage). I am not quite sure, whether the type changes are accepted by the driver, but I find it as bad design of the shaders if the types doesn't match between the stages. Maybe such behavior could be tweaked with the ogre's configuration so everybody could decide during build the ogre whether this kind of checks in HLMS are desired or not.

At least those error checks after the draw calls could be integrated, couldn't they?

[dark_sylinc]

At least those error checks after the draw calls could be integrated, couldn't they?

Yes

[bishopnator]

Created a pull request: https://github.com/OGRECave/ogre-next/pull/594

I will check and verify the implementation which I pasted in my initial post. If I get relatively good feeling from the it, I will try to create separate PR with changes which check for the linkage errors in the Hlms::createShaderCacheEntry.

[bishopnator]

Here is the second pull request: https://github.com/OGRECave/ogre-next/pull/595
I created a linkage errors in PbsHlms scripts and checked the behavior of this new functionality and it detects the failures correct. However there are multiple ways how to declare output/input variables in GLSL and it is out of my knowledge to test everything very deep.

Regarding of D3D11 implementation, it was necessary to slightly modify the code from my initial post - there is unordered_map which maps a register to slot and the problem is that multiple variables can share same register (e.g. if there is input variable "uint drawId" followed by "float myvar". Both values are then packed in the same register (drawId as x-value and myvar as y-value). I updated this mapping there. Also logging of course - instead of cout, the ogre's logging is used. Rest is same.