From 4919fdb6e6e28f46f1f5df8cf931a7a0823cb93f Mon Sep 17 00:00:00 2001 From: nmzik Date: Fri, 30 Oct 2026 09:35:45 +0100 Subject: [PATCH] shader: validate the IR once, at SPIR-V emission --- .../recompiler/backend/spirv/SpirvEmitter.cpp | 2 +- .../frontend/translate/Translate.cpp | 1 - src/graphics/shader/recompiler/ir/Program.cpp | 4 +- src/graphics/shader/recompiler/ir/ShaderIR.h | 2 +- tests/ResourceTrackingTests.cpp | 6 +-- tests/ScalarProvenanceTests.cpp | 6 +-- tests/shaderCfgTests.cpp | 44 ++++++++++++++++--- 7 files changed, 47 insertions(+), 18 deletions(-) diff --git a/src/graphics/shader/recompiler/backend/spirv/SpirvEmitter.cpp b/src/graphics/shader/recompiler/backend/spirv/SpirvEmitter.cpp index 6f6028a02..8e9c9a199 100644 --- a/src/graphics/shader/recompiler/backend/spirv/SpirvEmitter.cpp +++ b/src/graphics/shader/recompiler/backend/spirv/SpirvEmitter.cpp @@ -219,7 +219,7 @@ std::vector EmitProgram(IR::Program& program, ShaderStageInputInfo inp !program.shader_info_complete) { Fail(program, "SPIR-V emitter requires a fully planned native shader program"); } - IR::ValidateProgram(program, true); + IR::ValidateProgram(program); program.bindings = {.push_data_start_dword = push_data_start_dword}; EmitterState state(program, input_info, profile); if (emit_debug_info) { diff --git a/src/graphics/shader/recompiler/frontend/translate/Translate.cpp b/src/graphics/shader/recompiler/frontend/translate/Translate.cpp index 76a537033..01ef14af6 100644 --- a/src/graphics/shader/recompiler/frontend/translate/Translate.cpp +++ b/src/graphics/shader/recompiler/frontend/translate/Translate.cpp @@ -1446,7 +1446,6 @@ IR::Program TranslateProgram(const Decoder::Program& decoded, const CFG::Graph& } translator.AddBranchCondition(cfg, cfg_block, *block); } - IR::ValidateProgram(result, false); return result; } diff --git a/src/graphics/shader/recompiler/ir/Program.cpp b/src/graphics/shader/recompiler/ir/Program.cpp index f220f9603..ef23b2c04 100644 --- a/src/graphics/shader/recompiler/ir/Program.cpp +++ b/src/graphics/shader/recompiler/ir/Program.cpp @@ -244,7 +244,7 @@ bool HasShaderMemoryWrites(const Program& program) { return false; } -void ValidateProgram(const Program& program, bool require_ssa) { +void ValidateProgram(const Program& program) { if (program.blocks.size() != program.block_storage.size()) { return Fail("value IR block storage is inconsistent"); } @@ -400,7 +400,7 @@ void ValidateProgram(const Program& program, bool require_ssa) { } else { saw_non_phi = true; } - if (require_ssa && IsRegisterStatePseudo(inst.GetOpcode())) { + if (IsRegisterStatePseudo(inst.GetOpcode())) { return Fail(fmt::format("register-state pseudo {} survived SSA rewrite", ValueOpcodeName(inst.GetOpcode()))); } diff --git a/src/graphics/shader/recompiler/ir/ShaderIR.h b/src/graphics/shader/recompiler/ir/ShaderIR.h index 09f6b03b6..d4cb31e50 100644 --- a/src/graphics/shader/recompiler/ir/ShaderIR.h +++ b/src/graphics/shader/recompiler/ir/ShaderIR.h @@ -751,7 +751,7 @@ uint32_t StorageBufferElementBits(const Program& program, const MemoryInfo& memo std::string ProgramToString(const Program& program); bool HasShaderMemoryWrites(const Program& program); -void ValidateProgram(const Program& program, bool require_ssa); +void ValidateProgram(const Program& program); bool EquivalentValue(const ResourcePlan& program, Value left, Value right); bool EquivalentSelectorDomain(const ResourcePlan& program, const DescriptorSource::IndirectDescriptor& left, diff --git a/tests/ResourceTrackingTests.cpp b/tests/ResourceTrackingTests.cpp index 9a6315ede..54287ab6c 100644 --- a/tests/ResourceTrackingTests.cpp +++ b/tests/ResourceTrackingTests.cpp @@ -319,7 +319,7 @@ void TestInvariantIndirectImageMaterialization(bool nested) { fixture->PlanAndTrack(); auto plan = ExtractResourcePlan(fixture->program); EliminateDeadCode(fixture->program.blocks); - ValidateProgram(fixture->program, true); + ValidateProgram(fixture->program); const auto &source = *plan.descriptor_sources[plan.info.images[0].source].indirect_descriptor; Check(!source.selector && fixture->program.info.buffers.size() == (nested ? 3u : 2u) && std::ranges::any_of(*fixture->block, [](const Inst &inst) { @@ -7589,7 +7589,7 @@ ResourcePlan ConditionalSamplerPlan(bool diamond, bool reverse, bool reverse_phi Check(sampler_words[0].ResolveInstruction()->GetOpcode() == ValueOpcode::Phi, "host descriptor selection changed the GPU Phi"); EliminateDeadCode(fixture.program.blocks); - ValidateProgram(fixture.program, true); + ValidateProgram(fixture.program); return ExtractResourcePlan(fixture.program); } @@ -7855,7 +7855,7 @@ void TestFiniteImageWaterfall() { fixture->PlanAndTrack(); const auto plan = ExtractResourcePlan(fixture->program); EliminateDeadCode(fixture->program.blocks); - ValidateProgram(fixture->program, true); + ValidateProgram(fixture->program); const auto& finite = plan.descriptor_sources[plan.info.images[0].source].indirect_descriptor; Check(finite && finite->sources.size() == 3u, "waterfall image selection mixed descriptor words or lost a candidate"); diff --git a/tests/ScalarProvenanceTests.cpp b/tests/ScalarProvenanceTests.cpp index fd4e4a5a0..7bb333ffc 100644 --- a/tests/ScalarProvenanceTests.cpp +++ b/tests/ScalarProvenanceTests.cpp @@ -788,7 +788,7 @@ void TestCompleteWaveReduction() { fixture.BlockAt().terminator.kind = Terminator::Branch; fixture.BlockAt().terminator.true_block = &fixture.BlockAt(1); } - ValidateProgram(fixture.program, true); + ValidateProgram(fixture.program); // The pipeline folds full-EXEC predicates to true before eliminating reads. const uint32_t wave_size = test == Case::Wave32 || rows ? 32u : 64u; ConstantPropagationPass(fixture.program.blocks, wave_size); @@ -812,7 +812,7 @@ void TestCompleteWaveReduction() { } Check(updates == 2 && preserved.Instruction()->Arg(0).Instruction()->HasUses(), "wave reduction destroyed an independently consumed prefix or kept its dead suffix"); - ValidateProgram(fixture.program, true); + ValidateProgram(fixture.program); } else { Check(use.Instruction()->Arg(0).Resolve() == result, "rejected wave reduction proof changed its original result"); @@ -873,7 +873,7 @@ void TestControlFlowValueSurvivesReadLaneFolding() { Check(entry_info.condition == Value(true), "folded branch condition did not survive identity removal"); - ValidateProgram(fixture.program, true); + ValidateProgram(fixture.program); } void TestDeadPhiCyclesAndPlanningRoots() { diff --git a/tests/shaderCfgTests.cpp b/tests/shaderCfgTests.cpp index 60ccdd7f0..53a4fc715 100644 --- a/tests/shaderCfgTests.cpp +++ b/tests/shaderCfgTests.cpp @@ -12778,7 +12778,7 @@ void TestTypedEntryStateIsMinimal() { IR::RewriteToSsa(values.blocks); IR::RemoveIdentities(values); IR::EliminateDeadCode(values.blocks); - IR::ValidateProgram(values, true); + IR::ValidateProgram(values); }; check(32u); @@ -12799,13 +12799,42 @@ void TestFinalSsaRejectsRegisterStatePseudos() { } else { ir.Emit(ValueOpcode::ReferenceU32, {ir.GetScalarReg(ScalarReg{0})}); } - ExpectFatal([&] { ValidateProgram(program, true); }, + ExpectFatal([&] { ValidateProgram(program); }, "final SSA accepted a register-state pseudo operation"); }; check_rejected(false); check_rejected(true); } + +void TestEmitterValidatesFinalProgram() { + using namespace ShaderRecompiler; + const uint32_t shader[] = { + EncodeMubuf0(0x1c, 0, false), // buffer_store_dword v0 + EncodeMubuf1(0, 12, 0), + 0xbf810000u, + }; + const auto options = MakeCompileOptions(ShaderType::Compute); + auto result = RecompileForTest(shader, options); + // Only emission validates, so a definition moved below its same-block user must be fatal. + bool moved = false; + for (auto *block : result.program.blocks) { + auto &list = block->Instructions(); + for (auto user = list.begin(); !moved && user != list.end(); ++user) { + for (size_t arg = 0; !moved && arg < user->NumArgs(); ++arg) { + const auto *def = user->Arg(arg).TryInstruction(); + if (def == nullptr || def->Parent() != block) continue; + const auto where = std::ranges::find_if( + list, [&](const IR::Inst &inst) { return &inst == def; }); + list.splice(std::next(user), list, where); + moved = true; + } + } + } + Check(moved, "emitter fixture has no same-block definition"); + ExpectFatal([&] { Spirv::EmitProgram(result.program, options.input_info); }, + "SPIR-V emission accepted a use before its definition"); +} #endif void TestSsaRewriteSealsFilledBlocks() { @@ -12888,7 +12917,7 @@ void TestValuePhiValidation() { #if KYTY_PLATFORM != KYTY_PLATFORM_WINDOWS const auto expect_invalid = [](const Program &program, const char *text) { - ExpectFatal([&] { ValidateProgram(program, true); }, text); + ExpectFatal([&] { ValidateProgram(program); }, text); }; #endif const auto validate = [&](InvalidPhi invalid) { @@ -12940,7 +12969,7 @@ void TestValuePhiValidation() { } if (invalid == InvalidPhi::None) { - ValidateProgram(program, true); + ValidateProgram(program); } #if KYTY_PLATFORM != KYTY_PLATFORM_WINDOWS else { @@ -13306,7 +13335,7 @@ void TestLaneMaskProjection() { gated_raw_consumer.Resolve().TryInstruction()->Arg(0).Resolve() == gated_mask.Resolve(), "mask projection changed a raw scalar mask consumer"); RemoveIdentities(program); - ValidateProgram(program, true); + ValidateProgram(program); } } @@ -13376,7 +13405,7 @@ void TestLateConstantPropagation() { "late constants did not reach Phi, nested mask patterns or new logical nodes"); RemoveIdentities(program); Check(exit->condition == Value(true), "late constant lost its control-flow use"); - ValidateProgram(program, true); + ValidateProgram(program); } } @@ -13586,7 +13615,7 @@ void TestNativeWideValueValidation() { } }; const auto check_rejected = [](const Program &program, const char *text) { - ExpectFatal([&] { ValidateProgram(program, true); }, text); + ExpectFatal([&] { ValidateProgram(program); }, text); }; { @@ -17404,6 +17433,7 @@ int main() { TestTypedEntryStateIsMinimal(); #if KYTY_PLATFORM != KYTY_PLATFORM_WINDOWS TestFinalSsaRejectsRegisterStatePseudos(); + TestEmitterValidatesFinalProgram(); #endif TestSsaRewriteSealsFilledBlocks(); TestValuePhiValidation();