From 80e1cbfba4d6cdee22a4c8b9421fd061af31a7ef Mon Sep 17 00:00:00 2001 From: baldurk Date: Tue, 14 Apr 2020 15:11:41 +0100 Subject: [PATCH] Don't duplicate workgroup for derivatives * Because SPIR-V is SSA, we can use the current workgroup for all cross-group operations, knowing that the output ID of any such opcode can't also be the input (so doing workgroup operations one-at-a-time is still fine, we won't corrupt results of later threads by updating earlier ones. --- .../driver/shaders/spirv/spirv_debug.cpp | 51 +++++++++---------- renderdoc/driver/shaders/spirv/spirv_debug.h | 4 +- .../shaders/spirv/spirv_debug_setup.cpp | 14 +---- 3 files changed, 29 insertions(+), 40 deletions(-) diff --git a/renderdoc/driver/shaders/spirv/spirv_debug.cpp b/renderdoc/driver/shaders/spirv/spirv_debug.cpp index caf3d4b50..2cc6e6c1a 100644 --- a/renderdoc/driver/shaders/spirv/spirv_debug.cpp +++ b/renderdoc/driver/shaders/spirv/spirv_debug.cpp @@ -182,7 +182,7 @@ void ThreadState::EnterFunction(ShaderDebugState *state, const rdcarray &arg nextInstruction = debugger.GetInstructionForIter(it); } -const ShaderVariable &ThreadState::GetSrc(Id id) +const ShaderVariable &ThreadState::GetSrc(Id id) const { return ids[id]; } @@ -252,8 +252,7 @@ void ThreadState::JumpToLabel(Id target) } } -void ThreadState::StepNext(ShaderDebugState *state, - const rdcarray> &prevWorkgroup) +void ThreadState::StepNext(ShaderDebugState *state, const rdcarray &workgroup) { Iter it = debugger.GetIterForInstruction(nextInstruction); nextInstruction++; @@ -404,8 +403,8 @@ void ThreadState::StepNext(ShaderDebugState *state, OpDPdx deriv(it); // coarse derivatives are identical across the quad, based on the top-left. - ShaderVariable var = prevWorkgroup[0][deriv.p]; - ShaderVariable other = prevWorkgroup[1][deriv.p]; + ShaderVariable var = workgroup[0].GetSrc(deriv.p); + ShaderVariable other = workgroup[1].GetSrc(deriv.p); for(uint8_t c = 0; c < var.columns; c++) var.value.fv[c] = other.value.fv[c] - var.value.fv[c]; @@ -421,8 +420,8 @@ void ThreadState::StepNext(ShaderDebugState *state, OpDPdx deriv(it); // coarse derivatives are identical across the quad, based on the top-left. - ShaderVariable var = prevWorkgroup[0][deriv.p]; - ShaderVariable other = prevWorkgroup[2][deriv.p]; + ShaderVariable var = workgroup[0].GetSrc(deriv.p); + ShaderVariable other = workgroup[2].GetSrc(deriv.p); for(uint8_t c = 0; c < var.columns; c++) var.value.fv[c] = other.value.fv[c] - var.value.fv[c]; @@ -452,13 +451,13 @@ void ThreadState::StepNext(ShaderDebugState *state, // top-left if(xdirection) { - a = prevWorkgroup[0][deriv.p]; - b = prevWorkgroup[1][deriv.p]; + a = workgroup[0].GetSrc(deriv.p); + b = workgroup[1].GetSrc(deriv.p); } else { - a = prevWorkgroup[0][deriv.p]; - b = prevWorkgroup[2][deriv.p]; + a = workgroup[0].GetSrc(deriv.p); + b = workgroup[2].GetSrc(deriv.p); } } else @@ -466,13 +465,13 @@ void ThreadState::StepNext(ShaderDebugState *state, // bottom-left if(xdirection) { - a = prevWorkgroup[2][deriv.p]; - b = prevWorkgroup[3][deriv.p]; + a = workgroup[2].GetSrc(deriv.p); + b = workgroup[3].GetSrc(deriv.p); } else { - a = prevWorkgroup[0][deriv.p]; - b = prevWorkgroup[2][deriv.p]; + a = workgroup[0].GetSrc(deriv.p); + b = workgroup[2].GetSrc(deriv.p); } } } @@ -483,13 +482,13 @@ void ThreadState::StepNext(ShaderDebugState *state, // top-right if(xdirection) { - a = prevWorkgroup[0][deriv.p]; - b = prevWorkgroup[1][deriv.p]; + a = workgroup[0].GetSrc(deriv.p); + b = workgroup[1].GetSrc(deriv.p); } else { - a = prevWorkgroup[1][deriv.p]; - b = prevWorkgroup[3][deriv.p]; + a = workgroup[1].GetSrc(deriv.p); + b = workgroup[3].GetSrc(deriv.p); } } else @@ -497,13 +496,13 @@ void ThreadState::StepNext(ShaderDebugState *state, // bottom-right if(xdirection) { - a = prevWorkgroup[2][deriv.p]; - b = prevWorkgroup[3][deriv.p]; + a = workgroup[2].GetSrc(deriv.p); + b = workgroup[3].GetSrc(deriv.p); } else { - a = prevWorkgroup[1][deriv.p]; - b = prevWorkgroup[3][deriv.p]; + a = workgroup[1].GetSrc(deriv.p); + b = workgroup[3].GetSrc(deriv.p); } } } @@ -1577,9 +1576,9 @@ void ThreadState::StepNext(ShaderDebugState *state, if(derivId != Id()) { // calculate DDX/DDY in coarse fashion - ShaderVariable topleft = prevWorkgroup[0][derivId]; - ShaderVariable topright = prevWorkgroup[1][derivId]; - ShaderVariable bottomleft = prevWorkgroup[2][derivId]; + ShaderVariable topleft = workgroup[0].GetSrc(derivId); + ShaderVariable topright = workgroup[1].GetSrc(derivId); + ShaderVariable bottomleft = workgroup[2].GetSrc(derivId); ddxCalc = ddyCalc = topleft; diff --git a/renderdoc/driver/shaders/spirv/spirv_debug.h b/renderdoc/driver/shaders/spirv/spirv_debug.h index 92df5e10e..3b557237e 100644 --- a/renderdoc/driver/shaders/spirv/spirv_debug.h +++ b/renderdoc/driver/shaders/spirv/spirv_debug.h @@ -144,7 +144,7 @@ struct ThreadState ~ThreadState(); void EnterFunction(ShaderDebugState *state, const rdcarray &arguments); - void StepNext(ShaderDebugState *state, const rdcarray> &prevWorkgroup); + void StepNext(ShaderDebugState *state, const rdcarray &workgroup); void FillCallstack(ShaderDebugState &state); @@ -183,7 +183,7 @@ struct ThreadState uint32_t workgroupIndex; bool done; - const ShaderVariable &GetSrc(Id id); + const ShaderVariable &GetSrc(Id id) const; private: void SetDst(ShaderDebugState *state, Id id, const ShaderVariable &val); diff --git a/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp b/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp index 8d49fc534..fe50b186c 100644 --- a/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp +++ b/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp @@ -590,10 +590,6 @@ rdcarray Debugger::ContinueDebug() if(active.Finished()) return ret; - rdcarray> oldworkgroup; - - oldworkgroup.resize(workgroup.size()); - rdcarray activeMask; // do 100 in a chunk @@ -602,12 +598,6 @@ rdcarray Debugger::ContinueDebug() if(active.Finished()) break; - // set up the old workgroup so that cross-workgroup/cross-quad operations (e.g. DDX/DDY) get - // consistent results even when we step the quad out of order. Otherwise if an operation reads - // and writes from the same register we'd trash data needed for other workgroup elements. - for(size_t i = 0; i < oldworkgroup.size(); i++) - oldworkgroup[i] = workgroup[i].ids; - // calculate the current mask of which threads are active CalcActiveMask(activeMask); @@ -653,7 +643,7 @@ rdcarray Debugger::ContinueDebug() l++; } - thread.StepNext(&state, oldworkgroup); + thread.StepNext(&state, workgroup); state.stepIndex = steps; state.sourceVars = thread.sourceVars; thread.FillCallstack(state); @@ -661,7 +651,7 @@ rdcarray Debugger::ContinueDebug() } else { - thread.StepNext(NULL, oldworkgroup); + thread.StepNext(NULL, workgroup); } } }