From f5ba4e9c68dabe3be38fea19d484b02e7b9de9c6 Mon Sep 17 00:00:00 2001 From: Chad Date: Thu, 8 Oct 2026 14:02:49 -0500 Subject: [PATCH] fix(recompiler): throw for descriptor web leaves that are not SRT descriptors A path whose T# or S# dwords hold a program-counter, EXEC or compare-mask value is no longer folded onto the first descriptor: the web split gives up and the read throws as on main. Cache the invariant-Phi resolution per web, lower the state limit to 512 and test the program-counter, EXEC and compare-mask rows, unwritten registers, and the arm and state limits. Co-Authored-By: Claude Opus 5.5 --- .../libSceAgcDriver/tests/ShaderMemory.cpp | 66 +++++++++++++++---- .../Optimization/src/ResourceTracker.cpp | 42 +++++------- docs/dev/TechnicalDebt.md | 1 - 3 files changed, 72 insertions(+), 37 deletions(-) diff --git a/core/libs/prx/libSceAgcDriver/tests/ShaderMemory.cpp b/core/libs/prx/libSceAgcDriver/tests/ShaderMemory.cpp index 2c0d5ac08a..d3e266be78 100644 --- a/core/libs/prx/libSceAgcDriver/tests/ShaderMemory.cpp +++ b/core/libs/prx/libSceAgcDriver/tests/ShaderMemory.cpp @@ -511,18 +511,62 @@ void verifyDescriptorPhis() { expectFailure([&] { static_cast(GetResourcePlan(dynamic)); }, "GetSamplerResource dword 0 is not a valid runtime value", "descriptor Phi: an edge without an SRT slot was accepted"); const std::vector loopCode{0xf4080500u, 0xfa000020u, 0xf4080600u, 0xfa000040u, 0xf40c0200u, 0xfa000000u, 0xbe910380u, 0xbf8cc07fu, 0xf09c0f08u, 0x00a20000u, 0xbf8c3f70u, 0xe0700000u, 0x80060000u, 0xbf068011u, 0xbf850003u, 0xf40c0200u, 0xfa000000u, 0xbf820002u, 0xf40c0200u, 0xfa000060u, 0xbf8cc07fu, 0x80118111u, 0xbf0a8211u, 0xbf85fff0u, 0xbf810000u}; - auto programCounterCode = loopCode; - programCounterCode.insert(programCounterCode.begin() + 8, 0xbe8e1f00u); - for (const auto* code : std::array*, 2>{&loopCode, &programCounterCode}) { - const auto loopCapture = compile(*code, 2u, 1u); - const auto& loopImages = loopCapture->snapshot.images; - const auto holdsLoopImage = [&](const std::array& words) { - return std::ranges::any_of(loopImages, [&](const DescriptorValue& value) { - return std::equal(words.begin(), words.end(), value.dwords.begin()); - }); - }; - require(loopImages.size() == 2u && holdsLoopImage(first) && holdsLoopImage(second), "descriptor Phi: the loop's chained T# Phis were not split into the two SRT T#s"); + const auto loopCapture = compile(loopCode, 2u, 1u); + const auto& loopImages = loopCapture->snapshot.images; + const auto holdsLoopImage = [&](const std::array& words) { + return std::ranges::any_of(loopImages, [&](const DescriptorValue& value) { + return std::equal(words.begin(), words.end(), value.dwords.begin()); + }); + }; + require(loopImages.size() == 2u && holdsLoopImage(first) && holdsLoopImage(second), "descriptor Phi: the loop's chained T# Phis were not split into the two SRT T#s"); + + const auto entryWrites = [&](std::initializer_list words) { + auto code = loopCode; + code.insert(code.begin() + 8, words); + return code; + }; + const std::array, const char*>, 3> invalidEntries{{ + {entryWrites({0xbe8e1f00u}), "descriptor Phi: a T# holding the program counter was accepted"}, + {entryWrites({0xbe8e037eu}), "descriptor Phi: a T# holding EXEC was accepted"}, + {entryWrites({0x7d840080u, 0xbe8e036au}), "descriptor Phi: a T# holding a compare mask was accepted"}, + }}; + for (const auto& [code, message] : invalidEntries) { + auto request = makeRequest(code); + expectFailure([&] { static_cast(GetResourcePlan(request)); }, "GetImageResource dword 0 is not a valid runtime value", message); } + auto unwritten = loopCode; + unwritten[4] = 0xf4080200u; + auto unwrittenRequest = makeRequest(unwritten); + require(GetResourcePlan(unwrittenRequest)->info.images.size() == 3u, "descriptor Phi: a T# whose unwritten registers read as 0 was not planned as its own image"); + + const std::array prologue{0xf4080500u, 0xfa000020u, 0xf4080600u, 0xfa000040u, 0xf40c0200u, 0xfa000000u, 0xbf8cc07fu}; + const std::array epilogue{0xbf8cc07fu, 0xf09c0f08u, 0x00a20000u, 0xbf8c3f70u, 0xe0700000u, 0x80060000u, 0xbf810000u}; + const auto planImages = [&](const std::vector& body) { + std::vector code(prologue.begin(), prologue.end()); + code.insert(code.end(), body.begin(), body.end()); + code.insert(code.end(), epilogue.begin(), epilogue.end()); + auto request = makeRequest(code); + return GetResourcePlan(request)->info.images.size(); + }; + const auto distinctLoads = [](std::uint32_t loads) { + std::vector body; + for (std::uint32_t load = 0; load < loads; load++) { + body.insert(body.end(), {0xbf068014u, 0xbf850002u, 0xf40c0200u, 0xfa000060u + load * 0x20u}); + } + return body; + }; + require(planImages(distinctLoads(63u)) == 64u, "descriptor Phi: 64 distinct T#s were not given one image each"); + expectFailure([&] { static_cast(planImages(distinctLoads(64u))); }, "GetImageResource dword 0 is not a valid runtime value", "descriptor Phi: more than 64 distinct T#s were accepted"); + + const auto splitReloads = [](std::uint32_t joins) { + std::vector body; + for (std::uint32_t join = 0; join < joins; join++) { + body.insert(body.end(), {0xbf068014u, 0xbf850003u, 0xf40003c0u, join % 2u == 0u ? 0xfa000088u : 0xfa00008cu, 0xbf820006u, 0xf4080200u, 0xfa000060u, 0xf4040300u, 0xfa000070u, 0xf4000380u, 0xfa000078u}); + } + return body; + }; + require(planImages(splitReloads(8u)) == 4u, "descriptor Phi: separate reloads of T# dword 7 and dwords 0-6 were not split into their 4 T#s"); + expectFailure([&] { static_cast(planImages(splitReloads(260u))); }, "GetImageResource dword 0 is not a valid runtime value", "descriptor Phi: a web over more than 512 descriptor tuples was accepted"); } void verifyProgramCounterRelativeData() { diff --git a/core/shader/recompiler/Optimization/src/ResourceTracker.cpp b/core/shader/recompiler/Optimization/src/ResourceTracker.cpp index 1f407ecc7f..992df862af 100644 --- a/core/shader/recompiler/Optimization/src/ResourceTracker.cpp +++ b/core/shader/recompiler/Optimization/src/ResourceTracker.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -741,25 +742,16 @@ private: inst.Parent()->RemoveInstruction(&inst); } - static bool CarriesControlValue(IrValue* value, std::uint32_t depth) { + static bool HoldsProgramCounter(IrValue* value, std::uint32_t depth) { value = value->Resolve(); - if (value->HasImmediate() || value->IsPhi()) { - return false; + if (value->Opcode() == IrOpcode::GetShaderBase) { + return true; } - switch (value->Opcode()) { - case IrOpcode::GetShaderBase: - case IrOpcode::GetExec: - case IrOpcode::GetExecLo: - case IrOpcode::GetExecHi: - case IrOpcode::Ballot: - case IrOpcode::UndefU32: return true; - default: break; - } - if (depth == 0u || (!Detail::IsRuntimeUniformOp(value->Opcode()) && value->Opcode() != IrOpcode::CompositeExtractU32x4)) { + if (value->HasImmediate() || depth == 0u || !Detail::IsRuntimeUniformOp(value->Opcode())) { return false; } for (std::size_t index = 0; index < value->ArgumentCount(); index++) { - if (CarriesControlValue(value->Argument(index), depth - 1u)) { + if (HoldsProgramCounter(value->Argument(index), depth - 1u)) { return true; } } @@ -767,34 +759,34 @@ private: } struct DescriptorWeb { - static constexpr std::size_t maxStates = 4096u; + static constexpr std::size_t maxStates = 512u; static constexpr std::size_t maxArms = 64u; std::vector> arms; std::map, IrValue*> selectors; std::vector> phis; + std::unordered_map resolved; }; IrValue* WebSelector(std::vector tuple, DescriptorWeb& web, IrValue& position) { for (IrValue*& value : tuple) { value = value->Resolve(); - while (value->IsPhi()) { - IrValue* invariant = ResolveInvariantPhi(m_program.Resources(), value); - if (invariant == nullptr || invariant == value) { - break; - } - value = invariant->Resolve(); + if (!value->IsPhi()) { + continue; } + const auto [entry, inserted] = web.resolved.try_emplace(value, value); + if (inserted) { + IrValue* invariant = ResolveInvariantPhi(m_program.Resources(), value); + entry->second = invariant == nullptr ? value : invariant->Resolve(); + } + value = entry->second; } if (const auto known = web.selectors.find(tuple); known != web.selectors.end()) { return known->second; } const auto phi = std::ranges::find_if(tuple, [](const IrValue* value) { return value->IsPhi(); }); if (phi == tuple.end()) { - if (std::ranges::any_of(tuple, [](IrValue* value) { return CarriesControlValue(value, phiSearchDepth); })) { - return &m_builder.Constant(0u); - } for (IrValue* value : tuple) { - if (Rematerialize(value, position, false, phiSearchDepth) == nullptr) { + if (HoldsProgramCounter(value, phiSearchDepth) || Rematerialize(value, position, false, phiSearchDepth) == nullptr) { return nullptr; } } diff --git a/docs/dev/TechnicalDebt.md b/docs/dev/TechnicalDebt.md index 859ee88b06..0844510647 100644 --- a/docs/dev/TechnicalDebt.md +++ b/docs/dev/TechnicalDebt.md @@ -233,7 +233,6 @@ Throughout the project, every function at every stage either **does exactly what - The shader recompiler [ignores `s_setreg_b32`](../../core/shader/recompiler/Translation/src/ScalarInstructions.cpp): writes to MODE (float rounding and denormal controls) and the other hardware registers have no effect, and `s_getreg_b32` reads of the MODE round fields report round to nearest even even after `s_setreg_b32` wrote another mode. The initial round mode from the shader's program registers is not read either. - On a host subgroup narrower than 32 lanes, [`threadBit`](../../core/shader/recompiler/Translation/src/TranslationContext/ExecMask.cpp) reads an EXEC written as a value (`s_mov_b32`/`s_mov_b64`) at bit (subgroup invocation id) of EXEC_LO, so lanes at or above the subgroup size read a lower lane's bit. - [`v_readlane_b32`](../../core/shader/recompiler/SpirvBackend/src/SpirvAlu/SpirvAluEmitterMath.cpp) and fetch-inactive `v_permlane16_b32`/`v_permlanex16_b32` read undefined data from a lane that the host subgroup does not hold or whose host invocation is inactive; the console reads the lane's VGPR. -- [Image reads through a web of descriptor Phis](../../core/shader/recompiler/Optimization/src/ResourceTracker.cpp) are split into one read per distinct SRT descriptor. A path whose T# or S# dwords hold a program-counter, EXEC or ballot value, or an undefined register, is folded onto the first descriptor: no real descriptor holds such a value, so the path is assumed not to reach the read. If a title does read through it, the hardware would fetch from a garbage descriptor, and the read here returns the first descriptor's result instead. - Without fetch-inactive, [`v_permlane16_b32` and `v_permlanex16_b32`](../../core/shader/recompiler/SpirvBackend/src/SpirvAlu/SpirvAluEmitterMath.cpp) take a source lane that the host subgroup does not hold as inactive and keep the old value (0 with bound_ctrl); the console reads that lane's VGPR. - [Shader recompilation](../../core/shader/recompiler/Recompiler.cpp) currently occurs right before it was transferred to Vulkan with caching, but should be moved to the [relinker](../../core/relinker/main.cpp) stage. For this purpose, [shader/recompiler](../../core/shader/recompiler) was written completely independently from [libs/prx](../../core/libs/prx). - [v_fma_f64](../../core/shader/recompiler/SpirvBackend/src/SpirvAlu/SpirvAluEmitterFloat64.cpp) is fused exactly only for normal inputs: with a subnormal input it is a separate multiply and add, which rounds twice.