From 7fd423dfe97e19f3a42a490aa45791065d1a1dcd Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 16:04:13 +0000 Subject: [PATCH] fix(shader): never branch conditionally to one label twice SPIR-V 1.6 requires the true and false labels of OpBranchConditional to differ. Two places produced the same label for both: - The structured terminator emitter, for a conditional branch whose targets are the same block, such as s_cbranch_scc0 to the next instruction. It now emits OpBranch, keeping OpLoopMerge on a loop header. - Pipeline specialization, when it folded a branch on a constant and kept the selection merge because the taken path is not linear. It wrote the taken label twice; the untaken side now names the selection's merge block, which the constant condition never takes. Found by recompiling mutated compute shaders with SPIR-V validation on; before this, 169 of 20000 were rejected for this rule, and builds without SPIRV-Tools handed the module to the driver. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VZWLdTjf1S57iLgn2rcaFb --- core/libs/prx/libSceAgcDriver/CMakeLists.txt | 13 ++++ .../tests/SpirvBranchTargets.cpp | 73 +++++++++++++++++++ .../SpirvBackend/src/SpirvFlowEmitter.cpp | 5 ++ .../SpirvBackend/src/SpirvSpecialization.cpp | 7 +- 4 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 core/libs/prx/libSceAgcDriver/tests/SpirvBranchTargets.cpp diff --git a/core/libs/prx/libSceAgcDriver/CMakeLists.txt b/core/libs/prx/libSceAgcDriver/CMakeLists.txt index 86d62006f..f50b3a734 100644 --- a/core/libs/prx/libSceAgcDriver/CMakeLists.txt +++ b/core/libs/prx/libSceAgcDriver/CMakeLists.txt @@ -466,6 +466,19 @@ if(BUILD_TESTING) endif() endif() +add_test_executable(agc_spirv_branch_targets_tests EXCLUDE_FROM_ALL tests/SpirvBranchTargets.cpp) +target_link_libraries(agc_spirv_branch_targets_tests PRIVATE shader_recompiler_agc_driver libc) +set_target_properties(agc_spirv_branch_targets_tests PROPERTIES CXX_EXTENSIONS OFF) +configure_windows_unwind(agc_spirv_branch_targets_tests) +if(BUILD_TESTING) + set_target_properties(agc_spirv_branch_targets_tests PROPERTIES EXCLUDE_FROM_ALL OFF) + add_test(NAME agc_spirv_branch_targets COMMAND agc_spirv_branch_targets_tests) + set_tests_properties(agc_spirv_branch_targets PROPERTIES ENVIRONMENT "ANYPS5_NO_SHADER_CACHE=1") + if(WIN32) + set_tests_properties(agc_spirv_branch_targets PROPERTIES ENVIRONMENT_MODIFICATION "PATH=path_list_prepend:$") + endif() +endif() + add_test_executable(agc_spirv_lds_atomic_lock_tests EXCLUDE_FROM_ALL tests/SpirvLdsAtomicLock.cpp) target_link_libraries(agc_spirv_lds_atomic_lock_tests PRIVATE shader_recompiler_agc_driver libc) set_target_properties(agc_spirv_lds_atomic_lock_tests PROPERTIES CXX_EXTENSIONS OFF) diff --git a/core/libs/prx/libSceAgcDriver/tests/SpirvBranchTargets.cpp b/core/libs/prx/libSceAgcDriver/tests/SpirvBranchTargets.cpp new file mode 100644 index 000000000..dda775da2 --- /dev/null +++ b/core/libs/prx/libSceAgcDriver/tests/SpirvBranchTargets.cpp @@ -0,0 +1,73 @@ +#include "Recompiler.hpp" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +using namespace ShaderRecompiler; + +void require(bool condition, const std::string& message) { + if (!condition) throw std::runtime_error(message); +} + +std::vector recompile(std::span code) { + const std::array userData{0x10000000u, 0x00100000u, 0x40u, 0x00027facu, 0u, 0u, 0u, 0u, 7u, 9u, 0x10002000u, 0u}; + const std::array extensions{"SPV_KHR_storage_buffer_storage_class"}; + const std::array capabilities{spv::CapabilityShader, spv::CapabilityGroupNonUniform}; + RecompileRequest request{}; + request.shader = {ShaderStage::Compute, 0x20000u, code, 0, {}}; + request.context.waveSize = 64; + request.context.userDataBaseRegister = 0; + request.context.userData = userData; + request.context.compute = ShaderComputeStageInfo{{64u, 1u, 1u}, 0u, {false, false, false}, false, 1u}; + request.target.vulkanVersion = 0x00403000u; + request.target.spirvVersion = 0x00010600u; + request.target.subgroupSize = 64; + request.target.bdaAbiVersion = 1; + request.target.supportedCapabilities = capabilities; + request.target.supportedExtensions = extensions; + request.target.maxWorkgroupSize = {1024u, 1024u, 64u}; + request.target.maxWorkgroupInvocations = 1024; + request.target.maxWorkgroupSharedMemoryBytes = 49152; + request.layout = {0, 0, 0, 128}; + request.useCache = false; + auto result = Recompile(request); + require(!result.spirv.empty(), "the shader did not recompile"); + return std::move(result.spirv); +} + +void requireDistinctTargets(const char* name, std::span code) { + const auto words = recompile(code); + for (std::size_t cursor = 5; cursor < words.size();) { + const auto count = words[cursor] >> 16u; + require(count != 0 && cursor + count <= words.size(), std::string(name) + ": truncated SPIR-V instruction"); + if ((words[cursor] & 0xffffu) == spv::OpBranchConditional) { + require(words[cursor + 2] != words[cursor + 3], std::string(name) + ": OpBranchConditional with the same true and false label, invalid in SPIR-V 1.6"); + } + cursor += count; + } +} + +} + +int main() { + try { + constexpr std::array zeroOffsetBranch{0xbf840000u, 0xbf810000u}; + requireDistinctTargets("s_cbranch_scc0 to the next instruction", zeroOffsetBranch); + constexpr std::array unalignedStore{0x7e020281u, 0xe0700000u, 0x80020100u, 0xbf810000u}; + requireDistinctTargets("store through a descriptor with an unaligned base", unalignedStore); + } catch (const std::exception& error) { + std::fprintf(stderr, "SpirvBranchTargets: %s\n", error.what()); + return 1; + } + std::puts("SpirvBranchTargets: no conditional branch names one label twice"); + return 0; +} diff --git a/core/shader/recompiler/SpirvBackend/src/SpirvFlowEmitter.cpp b/core/shader/recompiler/SpirvBackend/src/SpirvFlowEmitter.cpp index 5401643be..cf422ba12 100644 --- a/core/shader/recompiler/SpirvBackend/src/SpirvFlowEmitter.cpp +++ b/core/shader/recompiler/SpirvBackend/src/SpirvFlowEmitter.cpp @@ -217,6 +217,11 @@ void EmitStructuredTerminator(SpirvValueEmitContext& ctx, const IrProgram& progr const std::string message = "branch condition " + std::string(IrOpcodeName(resolved->Opcode())) + " of block " + std::to_string(info.id) + " (defined in " + where + ") was not emitted before the branch"; ctx.Fail(message.c_str()); } + if (ctx.Label(trueBlock) == ctx.Label(falseBlock)) { + if (term.loopHeader) emitMerge(); + state.module.AddFunction(spv::OpBranch, ctx.Label(trueBlock)); + return; + } auto condition = EmitBranchCondition(ctx, info); if (state.loopGuardLimit != 0 && term.trueBlock != term.falseBlock) { if (IsLoopMerge(program, term.trueBlock)) condition = GuardLoopExit(state, info, condition, true); diff --git a/core/shader/recompiler/SpirvBackend/src/SpirvSpecialization.cpp b/core/shader/recompiler/SpirvBackend/src/SpirvSpecialization.cpp index 6d8d41477..fddf14b8f 100644 --- a/core/shader/recompiler/SpirvBackend/src/SpirvSpecialization.cpp +++ b/core/shader/recompiler/SpirvBackend/src/SpirvSpecialization.cpp @@ -441,14 +441,17 @@ private: } if (target != 0u) { bool retainMerge = false; + std::uint32_t merge = 0; for (const auto index : indices) { auto& instruction = instructions[index]; if (Opcode(instruction) != spv::OpSelectionMerge) continue; - retainMerge = !linearSelection(target, instruction.at(1)); + merge = instruction.at(1); + retainMerge = !linearSelection(target, merge); if (!retainMerge) instruction.clear(); } + const bool takesTrue = op == spv::OpBranchConditional && target == terminal.at(2); const auto replacement = !retainMerge ? Make(spv::OpBranch, {target}) : op == spv::OpSwitch ? - Make(spv::OpSwitch, {terminal.at(1), target}) : Make(spv::OpBranchConditional, {terminal.at(1), target, target}); + Make(spv::OpSwitch, {terminal.at(1), target}) : Make(spv::OpBranchConditional, {terminal.at(1), takesTrue ? target : merge, takesTrue ? merge : target}); if (terminal != replacement) { terminal = replacement; changed = true;