From f9b86f3a8290621510978d70818295fa0cc960b3 Mon Sep 17 00:00:00 2001 From: Steve Karolewics Date: Sat, 9 May 2020 14:32:29 -0700 Subject: [PATCH] Fix broken D3D shader linkage test cases The rules for merging semantics into an array were not strict enough. If either the type, interpolation mode, or size is different, or if it is not using a register's x component, then we can't combine it. Also, the rules for marking a semantic as array length 1 were too strict, resulting in some semantics packing into other registers incorrectly. --- renderdoc/driver/shaders/dxbc/dxbc_debug.cpp | 123 +++++++++--------- .../demos/d3d11/d3d11_shader_linkage_zoo.cpp | 26 ++-- .../demos/d3d12/d3d12_shader_linkage_zoo.cpp | 26 ++-- 3 files changed, 95 insertions(+), 80 deletions(-) diff --git a/renderdoc/driver/shaders/dxbc/dxbc_debug.cpp b/renderdoc/driver/shaders/dxbc/dxbc_debug.cpp index 2a3a5328c..cb50c8ba9 100644 --- a/renderdoc/driver/shaders/dxbc/dxbc_debug.cpp +++ b/renderdoc/driver/shaders/dxbc/dxbc_debug.cpp @@ -4666,6 +4666,47 @@ void LookupSRVFormatFromShaderReflection(const DXBC::Reflection &reflection, } } +DXBCBytecode::InterpolationMode GetInterpolationModeForInputParam(const SigParameter &sig, + const DXBC::Reflection &psDxbc, + const DXBCBytecode::Program *program) +{ + if(sig.varType == VarType::SInt || sig.varType == VarType::UInt) + return DXBCBytecode::InterpolationMode::INTERPOLATION_CONSTANT; + + if(sig.varType == VarType::Float) + { + // if we're packed with ints on either side, we must be nointerpolation + size_t numInputs = psDxbc.InputSig.size(); + for(size_t j = 0; j < numInputs; j++) + { + if(sig.regIndex == psDxbc.InputSig[j].regIndex && psDxbc.InputSig[j].varType != VarType::Float) + return DXBCBytecode::InterpolationMode::INTERPOLATION_CONSTANT; + } + + DXBCBytecode::InterpolationMode interpolation = DXBCBytecode::INTERPOLATION_UNDEFINED; + + if(program) + { + for(size_t d = 0; d < program->GetNumDeclarations(); d++) + { + const DXBCBytecode::Declaration &decl = program->GetDeclaration(d); + + if(decl.declaration == DXBCBytecode::OPCODE_DCL_INPUT_PS && + decl.operand.indices[0].absolute && decl.operand.indices[0].index == sig.regIndex) + { + interpolation = decl.interpolation; + break; + } + } + } + + return interpolation; + } + + RDCERR("Unexpected input signature type: %s", ToStr(sig.varType).c_str()); + return DXBCBytecode::InterpolationMode::INTERPOLATION_UNDEFINED; +} + void GatherPSInputDataForInitialValues(const DXBC::DXBCContainer *dxbc, const DXBC::Reflection &prevStageDxbc, rdcarray &initialValues, @@ -4793,50 +4834,11 @@ void GatherPSInputDataForInitialValues(const DXBC::DXBCContainer *dxbc, nextreg = sig.regIndex + 1; - if(sig.varType == VarType::Float) - { - // if we're packed with ints on either side, we must be nointerpolation - bool nointerp = false; - for(size_t j = 0; j < numInputs; j++) - { - if(sig.regIndex == psDxbc.InputSig[j].regIndex && psDxbc.InputSig[j].varType != VarType::Float) - { - nointerp = true; - break; - } - } - - DXBCBytecode::InterpolationMode interpolation = DXBCBytecode::INTERPOLATION_UNDEFINED; - - if(program) - { - for(size_t d = 0; d < program->GetNumDeclarations(); d++) - { - const DXBCBytecode::Declaration &decl = program->GetDeclaration(d); - - if(decl.declaration == DXBCBytecode::OPCODE_DCL_INPUT_PS && - decl.operand.indices[0].absolute && decl.operand.indices[0].index == sig.regIndex) - { - interpolation = decl.interpolation; - break; - } - } - } - - if(nointerp) - psInputDefinition += "nointerpolation "; - else if(interpolation != DXBCBytecode::INTERPOLATION_UNDEFINED && - interpolation != DXBCBytecode::INTERPOLATION_CONSTANT) - psInputDefinition += ToStr(interpolation) + " "; - - psInputDefinition += "float"; - } - else if(sig.varType == VarType::SInt) - psInputDefinition += "nointerpolation int"; - else if(sig.varType == VarType::UInt) - psInputDefinition += "nointerpolation uint"; - else - RDCERR("Unexpected input signature type: %s", ToStr(sig.varType).c_str()); + DXBCBytecode::InterpolationMode interpolation = + GetInterpolationModeForInputParam(sig, psDxbc, program); + if(interpolation != DXBCBytecode::INTERPOLATION_UNDEFINED) + psInputDefinition += ToStr(interpolation) + " "; + psInputDefinition += ToStr(sig.varType); int numCols = (sig.regChannelMask & 0x1 ? 1 : 0) + (sig.regChannelMask & 0x2 ? 1 : 0) + (sig.regChannelMask & 0x4 ? 1 : 0) + (sig.regChannelMask & 0x8 ? 1 : 0); @@ -4869,17 +4871,22 @@ void GatherPSInputDataForInitialValues(const DXBC::DXBCContainer *dxbc, for(size_t j = i + 1; j < numInputs; j++) { - // if we've found the 'next' semantic - if(sig.semanticName == psDxbc.InputSig[j].semanticName && - nextIdx == psDxbc.InputSig[j].semanticIndex) - { - int jNumCols = (psDxbc.InputSig[j].regChannelMask & 0x1 ? 1 : 0) + - (psDxbc.InputSig[j].regChannelMask & 0x2 ? 1 : 0) + - (psDxbc.InputSig[j].regChannelMask & 0x4 ? 1 : 0) + - (psDxbc.InputSig[j].regChannelMask & 0x8 ? 1 : 0); + const SigParameter &jSig = psDxbc.InputSig[j]; - // if it's the same size, and it's at the start of the next register - if(jNumCols == numCols && psDxbc.InputSig[j].regChannelMask <= 0x3) + // if we've found the 'next' semantic + if(sig.semanticName == jSig.semanticName && nextIdx == jSig.semanticIndex) + { + int jNumCols = (jSig.regChannelMask & 0x1 ? 1 : 0) + (jSig.regChannelMask & 0x2 ? 1 : 0) + + (jSig.regChannelMask & 0x4 ? 1 : 0) + (jSig.regChannelMask & 0x8 ? 1 : 0); + + DXBCBytecode::InterpolationMode jInterp = + GetInterpolationModeForInputParam(jSig, psDxbc, program); + + // if it's the same size, type, and interpolation mode, then it could potentially be + // packed into an array. Check if it's using the first channel component to tell whether + // it's tightly packed with another semantic. + if(jNumCols == numCols && interpolation == jInterp && sig.varType == jSig.varType && + jSig.regChannelMask & 0x1) { if(arrayLength == 0) arrayLength = 2; @@ -4908,13 +4915,13 @@ void GatherPSInputDataForInitialValues(const DXBC::DXBCContainer *dxbc, // detected but it WILL be put in its own register (not packed together), so detect this // case too. // Note we have to search *backwards* because we need to know if this register should have - // been packed into the previous register, but wasn't. float/float2 can be packed after an - // array just fine. - if(included && i > 0 && arrayLength == 0 && numCols <= 2 && sig.regChannelMask <= 0x3) + // been packed into the previous register, but wasn't. float/float2/float3 can be packed after + // an array just fine, so long as the sum of their components doesn't exceed a register width + if(included && i > 0 && arrayLength == 0) { const SigParameter &prev = psDxbc.InputSig[i - 1]; - if(prev.regIndex != sig.regIndex && prev.compCount <= 2 && prev.regChannelMask <= 0x3) + if(prev.regIndex != sig.regIndex && prev.compCount + sig.compCount <= 4) arrayLength = 1; } diff --git a/util/test/demos/d3d11/d3d11_shader_linkage_zoo.cpp b/util/test/demos/d3d11/d3d11_shader_linkage_zoo.cpp index 4b88758f0..f3eef81da 100644 --- a/util/test/demos/d3d11/d3d11_shader_linkage_zoo.cpp +++ b/util/test/demos/d3d11/d3d11_shader_linkage_zoo.cpp @@ -264,6 +264,11 @@ float4 main(v2f IN) : SV_Target0 {false, VarType::Float, 3, 0, "TEXCOORD1", true}, {false, VarType::Float, 2, 0, "TEXCOORD2", true}})); + tests.push_back(BuildTestCase({{false, VarType::Float, 2, 1, "TEXCOORD0", true}, + {false, VarType::Float, 2, 1, "TEXCOORD1", true}, + {false, VarType::Float, 3, 2, "TEXCOORD2", true}, + {false, VarType::Float, 2, 0, "TEXCOORD4", true}})); + // Semantics that don't pack together due to being arrays tests.push_back(BuildTestCase({{false, VarType::Float, 1, 2, "TEXCOORD0", true}})); tests.push_back(BuildTestCase({{false, VarType::Float, 2, 1, "TEXCOORD0", true}, @@ -276,26 +281,25 @@ float4 main(v2f IN) : SV_Target0 // Tests focusing on different interpolation modes tests.push_back(BuildTestCase({{false, VarType::Float, 2, 0, "TEXCOORD0", true}, {true, VarType::Float, 2, 0, "TEXCOORD1", true}})); - // The following test is currently broken: the semantics live in v1.x and v1.y, but during - // debugging our initial inputs shader places them in an array[2], resulting in v1.x and v2.x + // These semantics are placed in v1.x and v1.y since they share interpolation modes and types + // (all int semantics are nointerpolation). Test that they don't get placed in v1.x and v2.x tests.push_back(BuildTestCase({{false, VarType::UInt, 1, 0, "TEXCOORD0", true}, {true, VarType::UInt, 1, 0, "TEXCOORD1", true}})); - // The following test is currently broken: the semantics live in v1.x and v2.x, but during - // debugging our initial inputs shader places them in an array[2], resulting in the correct - // register placement, but incorrect interpolation modes since uints are always nointerpolation + // These semantics are placed in v1.x and v2.x since their interpolation modes differ. Test that + // they don't turn into an array[2] which would result in an erroneous interpolation mode for + // one semantic or the other tests.push_back(BuildTestCase({{false, VarType::Float, 1, 0, "TEXCOORD0", true}, {false, VarType::UInt, 1, 0, "TEXCOORD1", true}})); - // The following test is currently broken: the semantics live in v1.x and v1.y, despite having - // different types since the interpolation mode is the same. During debugging our initial inputs - // shader places them in an array[2], resulting in the wrong register placement + // These semantics are placed in v1.x and v1.y despite having different types since the + // interpolation mode is the same. Test that they don't turn into an array[2] which would place + // them in the wrong registers tests.push_back(BuildTestCase({{true, VarType::Float, 1, 0, "TEXCOORD0", true}, {false, VarType::UInt, 1, 0, "TEXCOORD1", true}})); // Bespoke tests for broken scenarios discovered through bug reports: - // The following test is currently broken: the semantics live in v1.xy, v2.x, and v3.xyz due - // to each being an array. During debugging, out initial input shader defines the last semantic - // without an array size, so FXC packs it into v2.yzw + // These semantics live in v1.xy, v2.x, and v3.xyz due to each being an array. If any of them + // are not treated as an array[1], they will incorrectly pack together with a previous semantic tests.push_back(BuildTestCase({{false, VarType::Float, 2, 1, "TEXCOORD0", true}, {false, VarType::Float, 1, 1, "TEXCOORD1", false}, {false, VarType::Float, 3, 1, "TEXCOORD2", true}})); diff --git a/util/test/demos/d3d12/d3d12_shader_linkage_zoo.cpp b/util/test/demos/d3d12/d3d12_shader_linkage_zoo.cpp index ba877f837..3d6b62511 100644 --- a/util/test/demos/d3d12/d3d12_shader_linkage_zoo.cpp +++ b/util/test/demos/d3d12/d3d12_shader_linkage_zoo.cpp @@ -250,6 +250,11 @@ float4 main(v2f IN) : SV_Target0 {false, VarType::Float, 3, 0, "TEXCOORD1", true}, {false, VarType::Float, 2, 0, "TEXCOORD2", true}})); + psos.push_back(BuildPSO(sig, {{false, VarType::Float, 2, 1, "TEXCOORD0", true}, + {false, VarType::Float, 2, 1, "TEXCOORD1", true}, + {false, VarType::Float, 3, 2, "TEXCOORD2", true}, + {false, VarType::Float, 2, 0, "TEXCOORD4", true}})); + // Semantics that don't pack together due to being arrays psos.push_back(BuildPSO(sig, {{false, VarType::Float, 1, 2, "TEXCOORD0", true}})); psos.push_back(BuildPSO(sig, {{false, VarType::Float, 2, 1, "TEXCOORD0", true}, @@ -262,26 +267,25 @@ float4 main(v2f IN) : SV_Target0 // Tests focusing on different interpolation modes psos.push_back(BuildPSO(sig, {{false, VarType::Float, 2, 0, "TEXCOORD0", true}, {true, VarType::Float, 2, 0, "TEXCOORD1", true}})); - // The following test is currently broken: the semantics live in v1.x and v1.y, but during - // debugging our initial inputs shader places them in an array[2], resulting in v1.x and v2.x + // These semantics are placed in v1.x and v1.y since they share interpolation modes and types + // (all int semantics are nointerpolation). Test that they don't get placed in v1.x and v2.x psos.push_back(BuildPSO(sig, {{false, VarType::UInt, 1, 0, "TEXCOORD0", true}, {true, VarType::UInt, 1, 0, "TEXCOORD1", true}})); - // The following test is currently broken: the semantics live in v1.x and v2.x, but during - // debugging our initial inputs shader places them in an array[2], resulting in the correct - // register placement, but incorrect interpolation modes since uints are always nointerpolation + // These semantics are placed in v1.x and v2.x since their interpolation modes differ. Test that + // they don't turn into an array[2] which would result in an erroneous interpolation mode for + // one semantic or the other psos.push_back(BuildPSO(sig, {{false, VarType::Float, 1, 0, "TEXCOORD0", true}, {false, VarType::UInt, 1, 0, "TEXCOORD1", true}})); - // The following test is currently broken: the semantics live in v1.x and v1.y, despite having - // different types since the interpolation mode is the same. During debugging our initial inputs - // shader places them in an array[2], resulting in the wrong register placement + // These semantics are placed in v1.x and v1.y despite having different types since the + // interpolation mode is the same. Test that they don't turn into an array[2] which would place + // them in the wrong registers psos.push_back(BuildPSO(sig, {{true, VarType::Float, 1, 0, "TEXCOORD0", true}, {false, VarType::UInt, 1, 0, "TEXCOORD1", true}})); // Bespoke tests for broken scenarios discovered through bug reports: - // The following test is currently broken: the semantics live in v1.xy, v2.x, and v3.xyz due - // to each being an array. During debugging, out initial input shader defines the last semantic - // without an array size, so FXC packs it into v2.yzw + // These semantics live in v1.xy, v2.x, and v3.xyz due to each being an array. If any of them + // are not treated as an array[1], they will incorrectly pack together with a previous semantic psos.push_back(BuildPSO(sig, {{false, VarType::Float, 2, 1, "TEXCOORD0", true}, {false, VarType::Float, 1, 1, "TEXCOORD1", false}, {false, VarType::Float, 3, 1, "TEXCOORD2", true}}));