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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VZWLdTjf1S57iLgn2rcaFb
This commit is contained in:
parent
d592d54e11
commit
7fd423dfe9
|
|
@ -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:$<TARGET_FILE_DIR:libc>")
|
||||
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)
|
||||
|
|
|
|||
|
|
@ -0,0 +1,73 @@
|
|||
#include "Recompiler.hpp"
|
||||
#include <spirv/unified1/spirv.hpp>
|
||||
#include <array>
|
||||
#include <cstdint>
|
||||
#include <cstdio>
|
||||
#include <exception>
|
||||
#include <span>
|
||||
#include <stdexcept>
|
||||
#include <string>
|
||||
#include <string_view>
|
||||
#include <vector>
|
||||
|
||||
namespace {
|
||||
|
||||
using namespace ShaderRecompiler;
|
||||
|
||||
void require(bool condition, const std::string& message) {
|
||||
if (!condition) throw std::runtime_error(message);
|
||||
}
|
||||
|
||||
std::vector<std::uint32_t> recompile(std::span<const std::uint32_t> code) {
|
||||
const std::array<std::uint32_t, 12> userData{0x10000000u, 0x00100000u, 0x40u, 0x00027facu, 0u, 0u, 0u, 0u, 7u, 9u, 0x10002000u, 0u};
|
||||
const std::array<std::string_view, 1> extensions{"SPV_KHR_storage_buffer_storage_class"};
|
||||
const std::array<std::uint32_t, 2> 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<const std::uint32_t> 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<std::uint32_t, 2> zeroOffsetBranch{0xbf840000u, 0xbf810000u};
|
||||
requireDistinctTargets("s_cbranch_scc0 to the next instruction", zeroOffsetBranch);
|
||||
constexpr std::array<std::uint32_t, 4> 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;
|
||||
}
|
||||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Reference in New Issue