From 671add403597d65b125b3c2c2229f53bc724f2db Mon Sep 17 00:00:00 2001 From: Steve Karolewics Date: Fri, 7 Feb 2020 09:24:19 -0800 Subject: [PATCH] Fix shader debugging and pipeline viewer with constant buffer arrays With SM5.1 and D3D12, constant buffer arrays can be declared which are treated as an array of resources on the shader side, and a simple collection of descriptors on the D3D12 side. DXBCDebug::GlobalState handles the bridge between these by storing each array resource as a nested ShaderVariable struct. Accessing the data for instructions traverses the structure similarly. The shader viewer handles nested resources and displays them in the appropriate tree view. In the pipeline viewer, descriptors for CB arrays now indicate the array index and correctly handle buffer offsets for viewing. --- .../Windows/ConstantBufferPreviewer.cpp | 6 ++ .../D3D12PipelineStateViewer.cpp | 16 ++-- qrenderdoc/Windows/ShaderViewer.cpp | 35 +++++++- renderdoc/api/replay/pipestate.h | 5 +- renderdoc/api/replay/pipestate.inl | 5 +- renderdoc/driver/d3d12/d3d12_shaderdebug.cpp | 6 +- renderdoc/driver/shaders/dxbc/dxbc_debug.cpp | 82 +++++++++++++++---- .../driver/shaders/dxbc/dxbc_reflect.cpp | 2 +- 8 files changed, 131 insertions(+), 26 deletions(-) diff --git a/qrenderdoc/Windows/ConstantBufferPreviewer.cpp b/qrenderdoc/Windows/ConstantBufferPreviewer.cpp index 03bb19591..582a4465f 100644 --- a/qrenderdoc/Windows/ConstantBufferPreviewer.cpp +++ b/qrenderdoc/Windows/ConstantBufferPreviewer.cpp @@ -144,8 +144,11 @@ void ConstantBufferPreviewer::OnEventChanged(uint32_t eventId) setVariables(vars); if(wasEmpty) { + // Expand before resizing so that collapsed data will already be visible when expanded + ui->variables->expandAll(); for(int i = 0; i < 3; i++) ui->variables->resizeColumnToContents(i); + ui->variables->collapseAll(); } ui->variables->applyExpansion(state, 0); }); @@ -169,8 +172,11 @@ void ConstantBufferPreviewer::OnEventChanged(uint32_t eventId) setVariables(vars); if(wasEmpty) { + // Expand before resizing so that collapsed data will already be visible when expanded + ui->variables->expandAll(); for(int i = 0; i < 3; i++) ui->variables->resizeColumnToContents(i); + ui->variables->collapseAll(); } // if we have saved expansion state for the new shader, apply it, otherwise apply the diff --git a/qrenderdoc/Windows/PipelineState/D3D12PipelineStateViewer.cpp b/qrenderdoc/Windows/PipelineState/D3D12PipelineStateViewer.cpp index 39c683eda..2830a50f9 100644 --- a/qrenderdoc/Windows/PipelineState/D3D12PipelineStateViewer.cpp +++ b/qrenderdoc/Windows/PipelineState/D3D12PipelineStateViewer.cpp @@ -59,21 +59,22 @@ struct D3D12CBufTag D3D12CBufTag() { idx = ~0U; - space = reg = 0; + space = reg = arrayIdx = 0; } D3D12CBufTag(uint32_t s, uint32_t r) { idx = ~0U; space = s; reg = r; + arrayIdx = 0; } D3D12CBufTag(uint32_t i) { idx = i; - space = reg = 0; + space = reg = arrayIdx = 0; } - uint32_t idx, space, reg; + uint32_t idx, space, reg, arrayIdx; }; Q_DECLARE_METATYPE(D3D12CBufTag); @@ -1169,7 +1170,9 @@ void D3D12PipelineStateViewer::setShaderState(const D3D12Pipe::Shader &stage, RD { bind = &bm; shaderCBuf = &res; - tag = QVariant::fromValue(D3D12CBufTag(i)); + D3D12CBufTag cbufTag(i); + cbufTag.arrayIdx = reg - bm.bind; + tag = QVariant::fromValue(cbufTag); break; } } @@ -1213,6 +1216,9 @@ void D3D12PipelineStateViewer::setShaderState(const D3D12Pipe::Shader &stage, RD if(shaderCBuf && !shaderCBuf->name.empty()) regname += lit(": ") + shaderCBuf->name; + if(bind != NULL && bind->arraySize > 1) + regname += tr("[%1]").arg(reg - bind->bind); + QString sizestr; if(bytesize == (uint32_t)length) sizestr = tr("%1 Variables, %2 bytes").arg(numvars).arg(length); @@ -2018,7 +2024,7 @@ void D3D12PipelineStateViewer::cbuffer_itemActivated(RDTreeWidgetItem *item, int return; } - IConstantBufferPreviewer *prev = m_Ctx.ViewConstantBuffer(stage->stage, cb.idx, 0); + IConstantBufferPreviewer *prev = m_Ctx.ViewConstantBuffer(stage->stage, cb.idx, cb.arrayIdx); m_Ctx.AddDockWindow(prev->Widget(), DockReference::TransientPopupArea, this, 0.3f); } diff --git a/qrenderdoc/Windows/ShaderViewer.cpp b/qrenderdoc/Windows/ShaderViewer.cpp index 8a1847844..61491f2c4 100644 --- a/qrenderdoc/Windows/ShaderViewer.cpp +++ b/qrenderdoc/Windows/ShaderViewer.cpp @@ -1765,13 +1765,17 @@ void ShaderViewer::combineStructures(RDTreeWidgetItem *root, int skipPrefixLengt continue; } - // sort the children by offset, then name (in case all offsets are empty) + // Sort the children by offset, then global source var index, then by text. + // Using the global source var index allows resource arrays to be presented in index order + // rather than by name, so for example arr[2] comes before arr[10] std::sort(matches.begin(), matches.end(), [](const RDTreeWidgetItem *a, const RDTreeWidgetItem *b) { VariableTag at = a->tag().value(); VariableTag bt = b->tag().value(); if(at.offset != bt.offset) return at.offset < bt.offset; + if(at.globalSourceVar != bt.globalSourceVar) + return at.globalSourceVar < bt.globalSourceVar; return a->text(0) < b->text(0); }); @@ -2188,6 +2192,26 @@ void ShaderViewer::updateDebugState() node->addChild(child); } } + else + { + // Check if this is a constant buffer array + int arrayCount = m_Trace->constantBlocks[i].members[j].members.count(); + for(int k = 0; k < arrayCount; k++) + { + if(m_Trace->constantBlocks[i].members[j].members[k].rows > 0 || + m_Trace->constantBlocks[i].members[j].members[k].columns > 0) + { + name = m_Trace->constantBlocks[i].members[j].members[k].name; + RDTreeWidgetItem *child = + new RDTreeWidgetItem({name, name, lit("Constant"), + stringRep(m_Trace->constantBlocks[i].members[j].members[k])}); + node->setTag(QVariant::fromValue( + VariableTag(DebugVariableReference(DebugVariableType::Constant, name)))); + + node->addChild(child); + } + } + } } if(node->childCount() == 0) @@ -2871,13 +2895,22 @@ const ShaderVariable *ShaderViewer::GetRegisterVariable(const DebugVariableRefer { for(int i = 0; i < m_Trace->constantBlocks.count(); i++) { + // Root constant if(m_Trace->constantBlocks[i].name == r.name) return &m_Trace->constantBlocks[i]; for(int j = 0; j < m_Trace->constantBlocks[i].members.count(); j++) { + // Variable in constant buffer if(m_Trace->constantBlocks[i].members[j].name == r.name) return &m_Trace->constantBlocks[i].members[j]; + + for(int k = 0; k < m_Trace->constantBlocks[i].members[j].members.count(); k++) + { + // Variable in constant buffer array + if(m_Trace->constantBlocks[i].members[j].members[k].name == r.name) + return &m_Trace->constantBlocks[i].members[j].members[k]; + } } } diff --git a/renderdoc/api/replay/pipestate.h b/renderdoc/api/replay/pipestate.h index d3d6ccaa9..28b47e517 100644 --- a/renderdoc/api/replay/pipestate.h +++ b/renderdoc/api/replay/pipestate.h @@ -141,7 +141,10 @@ public: :return: A boolean indicating if binding arrays of resources is supported. :rtype: ``bool`` )"); - bool SupportsResourceArrays() const { return IsCaptureLoaded() && IsCaptureVK(); } + bool SupportsResourceArrays() const + { + return IsCaptureLoaded() && (IsCaptureVK() || IsCaptureD3D12()); + } DOCUMENT(R"(Determines whether or not the current capture uses explicit barriers. :return: A boolean indicating if explicit barriers are used. diff --git a/renderdoc/api/replay/pipestate.inl b/renderdoc/api/replay/pipestate.inl index d685d7e9d..cc311be9a 100644 --- a/renderdoc/api/replay/pipestate.inl +++ b/renderdoc/api/replay/pipestate.inl @@ -928,10 +928,11 @@ BoundCBuffer PipeState::GetConstantBuffer(ShaderStage stage, uint32_t BufIdx, ui if(space == -1) return BoundCBuffer(); - if(space >= s.spaces.count() || bind.bind >= s.spaces[space].constantBuffers.count()) + int32_t shaderReg = bind.bind + (int32_t)ArrayIdx; + if(space >= s.spaces.count() || shaderReg >= s.spaces[space].constantBuffers.count()) return BoundCBuffer(); - const D3D12Pipe::ConstantBuffer &descriptor = s.spaces[space].constantBuffers[bind.bind]; + const D3D12Pipe::ConstantBuffer &descriptor = s.spaces[space].constantBuffers[shaderReg]; buf = descriptor.resourceId; ByteOffset = descriptor.byteOffset; diff --git a/renderdoc/driver/d3d12/d3d12_shaderdebug.cpp b/renderdoc/driver/d3d12/d3d12_shaderdebug.cpp index 81fb82b97..41330bbf5 100644 --- a/renderdoc/driver/d3d12/d3d12_shaderdebug.cpp +++ b/renderdoc/driver/d3d12/d3d12_shaderdebug.cpp @@ -1123,9 +1123,13 @@ void GatherConstantBuffers(WrappedID3D12Device *pDevice, const DXBCBytecode::Pro ID3D12Resource *pCbvResource = pDevice->GetResourceManager()->GetCurrentAs(resId); cbufData.clear(); - pDevice->GetDebugManager()->GetBufferData(pCbvResource, element.offset, 0, cbufData); + + pDevice->GetDebugManager()->GetBufferData(pCbvResource, element.offset + byteOffset, + 0, cbufData); AddCBufferToGlobalState(program, global, sourceVars, refl, mapping, slot, cbufData); } + + desc++; } } } diff --git a/renderdoc/driver/shaders/dxbc/dxbc_debug.cpp b/renderdoc/driver/shaders/dxbc/dxbc_debug.cpp index 1fa41ba99..af64bebd9 100644 --- a/renderdoc/driver/shaders/dxbc/dxbc_debug.cpp +++ b/renderdoc/driver/shaders/dxbc/dxbc_debug.cpp @@ -1452,13 +1452,17 @@ ShaderVariable ThreadState::GetSrc(const Operand &oper, const Operation &op, boo // For this operand, index 0 is always the logical identifier that we can use to // lookup the correct cbuffer. The location of the access entry differs for SM5.1, // as index 1 stores the shader register. + bool isCBArray = false; uint32_t cbIdentifier = indices[0]; + uint32_t cbArrayIndex = ~0U; uint32_t cbLookup = program->IsShaderModel51() ? indices[2] : indices[1]; for(size_t i = 0; i < reflection->CBuffers.size(); i++) { if(reflection->CBuffers[i].identifier == cbIdentifier) { cb = (int)i; + cbArrayIndex = indices[1] - reflection->CBuffers[i].reg; + isCBArray = reflection->CBuffers[i].bindCount > 1; break; } } @@ -1468,14 +1472,27 @@ ShaderVariable ThreadState::GetSrc(const Operand &oper, const Operation &op, boo if(cb >= 0 && cb < global.constantBlocks.count()) { - RDCASSERTMSG("Out of bounds cbuffer lookup", - cbLookup < (uint32_t)global.constantBlocks[cb].members.count(), cbLookup, - global.constantBlocks[cb].members.count()); + RDCASSERTMSG("Invalid cbuffer array index", + !isCBArray || cbArrayIndex < (uint32_t)global.constantBlocks[cb].members.count(), + isCBArray, cb, global.constantBlocks[cb].members.count()); - if(cbLookup < (uint32_t)global.constantBlocks[cb].members.count()) - v = s = global.constantBlocks[cb].members[cbLookup]; + if(!isCBArray || cbArrayIndex < (uint32_t)global.constantBlocks[cb].members.count()) + { + const rdcarray &targetVars = + isCBArray ? global.constantBlocks[cb].members[cbArrayIndex].members + : global.constantBlocks[cb].members; + RDCASSERTMSG("Out of bounds cbuffer lookup", cbLookup < (uint32_t)targetVars.count(), + cbLookup, targetVars.count()); + + if(cbLookup < (uint32_t)targetVars.count()) + v = s = targetVars[cbLookup]; + else + v = s = ShaderVariable("", 0U, 0U, 0U, 0U); + } else + { v = s = ShaderVariable("", 0U, 0U, 0U, 0U); + } } else { @@ -4185,10 +4202,15 @@ void AddCBufferToGlobalState(const DXBCBytecode::Program &program, GlobalState & size_t numCBs = mapping.constantBlocks.size(); for(size_t i = 0; i < numCBs; ++i) { - if((uint32_t)mapping.constantBlocks[i].bindset == slot.registerSpace && - (uint32_t)mapping.constantBlocks[i].bind == slot.shaderRegister) + const Bindpoint &bp = mapping.constantBlocks[i]; + if(slot.registerSpace == (uint32_t)bp.bindset && slot.shaderRegister >= (uint32_t)bp.bind && + slot.shaderRegister < (uint32_t)(bp.bind + bp.arraySize)) { - RDCASSERTMSG("Reassigning previously filled cbuffer", global.constantBlocks[i].members.empty()); + uint32_t arrayIndex = slot.shaderRegister - bp.bind; + rdcarray &targetVars = + bp.arraySize > 1 ? global.constantBlocks[i].members[arrayIndex].members + : global.constantBlocks[i].members; + RDCASSERTMSG("Reassigning previously filled cbuffer", targetVars.empty()); uint32_t cbufferIndex = program.IsShaderModel51() ? (uint32_t)i : slot.shaderRegister; @@ -4201,16 +4223,36 @@ void AddCBufferToGlobalState(const DXBCBytecode::Program &program, GlobalState & DebugVariableReference(DebugVariableType::Constant, global.constantBlocks[i].name)); sourceVars.push_back(cbSourceMapping); + rdcstr identifierPrefix = global.constantBlocks[i].name; + rdcstr variablePrefix = refl.constantBlocks[i].name; + if(bp.arraySize > 1) + { + identifierPrefix = + StringFormat::Fmt("%s[%u]", global.constantBlocks[i].name.c_str(), arrayIndex); + variablePrefix = StringFormat::Fmt("%s[%u]", refl.constantBlocks[i].name.c_str(), arrayIndex); + + // The above sourceVar is for the logical identifier, and FlattenVariables adds the + // individual elements of the constant buffer. For CB arrays, add an extra source + // var for the CB array index + SourceVariableMapping cbArrayMapping; + global.constantBlocks[i].members[arrayIndex].name = identifierPrefix; + cbArrayMapping.name = variablePrefix; + cbArrayMapping.variables.push_back( + DebugVariableReference(DebugVariableType::Constant, identifierPrefix)); + sourceVars.push_back(cbArrayMapping); + } + const rdcarray &constants = + (bp.arraySize > 1) ? refl.constantBlocks[i].variables[0].type.members + : refl.constantBlocks[i].variables; + rdcarray vars; - StandardFillCBufferVariables(refl.resourceId, refl.constantBlocks[i].variables, vars, cbufData); - FlattenVariables(global.constantBlocks[i].name, refl.constantBlocks[i].variables, vars, - global.constantBlocks[i].members, refl.constantBlocks[i].name + ".", 0, + StandardFillCBufferVariables(refl.resourceId, constants, vars, cbufData); + FlattenVariables(identifierPrefix, constants, vars, targetVars, variablePrefix + ".", 0, sourceVars); - for(size_t c = 0; c < global.constantBlocks[i].members.size(); c++) + for(size_t c = 0; c < targetVars.size(); c++) { - global.constantBlocks[i].members[c].name = - StringFormat::Fmt("%s[%u]", global.constantBlocks[i].name.c_str(), (uint32_t)c); + targetVars[c].name = StringFormat::Fmt("%s[%u]", identifierPrefix.c_str(), (uint32_t)c); } return; @@ -5003,7 +5045,17 @@ ShaderDebugTrace *InterpretDebugger::BeginDebug(const DXBC::DXBCContainer *dxbcC } // Set the number of constant buffers in the trace, but assignment happens later - global.constantBlocks.resize(refl.constantBlocks.size()); + size_t numCBuffers = dxbc->GetReflection()->CBuffers.size(); + global.constantBlocks.resize(numCBuffers); + for(size_t i = 0; i < numCBuffers; ++i) + { + uint32_t bindCount = dxbc->GetReflection()->CBuffers[i].bindCount; + if(bindCount > 1) + { + // With a CB array, we use a nesting structure for the trace + global.constantBlocks[i].members.resize(bindCount); + } + } struct ResList { diff --git a/renderdoc/driver/shaders/dxbc/dxbc_reflect.cpp b/renderdoc/driver/shaders/dxbc/dxbc_reflect.cpp index b81ef0a29..abc53306f 100644 --- a/renderdoc/driver/shaders/dxbc/dxbc_reflect.cpp +++ b/renderdoc/driver/shaders/dxbc/dxbc_reflect.cpp @@ -269,7 +269,7 @@ void MakeShaderReflection(DXBC::DXBCContainer *dxbc, ShaderReflection *refl, cb.bindPoint = (int32_t)i; Bindpoint map; - map.arraySize = 1; + map.arraySize = dxbc->GetReflection()->CBuffers[i].bindCount; map.bindset = dxbc->GetReflection()->CBuffers[i].space; map.bind = dxbc->GetReflection()->CBuffers[i].reg; map.used = true;