diff --git a/renderdoc/driver/shaders/spirv/spirv_debug.cpp b/renderdoc/driver/shaders/spirv/spirv_debug.cpp index 68c76328f..47c7f861a 100644 --- a/renderdoc/driver/shaders/spirv/spirv_debug.cpp +++ b/renderdoc/driver/shaders/spirv/spirv_debug.cpp @@ -97,6 +97,21 @@ void ThreadState::EnterFunction(ShaderDebugState *state, const rdcarray &arg OpFunction func(it); StackFrame *frame = new StackFrame(); frame->function = func.result; + + // if there's a previous stack frame, save its live list + if(!callstack.empty()) + { + callstack.back()->live = live; + callstack.back()->sourceVars = sourceVars; + } + + // start with just globals + live = debugger.GetLiveGlobals(); + sourceVars = debugger.GetGlobalSourceVars(); + // process the outgoing scope + if(state) + ProcessScopeChange(*state, frame->live, live); + callstack.push_back(frame); it++; @@ -156,9 +171,6 @@ void ThreadState::EnterFunction(ShaderDebugState *state, const rdcarray &arg if(decl.HasInitializer()) AssignValue(stackvar, ids[decl.initializer]); - // TODO we need to handle re-entry into functions (ID lifetime) - live.removeOne(decl.result); - SetDst(state, decl.result, debugger.MakePointerVariable(decl.result, &stackvar)); it++; @@ -176,102 +188,47 @@ const ShaderVariable &ThreadState::GetSrc(Id id) void ThreadState::SetDst(ShaderDebugState *state, Id id, const ShaderVariable &val) { - // if we don't have a state to track, we can take a much faster pa We just update the value and - // return. We don't have to propagate ID changes between aliased pointers because *internally* we - // always look up pointers and don't care about aliasing. That's only needed for *external* facing - // things because we have to update the changes for all copied pointer values. - if(!state) - { - if(ids[id].name.empty()) - { - // for uninitialised values, init by copying - ids[id] = val; - ids[id].name = debugger.GetRawName(id); - live.push_back(id); - - debugger.AddSourceVars(id); - } - else - { - RDCASSERT(ids[id].isPointer); - // otherwise just update the pointed-to value (only pointers should be existing before being - // assigned) - debugger.WriteThroughPointer(ids[id], val); - } - - return; - } - - // otherwise when we're tracking changes, take a slower pa - - if(ContainsNaNInf(val)) + if(state && ContainsNaNInf(val)) state->flags |= ShaderEvents::GeneratedNanOrInf; - ShaderVariable &var = ids[id]; + ids[id] = val; + ids[id].name = debugger.GetRawName(id); - if(!var.name.empty()) + auto it = std::lower_bound(live.begin(), live.end(), id); + live.insert(it - live.begin(), id); + + if(state) { - // if this ID was already initialised, we must have a change of a pointer. Otherwise we're SSA - // form so no other ID should change. - - RDCASSERT(var.isPointer); - - // if var is a pointer we update the underlying storage and generate at least one change, plus - // any additional ones for other pointers. - Id ptrid = debugger.GetPointerBaseId(var); - - rdcarray changes; - ShaderVariableChange basechange; - basechange.before = debugger.EvaluatePointerVariable(ids[ptrid]); - - rdcarray &pointers = pointersForId[ptrid]; - - changes.resize(pointers.size()); - - // for every other pointer, evaluate its value now before - for(size_t i = 0; i < pointers.size(); i++) - changes[i].before = debugger.EvaluatePointerVariable(ids[pointers[i]]); - - debugger.WriteThroughPointer(var, val); - - // now evaluate the value after - for(size_t i = 0; i < pointers.size(); i++) - changes[i].after = debugger.EvaluatePointerVariable(ids[pointers[i]]); - - // if the pointer we're writing is one of the aliased pointers, be sure we add it even if it's a - // no-op change - int ptrIdx = pointers.indexOf(id); - - if(ptrIdx >= 0) - { - state->changes.push_back(changes[ptrIdx]); - changes.erase(ptrIdx); - } - - // remove any no-op changes. Some pointers might point to the same ID but a child that wasn't - // written to. Note that this might not actually mean nothing was changed (if e.g. we're - // assigning the same value) but that false negative is not a concern. - changes.removeIf([](const ShaderVariableChange &c) { return c.before == c.after; }); - - state->changes.append(changes); - - // always add a change for the base storage variable written itself, even if that's a no-op. - // This one is not included in any of the pointers lists above - basechange.after = debugger.EvaluatePointerVariable(ids[ptrid]); - state->changes.push_back(basechange); - } - else - { - // otherwise it's a new SSA variable, record a change-from-nothing - ids[id] = val; - ids[id].name = debugger.GetRawName(id); - live.push_back(id); - ShaderVariableChange change; change.after = debugger.EvaluatePointerVariable(ids[id]); state->changes.push_back(change); - debugger.AddSourceVars(id); + debugger.AddSourceVars(sourceVars, id); + } +} + +void ThreadState::ProcessScopeChange(ShaderDebugState &state, const rdcarray &oldLive, + const rdcarray &newLive) +{ + // all oldLive (except globals) are going out of scope. all newLive (except globals) are coming + // into scope + + const rdcarray &liveGlobals = debugger.GetLiveGlobals(); + + for(const Id id : oldLive) + { + if(liveGlobals.contains(id)) + continue; + + state.changes.push_back({debugger.EvaluatePointerVariable(ids[id])}); + } + + for(const Id id : newLive) + { + if(liveGlobals.contains(id)) + continue; + + state.changes.push_back({ShaderVariable(), debugger.EvaluatePointerVariable(ids[id])}); } } @@ -315,7 +272,67 @@ void ThreadState::StepNext(ShaderDebugState *state, RDCASSERT(ids[store.pointer].isPointer); - SetDst(state, store.pointer, GetSrc(store.object)); + // this is the only place we don't use SetDst because it's the only place that "violates" SSA + // i.e. changes an existing value. That way SetDst can always unconditionally assign values, + // and only here do we write through pointers + + ShaderVariable val = GetSrc(store.object); + + if(!state) + { + debugger.WriteThroughPointer(ids[store.pointer], val); + } + else + { + ShaderVariable &var = ids[store.pointer]; + + if(ContainsNaNInf(val)) + state->flags |= ShaderEvents::GeneratedNanOrInf; + + // if var is a pointer we update the underlying storage and generate at least one change, + // plus any additional ones for other pointers. + Id ptrid = debugger.GetPointerBaseId(var); + + rdcarray changes; + ShaderVariableChange basechange; + basechange.before = debugger.EvaluatePointerVariable(ids[ptrid]); + + rdcarray &pointers = pointersForId[ptrid]; + + changes.resize(pointers.size()); + + // for every other pointer, evaluate its value now before + for(size_t i = 0; i < pointers.size(); i++) + changes[i].before = debugger.EvaluatePointerVariable(ids[pointers[i]]); + + debugger.WriteThroughPointer(var, val); + + // now evaluate the value after + for(size_t i = 0; i < pointers.size(); i++) + changes[i].after = debugger.EvaluatePointerVariable(ids[pointers[i]]); + + // if the pointer we're writing is one of the aliased pointers, be sure we add it even if + // it's a no-op change + int ptrIdx = pointers.indexOf(store.pointer); + + if(ptrIdx >= 0) + { + state->changes.push_back(changes[ptrIdx]); + changes.erase(ptrIdx); + } + + // remove any no-op changes. Some pointers might point to the same ID but a child that + // wasn't written to. Note that this might not actually mean nothing was changed (if e.g. + // we're assigning the same value) but that false negative is not a concern. + changes.removeIf([](const ShaderVariableChange &c) { return c.before == c.after; }); + + state->changes.append(changes); + + // always add a change for the base storage variable written itself, even if that's a no-op. + // This one is not included in any of the pointers lists above + basechange.after = debugger.EvaluatePointerVariable(ids[ptrid]); + state->changes.push_back(basechange); + } break; } @@ -483,6 +500,14 @@ void ThreadState::StepNext(ShaderDebugState *state, } nextInstruction = exitingFrame->funcCallInstruction; + + // process the outgoing and incoming scopes + if(state) + ProcessScopeChange(*state, live, callstack.back()->live); + + // restore the live list from the calling frame + live = callstack.back()->live; + sourceVars = callstack.back()->sourceVars; } delete exitingFrame; diff --git a/renderdoc/driver/shaders/spirv/spirv_debug.h b/renderdoc/driver/shaders/spirv/spirv_debug.h index 8e14c4754..d7dfda7d6 100644 --- a/renderdoc/driver/shaders/spirv/spirv_debug.h +++ b/renderdoc/driver/shaders/spirv/spirv_debug.h @@ -60,6 +60,10 @@ struct StackFrame // allocated storage for locals rdcarray locals; + // the thread's live list before the function was entered + rdcarray live; + rdcarray sourceVars; + private: // disallow copying to ensure the locals we allocate never move around StackFrame(const StackFrame &o) = delete; @@ -105,6 +109,8 @@ struct ThreadState // the list of IDs that are currently valid and live rdcarray live; + rdcarray sourceVars; + // index in the pixel quad int workgroupIndex; bool done; @@ -112,6 +118,8 @@ struct ThreadState private: const ShaderVariable &GetSrc(Id id); void SetDst(ShaderDebugState *state, Id id, const ShaderVariable &val); + void ProcessScopeChange(ShaderDebugState &state, const rdcarray &oldLive, + const rdcarray &newLive); }; class Debugger : public Processor, public ShaderDebugger @@ -133,7 +141,7 @@ public: const DataType &GetType(Id typeId); rdcstr GetRawName(Id id) const; rdcstr GetHumanName(Id id); - void AddSourceVars(Id id); + void AddSourceVars(rdcarray &sourceVars, Id id); void AllocateVariable(Id id, Id typeId, DebugVariableType sourceVarType, const rdcstr &sourceName, ShaderVariable &outVar); @@ -146,6 +154,8 @@ public: uint32_t GetNumInstructions() { return (uint32_t)instructionOffsets.size(); } GlobalState GetGlobal() { return global; } + const rdcarray &GetLiveGlobals() { return liveGlobals; } + const rdcarray &GetGlobalSourceVars() { return globalSourceVars; } ThreadState &GetActiveLane() { return workgroup[activeLaneIndex]; } private: virtual void PreParse(uint32_t maxId); @@ -155,8 +165,8 @@ private: void AllocateVariable(const Decorations &varDecorations, const Decorations &curDecorations, DebugVariableType sourceVarType, const rdcstr &sourceName, uint32_t offset, const DataType &inType, ShaderVariable &outVar); - void AddSourceVars(const DataType &inType, const rdcstr &sourceName, const rdcstr &varName, - uint32_t &offset); + void AddSourceVars(rdcarray &sourceVars, const DataType &inType, + const rdcstr &sourceName, const rdcstr &varName, uint32_t &offset); ///////////////////////////////////////////////////////// // debug data @@ -166,8 +176,6 @@ private: GlobalState global; rdcarray workgroup; - rdcarray sourceVars; - uint32_t activeLaneIndex = 0; ShaderStage stage; @@ -187,9 +195,17 @@ private: rdcarray memberNames; std::map entryLookup; + SparseIdMap idDeathOffset; + + // the live mutable global variables, to initialise a stack frame's live list + rdcarray liveGlobals; + rdcarray globalSourceVars; + struct Function { size_t begin = 0; + rdcarray parameters; + rdcarray variables; }; SparseIdMap functions; diff --git a/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp b/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp index bec9068f6..0648d63e4 100644 --- a/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp +++ b/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp @@ -127,7 +127,7 @@ ShaderDebugTrace *Debugger::BeginDebug(DebugAPIWrapper *apiWrapper, const Shader rdcstr sourceName = GetHumanName(v.id); - size_t oldSize = sourceVars.size(); + size_t oldSize = globalSourceVars.size(); const DataType &type = dataTypes[v.type]; @@ -141,8 +141,8 @@ ShaderDebugTrace *Debugger::BeginDebug(DebugAPIWrapper *apiWrapper, const Shader // I/O variable structs don't have offsets, so give them fake offsets to ensure they sort as // we want. Since FillVariable is depth-first the source vars are already in order. - for(size_t i = oldSize; i < sourceVars.size(); i++) - sourceVars[i].offset = uint32_t(i - oldSize); + for(size_t i = oldSize; i < globalSourceVars.size(); i++) + globalSourceVars[i].offset = uint32_t(i - oldSize); if(isInput) { @@ -197,7 +197,7 @@ ShaderDebugTrace *Debugger::BeginDebug(DebugAPIWrapper *apiWrapper, const Shader sourceVar.offset = 0; sourceVar.variables.push_back(DebugVariableReference(DebugVariableType::Constant, var.name)); - sourceVars.push_back(sourceVar); + globalSourceVars.push_back(sourceVar); } } } @@ -210,17 +210,19 @@ ShaderDebugTrace *Debugger::BeginDebug(DebugAPIWrapper *apiWrapper, const Shader for(size_t i = 0; i < global.constantBlocks.size(); i++) active.ids[cbufferIDs[i]] = MakePointerVariable(cbufferIDs[i], &global.constantBlocks[i]); - // only outputs are considered mutable - active.live.append(outputIDs); + std::sort(outputIDs.begin(), outputIDs.end()); - for(size_t i = 0; i < sourceVars.size();) + // only outputs are considered mutable + liveGlobals.append(outputIDs); + + for(size_t i = 0; i < globalSourceVars.size();) { - if(!sourceVars[i].variables.empty() && - (sourceVars[i].variables[0].type == DebugVariableType::Input || - sourceVars[i].variables[0].type == DebugVariableType::Constant)) + if(!globalSourceVars[i].variables.empty() && + (globalSourceVars[i].variables[0].type == DebugVariableType::Input || + globalSourceVars[i].variables[0].type == DebugVariableType::Constant)) { - ret->sourceVars.push_back(sourceVars[i]); - sourceVars.erase(i); + ret->sourceVars.push_back(globalSourceVars[i]); + globalSourceVars.erase(i); continue; } i++; @@ -262,7 +264,7 @@ rdcarray Debugger::ContinueDebug() for(const Id &v : active.live) initial.changes.push_back({ShaderVariable(), EvaluatePointerVariable(active.ids[v])}); - initial.sourceVars = sourceVars; + initial.sourceVars = active.sourceVars; initial.stepIndex = steps; @@ -299,30 +301,56 @@ rdcarray Debugger::ContinueDebug() CalcActiveMask(activeMask); // step all active members of the workgroup - for(size_t i = 0; i < workgroup.size(); i++) + for(size_t lane = 0; lane < workgroup.size(); lane++) { - if(activeMask[i]) + ThreadState &thread = workgroup[lane]; + + if(activeMask[lane]) { - if(workgroup[i].nextInstruction >= instructionOffsets.size()) + if(thread.nextInstruction >= instructionOffsets.size()) { - if(i == activeLaneIndex) + if(lane == activeLaneIndex) ret.push_back(ShaderDebugState()); continue; } - if(i == activeLaneIndex) + if(lane == activeLaneIndex) { ShaderDebugState state; - workgroup[i].StepNext(&state, oldworkgroup); + + // see if we're retiring any IDs at this state + for(size_t l = 0; l < thread.live.size();) + { + Id id = thread.live[l]; + if(idDeathOffset[id] < instructionOffsets[thread.nextInstruction]) + { + thread.live.erase(l); + ShaderVariableChange change; + change.before = EvaluatePointerVariable(thread.ids[id]); + state.changes.push_back(change); + + rdcstr name = GetRawName(id); + + thread.sourceVars.removeIf([name](const SourceVariableMapping &var) { + return var.variables[0].name.beginsWith(name); + }); + + continue; + } + + l++; + } + + thread.StepNext(&state, oldworkgroup); state.stepIndex = steps; - state.sourceVars = sourceVars; - workgroup[i].FillCallstack(state); + state.sourceVars = thread.sourceVars; + thread.FillCallstack(state); ret.push_back(state); } else { - workgroup[i].StepNext(NULL, oldworkgroup); + thread.StepNext(NULL, oldworkgroup); } } } @@ -552,7 +580,7 @@ rdcstr Debugger::GetHumanName(Id id) return name; } -void Debugger::AddSourceVars(Id id) +void Debugger::AddSourceVars(rdcarray &sourceVars, Id id) { rdcstr name; @@ -567,12 +595,12 @@ void Debugger::AddSourceVars(Id id) Id type = idTypes[id]; uint32_t offset = 0; - AddSourceVars(dataTypes[type], name, GetRawName(id), offset); + AddSourceVars(sourceVars, dataTypes[type], name, GetRawName(id), offset); } } -void Debugger::AddSourceVars(const DataType &inType, const rdcstr &sourceName, - const rdcstr &varName, uint32_t &offset) +void Debugger::AddSourceVars(rdcarray &sourceVars, const DataType &inType, + const rdcstr &sourceName, const rdcstr &varName, uint32_t &offset) { SourceVariableMapping sourceVar; @@ -585,7 +613,7 @@ void Debugger::AddSourceVars(const DataType &inType, const rdcstr &sourceName, case DataType::PointerType: { // step silently into pointers - AddSourceVars(dataTypes[inType.InnerType()], sourceName, varName, offset); + AddSourceVars(sourceVars, dataTypes[inType.InnerType()], sourceName, varName, offset); return; } case DataType::ScalarType: @@ -621,7 +649,8 @@ void Debugger::AddSourceVars(const DataType &inType, const rdcstr &sourceName, else childSourceName = sourceName + "." + inType.children[i].name; - AddSourceVars(dataTypes[inType.children[i].type], childSourceName, childVarName, offset); + AddSourceVars(sourceVars, dataTypes[inType.children[i].type], childSourceName, childVarName, + offset); } return; } @@ -632,7 +661,8 @@ void Debugger::AddSourceVars(const DataType &inType, const rdcstr &sourceName, { rdcstr idx = StringFormat::Fmt("[%u]", i); - AddSourceVars(dataTypes[inType.InnerType()], sourceName + idx, varName + idx, offset); + AddSourceVars(sourceVars, dataTypes[inType.InnerType()], sourceName + idx, varName + idx, + offset); } return; } @@ -783,7 +813,7 @@ void Debugger::AllocateVariable(const Decorations &varDecorations, const Decorat if(curDecorations.flags & Decorations::HasBuiltIn) sourceVar.builtin = MakeShaderBuiltin(stage, curDecorations.builtIn); - sourceVars.push_back(sourceVar); + globalSourceVars.push_back(sourceVar); if(sourceVarType == DebugVariableType::Input) { @@ -862,6 +892,10 @@ void Debugger::PostParse() for(const MemberName &mem : memberNames) dataTypes[mem.id].children[mem.member].name = mem.name; + // global IDs never hit a death point + for(const Variable &v : globals) + idDeathOffset[v.id] = ~0U; + memberNames.clear(); } @@ -871,6 +905,13 @@ void Debugger::RegisterOp(Iter it) OpDecoder opdata(it); + // we add +1 so that we don't remove the ID on its last use, but the next subsequent instruction + // since blocks always end with a terminator that doesn't consume IDs we're interested in + // (variables) we'll always have one extra instruction to step to + OpDecoder::ForEachID(it, [this, &it](Id id, bool result) { + idDeathOffset[id] = RDCMAX(it.offs() + 1, idDeathOffset[id]); + }); + if(opdata.op == Op::Line || opdata.op == Op::NoLine) { // ignore OpLine/OpNoLine @@ -909,6 +950,19 @@ void Debugger::RegisterOp(Iter it) curFunction->begin = it.offs(); } + else if(opdata.op == Op::FunctionParameter) + { + OpFunctionParameter param(it); + + curFunction->parameters.push_back(param.result); + } + else if(opdata.op == Op::Variable) + { + OpVariable var(it); + + if(var.storageClass == StorageClass::Function && curFunction) + curFunction->variables.push_back(var.result); + } // everything else inside a function becomes an instruction, including the OpFunction and // OpFunctionEnd. We won't actually execute these instructions @@ -917,6 +971,13 @@ void Debugger::RegisterOp(Iter it) if(opdata.op == Op::FunctionEnd) { + // don't automatically kill function parameters and variables. They will be manually killed when + // returning from a function's scope + for(const Id id : curFunction->parameters) + idDeathOffset[id] = ~0U; + for(const Id id : curFunction->variables) + idDeathOffset[id] = ~0U; + curFunction = NULL; } }