From af3a3f4bf6630c39a4d2c26b50c7286723e07f8f Mon Sep 17 00:00:00 2001 From: nmzik Date: Wed, 16 Sep 2026 17:45:19 +0400 Subject: [PATCH] Preserve uniform conditional sampler choices in resource plans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Related work: https://github.com/KytyPS5/KytyPS5/pull/558 Co-authored-by: Çağatay Güçlü <33038942+defektu@users.noreply.github.com> --- src/graphics/shader/recompiler/ir/Program.cpp | 20 +++ src/graphics/shader/recompiler/ir/ShaderIR.h | 1 + .../ir/passes/ResourceMaterialization.cpp | 9 +- .../recompiler/ir/passes/ResourceTracking.cpp | 75 +++++++++-- .../shader/recompiler/ir/passes/SrtWalker.cpp | 34 +++-- tests/ResourceTrackingTests.cpp | 120 ++++++++++++++++++ 6 files changed, 230 insertions(+), 29 deletions(-) diff --git a/src/graphics/shader/recompiler/ir/Program.cpp b/src/graphics/shader/recompiler/ir/Program.cpp index 9d834e137..93e94da33 100644 --- a/src/graphics/shader/recompiler/ir/Program.cpp +++ b/src/graphics/shader/recompiler/ir/Program.cpp @@ -112,6 +112,10 @@ ResourcePlan& ResourcePlan::operator=(ResourcePlan&& other) noexcept { } Program::~Program() { + // Planning expressions can refer to block values but outlive block storage in the base class. + for (auto& inst: value_storage) { + inst.Invalidate(); + } // Values may cross block boundaries. Detach all arguments before any block starts destroying // its instruction storage so reverse-use links always point to live definitions. for (auto* block: blocks) { @@ -184,6 +188,22 @@ Value ResolveInvariantPhi(const ResourcePlan& program, Value value) { return invariant; } +bool HasShaderMemoryWrites(const Program& program) { + for (const auto* block: program.blocks) { + for (const auto& inst: *block) { + const auto op = inst.GetOpcode(); + const auto buffer = BufferAccessOf(op); + const auto image = ImageOpcodeInfoOf(op).access; + if (buffer == BufferAccess::Write || buffer == BufferAccess::Atomic || + image == ImageAccess::Write || image == ImageAccess::Atomic || + AddressOpcodeInfoOf(op).access == AddressAccess::Write) { + return true; + } + } + } + return false; +} + void ValidateProgram(const Program& program, bool require_ssa) { if (program.blocks.size() != program.block_info.size() || program.blocks.size() != program.block_storage.size()) { diff --git a/src/graphics/shader/recompiler/ir/ShaderIR.h b/src/graphics/shader/recompiler/ir/ShaderIR.h index 8b6be623c..d87e217d1 100644 --- a/src/graphics/shader/recompiler/ir/ShaderIR.h +++ b/src/graphics/shader/recompiler/ir/ShaderIR.h @@ -563,6 +563,7 @@ struct Program: ResourcePlan { }; std::string ProgramToString(const Program& program); +bool HasShaderMemoryWrites(const Program& program); void ValidateProgram(const Program& program, bool require_ssa); void ResolveControlFlowIdentities(Program& program); diff --git a/src/graphics/shader/recompiler/ir/passes/ResourceMaterialization.cpp b/src/graphics/shader/recompiler/ir/passes/ResourceMaterialization.cpp index b4cc95ea7..0363ebb0b 100644 --- a/src/graphics/shader/recompiler/ir/passes/ResourceMaterialization.cpp +++ b/src/graphics/shader/recompiler/ir/passes/ResourceMaterialization.cpp @@ -647,7 +647,8 @@ bool BuildSamplerPlan(const ShaderInfo& base, const Images& images, SamplerPlan& } static std::vector ResourceControlFlow(const Program& program) { - if (program.blocks.size() != program.block_info.size()) { + // Any shader write may alias a scalar predicate read, including on a later loop visit. + if (program.blocks.size() != program.block_info.size() || HasShaderMemoryWrites(program)) { return {}; } std::unordered_map indices; @@ -687,12 +688,6 @@ static std::vector ResourceControlFlow(const Program& program) { const auto op = inst.GetOpcode(); const auto buffer = BufferAccessOf(op); const auto image = ImageOpcodeInfoOf(op); - // Any shader write may alias a scalar predicate read, including on a later loop visit. - if (buffer == BufferAccess::Write || buffer == BufferAccess::Atomic || - image.access == ImageAccess::Write || image.access == ImageAccess::Atomic || - AddressOpcodeInfoOf(op).access == AddressAccess::Write) { - return {}; - } if (buffer == BufferAccess::None && image.access == ImageAccess::None) { continue; } diff --git a/src/graphics/shader/recompiler/ir/passes/ResourceTracking.cpp b/src/graphics/shader/recompiler/ir/passes/ResourceTracking.cpp index f97942725..46c1fd407 100644 --- a/src/graphics/shader/recompiler/ir/passes/ResourceTracking.cpp +++ b/src/graphics/shader/recompiler/ir/passes/ResourceTracking.cpp @@ -88,6 +88,7 @@ public: m_info.samplers.clear(); m_info.sampled_pairs.clear(); m_info.uses_dma = false; + m_shader_writes = HasShaderMemoryWrites(program); } void Run() { @@ -168,15 +169,71 @@ private: std::abort(); } + Value LowerDescriptorPhi(Value value) { + value = value.Resolve(); + const auto* phi = value.TryInstruction(); + if (m_shader_writes || phi == nullptr || phi->GetOpcode() != ValueOpcode::Phi || + phi->NumArgs() != 2u || phi->NumPhiBlocks() != 2u || phi->GetType() != Type::U32 || + m_program.blocks.size() != m_program.block_info.size()) { + return value; + } + for (const auto& [original, selected]: m_descriptor_selections) { + if (original == phi) { + return selected; + } + } + const auto* merge = phi->Parent(); + for (uint32_t first = 0; first < 2; first++) { + const auto* branch = phi->PhiBlock(first); + const auto* alternate = phi->PhiBlock(first ^ 1u); + if (branch == nullptr || alternate == nullptr || merge == nullptr || branch == merge || + alternate == merge || branch == alternate || branch->ImmSuccessors().size() != 2u || + alternate->ImmPredecessors().size() != 1u || + alternate->ImmPredecessors()[0] != branch || + alternate->ImmSuccessors().size() != 1u || alternate->ImmSuccessors()[0] != merge) { + continue; + } + const auto branch_it = std::ranges::find(m_program.blocks, branch); + const auto alternate_it = std::ranges::find(m_program.blocks, alternate); + const auto merge_it = std::ranges::find(m_program.blocks, merge); + if (branch_it == m_program.blocks.end() || alternate_it == m_program.blocks.end() || + merge_it == m_program.blocks.end()) { + continue; + } + const auto& info = m_program.block_info[branch_it - m_program.blocks.begin()]; + const auto alternate_id = + m_program.block_info[alternate_it - m_program.blocks.begin()].id; + const auto merge_id = m_program.block_info[merge_it - m_program.blocks.begin()].id; + const auto& term = info.terminator; + if (term.kind != CFG::TerminatorKind::ConditionalBranch || + !((term.true_block == merge_id && term.false_block == alternate_id) || + (term.false_block == merge_id && term.true_block == alternate_id)) || + !ValidateRuntimeValue(m_program, info.condition, RuntimeValueType::Integer) || + !ValidateRuntimeValue(m_program, phi->Arg(0)) || + !ValidateRuntimeValue(m_program, phi->Arg(1))) { + continue; + } + // Retain a host expression; replacing the GPU Phi would break SSA dominance. + const auto true_arg = term.true_block == merge_id ? first : first ^ 1u; + auto& selected = m_program.value_storage.emplace_back(ValueOpcode::SelectU32); + selected.SetArg(0, info.condition); + selected.SetArg(1, phi->Arg(true_arg)); + selected.SetArg(2, phi->Arg(true_arg ^ 1u)); + m_descriptor_selections.emplace_back(phi, Value(&selected)); + return Value(&selected); + } + return value; + } + void MakeSource(const Inst& handle, uint32_t width, bool sampler, bool sample_adjust, - DescriptorSource& descriptor, uint32_t pc) const { + DescriptorSource& descriptor, uint32_t pc) { if (handle.NumArgs() != width) { Fail(pc, fmt::format("{} has {} descriptor dwords, expected {}", ValueOpcodeName(handle.GetOpcode()), handle.NumArgs(), width)); } descriptor.dword_count = width; for (uint32_t i = 0; i < width; i++) { - descriptor.dwords[i] = handle.Arg(i).Resolve(); + descriptor.dwords[i] = LowerDescriptorPhi(handle.Arg(i)); } if (sample_adjust) { descriptor.dwords[3] = CanonicalizeSampleAdjustDword3(descriptor.dwords[3]); @@ -744,12 +801,14 @@ private: } } - Program& m_program; - ShaderInfo m_info; - std::vector m_sources; - std::vector m_handle_patches; - std::vector m_memory_patches; - std::vector m_indirect_images; + Program& m_program; + ShaderInfo m_info; + std::vector m_sources; + std::vector m_handle_patches; + std::vector m_memory_patches; + std::vector m_indirect_images; + std::vector> m_descriptor_selections; + bool m_shader_writes = false; }; } // namespace diff --git a/src/graphics/shader/recompiler/ir/passes/SrtWalker.cpp b/src/graphics/shader/recompiler/ir/passes/SrtWalker.cpp index 2947c0f14..e099d2c6d 100644 --- a/src/graphics/shader/recompiler/ir/passes/SrtWalker.cpp +++ b/src/graphics/shader/recompiler/ir/passes/SrtWalker.cpp @@ -16,6 +16,13 @@ namespace { constexpr uint64_t AddressMask = 0x0000ffffffffffffull; +SrtRuntime CleanRuntime(SrtRuntime runtime) { + runtime.read_memory = runtime.read_specialization_memory != nullptr + ? runtime.read_specialization_memory + : +[](void*, uint64_t, uint32_t*) { return false; }; + return runtime; +} + const char* StageName(ShaderType stage) { switch (stage) { case ShaderType::Vertex: return "vertex"; @@ -646,8 +653,10 @@ private: case ValueOpcode::GetShaderBase: result = m_runtime.shader_base; return true; case ValueOpcode::Phi: return EvaluatePhi(inst, result); case ValueOpcode::ReadFirstLane: { - Evaluator active(m_program, m_runtime, m_clean_flat_slots, m_clean_evaluator, - inst.Arg(1)); + const auto clean_runtime = CleanRuntime(m_runtime); + Evaluator clean_active(m_program, clean_runtime, {}, nullptr, inst.Arg(1)); + Evaluator active(m_program, m_runtime, m_clean_flat_slots, &clean_active, + inst.Arg(1)); return active.EvaluateWide(inst.Arg(0), result); } case ValueOpcode::BitCastU32F32: @@ -892,12 +901,13 @@ private: } case ValueOpcode::SelectU32: case ValueOpcode::SelectU1: - case ValueOpcode::SelectF32: - if (ternary()) { - result = a != 0u ? b : c; - return true; + case ValueOpcode::SelectF32: { + auto& predicate = m_clean_evaluator != nullptr ? *m_clean_evaluator : *this; + if (predicate.EvaluateWide(inst.Arg(0), a)) { + return Arg(inst, a != 0u ? 1u : 2u, result); } return false; + } case ValueOpcode::IEqual32: if (binary()) { result = static_cast(a) == static_cast(b); @@ -977,7 +987,7 @@ bool EvaluateRuntimeSourcesImpl(const ResourcePlan& program, std::span& results, std::vector& flat, bool evaluate_flat, std::span clean_flat_slots, - std::vector& active_sources) { + std::vector& active_sources) { if (!program.srt_plan_complete) { return false; } @@ -985,8 +995,7 @@ bool EvaluateRuntimeSourcesImpl(const ResourcePlan& program, std::span active; @@ -1081,11 +1090,8 @@ bool EvaluateUniformValues(const ResourcePlan& program, std::span v if (values.size() != results.size()) { return false; } - auto clean = runtime; - clean.read_memory = runtime.read_specialization_memory != nullptr - ? runtime.read_specialization_memory - : +[](void*, uint64_t, uint32_t*) { return false; }; - Evaluator evaluator(program, clean); + const auto clean = CleanRuntime(runtime); + Evaluator evaluator(program, clean); for (size_t i = 0; i < values.size(); ++i) { if (!evaluator.Evaluate(values[i], results[i])) { return false; diff --git a/tests/ResourceTrackingTests.cpp b/tests/ResourceTrackingTests.cpp index 8d35cc480..51774c938 100644 --- a/tests/ResourceTrackingTests.cpp +++ b/tests/ResourceTrackingTests.cpp @@ -1208,6 +1208,125 @@ void TestPhiValidation() { "control-dependent descriptor phi was not rejected transactionally"); } +ResourcePlan ConditionalSamplerPlan(bool reverse, bool nonuniform = false, + bool writable = false) { + namespace CFG = Libs::Graphics::ShaderRecompiler::CFG; + Fixture fixture(ShaderType::Pixel); + auto *entry = fixture.block; + auto *alternate = fixture.AddBlock(); + auto *merge = fixture.AddBlock(); + entry->AddBranch(alternate); + entry->AddBranch(merge); + alternate->AddBranch(merge); + fixture.program.block_info[0].terminator = { + .kind = CFG::TerminatorKind::ConditionalBranch, + .true_block = reverse ? 1u : 2u, .false_block = reverse ? 2u : 1u}; + fixture.program.block_info[1].terminator = { + .kind = CFG::TerminatorKind::Branch, .true_block = 2}; + fixture.program.block_info[2].terminator.kind = CFG::TerminatorKind::Return; + const auto control = + fixture.Buffer({Value(0x2000u), Value(0u), Value(4u), Value(0u)}); + MemoryInfo scalar; + scalar.kind = ResourceKind::ScalarBuffer; + auto flag = fixture.Emit(ValueOpcode::ReadConstBuffer, + {control, Value(0u)}, fixture.AddMemory(scalar, 0x18c)); + if (nonuniform) { + flag = fixture.Emit(ValueOpcode::LaneId); + } + fixture.program.block_info[0].condition = + fixture.Emit(ValueOpcode::IEqual32, {flag, Value(0u)}); + if (writable) { + MemoryInfo memory; + memory.kind = ResourceKind::Buffer; + fixture.Emit(ValueOpcode::StoreBufferU32, + {control, Value(0u), Value(0u), Value(0u), Value(1u), Value(true)}, + fixture.AddMemory(memory, 0x170)); + } + + std::array sampler_words; + for (uint32_t word = 0; word < sampler_words.size(); ++word) { + const auto read = [&](Block *block, uint32_t address, uint32_t pc) { + const auto handle = fixture.Emit(ValueOpcode::GetAddressResource, + {Value(address), Value(0u)}, 0, block); + MemoryInfo memory; + memory.kind = ResourceKind::ScalarAddress; + memory.offset = word * 4; + return fixture.Emit(ValueOpcode::LoadAddressU32, + {handle, Value(0u), Value(0u), Value(true)}, + fixture.AddMemory(memory, pc), block); + }; + const auto initial = read(entry, 0x1000, 0x180); + const auto override = read(alternate, 0x1010, 0x1a0); + auto &phi = merge->AppendNewInst(ValueOpcode::Phi, {}, + static_cast(Type::U32)); + phi.AddPhiOperand(entry, initial); + phi.AddPhiOperand(alternate, override); + sampler_words[word] = Value(&phi); + } + fixture.block = merge; + const auto image = fixture.Image({Value(0u), Value(0u), Value(0u), Value(0u), + Value(0u), Value(0u), Value(0u), Value(0u)}); + const auto address = fixture.ImageAddress(); + for (uint32_t use = 0; use < 2; ++use) { + const auto sampler = fixture.Sampler(sampler_words); + MemoryInfo sample; + sample.kind = ResourceKind::Image; + sample.image_dimension = Decoder::ImageDimension::Dim2D; + const auto result = fixture.Emit(ValueOpcode::ImageSampleRaw, + {image, sampler, address}, + fixture.AddMemory(sample, 0x214)); + fixture.Emit(ValueOpcode::ReferenceU32, + {fixture.Emit(ValueOpcode::CompositeExtractU32x4, + {result, Value(0u)})}); + } + fixture.PlanAndTrack(); + Check(fixture.program.value_storage.size() == 4, + "repeated sampler uses retained duplicate planning selections"); + Check(sampler_words[0].ResolveInstruction()->GetOpcode() == ValueOpcode::Phi, + "host descriptor selection changed the GPU Phi"); + EliminateDeadCode(fixture.program.blocks); + ValidateProgram(fixture.program, true); + return ExtractResourcePlan(fixture.program); +} + +void TestConditionalSamplerPhi() { + for (const bool reverse : {false, true}) { + auto plan = ConditionalSamplerPlan(reverse); + const auto source = plan.info.samplers[0].source; + LinearTestMemory memory; + for (uint32_t word = 0; word < 8; ++word) { + memory.words[word] = 0x400u + word; + } + const SrtRuntime runtime{.read_memory = ReadLinearTestMemory, + .userdata = &memory, + .read_specialization_memory = ReadLinearTestMemory}; + for (uint32_t flag = 0; flag < 2; ++flag) { + memory.words[0x1000 / 4] = flag; + const uint32_t first = (flag != 0u) != reverse ? 4u : 0u; + memory.fail_address = first == 0u ? 0x1010u : 0x1000u; + DescriptorValue selected; + Check(EvaluateDescriptorSource(plan, source, runtime, selected), + "conditional sampler did not survive detached plan lifetime"); + for (uint32_t word = 0; word < 4; ++word) { + Check(selected.dwords[word] == memory.words[first + word], + "conditional sampler chose the wrong incoming descriptor"); + } + } + DescriptorValue selected; + auto no_clean_reader = runtime; + no_clean_reader.read_specialization_memory = nullptr; + Check(!EvaluateDescriptorSource(plan, source, no_clean_reader, selected), + "conditional sampler used unchecked memory for its predicate"); + memory.fail_address = 0x2000; + Check(!EvaluateDescriptorSource(plan, source, runtime, selected), + "conditional sampler ignored unavailable coherent predicate memory"); + } + CheckFatal([] { ConditionalSamplerPlan(false, true); }, + "not a valid runtime value", "nonuniform sampler selection was accepted"); + CheckFatal([] { ConditionalSamplerPlan(false, false, true); }, + "not a valid runtime value", "shader-written sampler predicate was accepted"); +} + void TestLoopCycleEnteredThroughRuntimeValue() { Fixture fixture; auto *entry = fixture.block; @@ -1865,6 +1984,7 @@ int main() { Run("SRT runtime", TestSrtFlatteningAndRuntimeMemoization); Run("dynamic SRT", TestDynamicSrtReadRemainsExplicit); Run("phi validation", TestPhiValidation); + Run("conditional sampler phi", TestConditionalSamplerPhi); Run("runtime-rooted loop", TestLoopCycleEnteredThroughRuntimeValue); Run("invariant loop phi", TestInvariantLoopPhi); Run("DMA address materialization", TestDmaAddressMaterialization);