From 86414760e6c0124d5c07ece1e7850f669a837e4b Mon Sep 17 00:00:00 2001 From: nmzik Date: Tue, 15 Sep 2026 09:50:12 +0400 Subject: [PATCH] Cull zero homogeneous vertex positions before rasterization Fix Playroom top right artifact on Nvidia by clipping zero-position vertices. --- src/common/logging/log.cpp | 8 +++ src/common/logging/log.h | 1 + .../renderer/pipeline/pipelineCache.cpp | 7 +- .../backend/spirv/spirvEmitterFlow.cpp | 28 ++++++++ .../backend/spirv/spirvEmitterInternal.h | 1 + .../backend/spirv/spirvEmitterModule.cpp | 9 +++ tests/ShaderRecompilerComputeTests.cpp | 70 +++++++++++++++++-- tests/shaderCfgTests.cpp | 28 ++++++-- 8 files changed, 135 insertions(+), 17 deletions(-) diff --git a/src/common/logging/log.cpp b/src/common/logging/log.cpp index 43264b69d..2777801c9 100644 --- a/src/common/logging/log.cpp +++ b/src/common/logging/log.cpp @@ -129,6 +129,14 @@ static void WriteImpl(std::string_view text, fmt::text_style style = {}) { } } +void WriteToConsoleAndLog(std::string_view text) { + WriteImpl(text); + if (g_initialized && g_direction != Direction::Console) { + WriteStdout(text); + } + Flush(); +} + void WriteFatal(std::string_view text) { if (g_direction == Direction::Silent || !g_initialized) { WriteStdout(text); diff --git a/src/common/logging/log.h b/src/common/logging/log.h index bbe2d2cd4..56416cbc8 100644 --- a/src/common/logging/log.h +++ b/src/common/logging/log.h @@ -25,6 +25,7 @@ Direction GetDirection(); bool IsSilent(); void Write(std::string_view text); void Write(fmt::text_style style, std::string_view text); +void WriteToConsoleAndLog(std::string_view text); void WriteFatal(std::string_view text); void WriteFatal(fmt::text_style style, std::string_view text); void Flush(); diff --git a/src/graphics/host_gpu/renderer/pipeline/pipelineCache.cpp b/src/graphics/host_gpu/renderer/pipeline/pipelineCache.cpp index 20f9e5ce4..44d8ba745 100644 --- a/src/graphics/host_gpu/renderer/pipeline/pipelineCache.cpp +++ b/src/graphics/host_gpu/renderer/pipeline/pipelineCache.cpp @@ -89,12 +89,7 @@ template void PipelineCacheLog(fmt::format_string format, Args&&... args) { auto message = fmt::format(format, std::forward(args)...); message += '\n'; - if (Log::GetDirection() != Log::Direction::Console) { - std::fwrite(message.data(), 1, message.size(), stdout); - std::fflush(stdout); - } - Log::Write(message); - Log::Flush(); + Log::WriteToConsoleAndLog(message); } bool ReadShaderGuestMemory(void*, uint64_t address, uint32_t* value) { diff --git a/src/graphics/shader/recompiler/backend/spirv/spirvEmitterFlow.cpp b/src/graphics/shader/recompiler/backend/spirv/spirvEmitterFlow.cpp index 106ebaa30..99a09b6d5 100644 --- a/src/graphics/shader/recompiler/backend/spirv/spirvEmitterFlow.cpp +++ b/src/graphics/shader/recompiler/backend/spirv/spirvEmitterFlow.cpp @@ -1,6 +1,9 @@ #include "graphics/shader/recompiler/backend/spirv/spirvEmitterInstructions.h" +#include "common/logging/log.h" + #include +#include namespace Libs::Graphics::ShaderRecompiler::Spirv::Emitter { namespace { @@ -500,6 +503,31 @@ void EmitSetAttribute(ValueEmitContext& ctx, const IR::Inst& inst) { state.builder.AddFunction(spv::OpStore, MeshOutputPointer(state, kind, exp.index), value); } else if (exp.kind == IR::ExportTargetKind::Position) { + if (state.invalid_position_clip_distance != UINT32_MAX) { + const auto zero = state.builder.Constant(spv::OpConstantNull, TypeF32Vector(state, 4)); + const auto equal = state.builder.AllocateId(); + const auto invalid = state.builder.AllocateId(); + const auto distance = state.builder.AllocateId(); + const auto distance_pointer = state.builder.AllocateId(); + state.builder.AddFunction(spv::OpFOrdEqual, TypeBoolVector(state, 4), equal, + value, zero); + state.builder.AddFunction(spv::OpAll, TypeBool(state), invalid, equal); + // Zero at valid vertices makes a primitive containing an invalid position + // collapse to its remaining edge, before the undefined 0/0 perspective divide. + state.builder.AddFunction(spv::OpSelect, TypeF32(state), distance, invalid, + ConstantF32Value(state, -1.0f), + ConstantF32Value(state, 0.0f)); + state.builder.AddFunction( + spv::OpAccessChain, TypePointer(state, spv::StorageClassOutput, TypeF32(state)), + distance_pointer, state.clip_distance_variable, + ConstantU32(state, state.invalid_position_clip_distance)); + state.builder.AddFunction(spv::OpStore, distance_pointer, distance); + static std::atomic_bool logged = false; + if (!logged.exchange(true, std::memory_order_relaxed)) { + Log::WriteToConsoleAndLog( + "Shader: emitted zero-position clip guard\n"); + } + } const auto pointer = state.builder.AllocateId(); state.builder.AddFunction( spv::OpAccessChain, diff --git a/src/graphics/shader/recompiler/backend/spirv/spirvEmitterInternal.h b/src/graphics/shader/recompiler/backend/spirv/spirvEmitterInternal.h index a2e9eda09..b6b6d9b3b 100644 --- a/src/graphics/shader/recompiler/backend/spirv/spirvEmitterInternal.h +++ b/src/graphics/shader/recompiler/backend/spirv/spirvEmitterInternal.h @@ -116,6 +116,7 @@ struct EmitterState { uint32_t per_vertex_variable = 0; uint32_t point_size_variable = 0; uint32_t clip_distance_variable = 0; + uint32_t invalid_position_clip_distance = UINT32_MAX; uint32_t cull_distance_variable = 0; uint32_t layer_variable = 0; uint32_t viewport_index_variable = 0; diff --git a/src/graphics/shader/recompiler/backend/spirv/spirvEmitterModule.cpp b/src/graphics/shader/recompiler/backend/spirv/spirvEmitterModule.cpp index add068bb1..4292adbd5 100644 --- a/src/graphics/shader/recompiler/backend/spirv/spirvEmitterModule.cpp +++ b/src/graphics/shader/recompiler/backend/spirv/spirvEmitterModule.cpp @@ -476,6 +476,15 @@ void DefineOutputs(EmitterState& state) { DefineMeshOutputs(state); return; } + if (state.program.stage == ShaderType::Vertex && clip_distance_count + cull_distance_count < 8u && + std::ranges::any_of(state.outputs, [](const OutputBinding& output) { + return output.kind == IR::StageOutputKind::Position; + })) { + // Reserve one plane for the enabled PA_CL_CLIP_CNTL clipping-error cull. + state.invalid_position_clip_distance = clip_distance_count++; + state.outputs.push_back({{IR::StageOutputKind::ClipDistance, + state.invalid_position_clip_distance, 0, "gl_ClipDistance"}}); + } const auto BuiltIn = [&](uint32_t& variable, uint32_t type, const char* name, spv::BuiltIn builtin) { if (variable == 0) { diff --git a/tests/ShaderRecompilerComputeTests.cpp b/tests/ShaderRecompilerComputeTests.cpp index 7d7a2e4d1..8347e5626 100644 --- a/tests/ShaderRecompilerComputeTests.cpp +++ b/tests/ShaderRecompilerComputeTests.cpp @@ -12717,7 +12717,9 @@ public: vertex_words); const auto pipeline = [&](bool enabled, uint8_t front, uint8_t back, bool provoking_last = false, - bool clockwise = false) -> PipelineCache::Pipeline & { + bool clockwise = false, + vk::PrimitiveTopology topology = vk::PrimitiveTopology::eTriangleList) + -> PipelineCache::Pipeline & { HW::ModeControl mode{}; mode.face = clockwise; mode.poly_mode = enabled; @@ -12727,11 +12729,11 @@ public: registers.SetModeControl(mode); return context.GetPipelineCache().GetGraphicsPipeline( std::span{&color, 1u}, depth, std::span{&vertex, 1u}, scheduler.Current(), &pixel, - vk::PrimitiveTopology::eTriangleList, false, + topology, false, PipelineCache::GraphicsPrograms{{vertex_shader}, pixel_shader}); }; auto &filled = pipeline(true, 2, 2); - const auto draw = [&](const PipelineCache::Pipeline &selected) { + const auto draw = [&](const PipelineCache::Pipeline &selected, uint32_t vertex_count = 3) { RenderExecutorTestAccess::BindRenderTarget(executor, color.image_id); if (depth.image_id) { RenderExecutorTestAccess::BindRenderTarget(executor, depth.image_id); @@ -12785,7 +12787,7 @@ public: : vk::ImageAspectFlags{}); const vk::DeviceSize offset = 0; cmd.bindVertexBuffers(0, 1, &buffer.buffer, &offset); - cmd.draw(3, 1, 0, 0); + cmd.draw(vertex_count, 1, 0, 0); command.EndRendering(); RenderExecutorTestAccess::ResetBindings(executor); }; @@ -13087,6 +13089,66 @@ public: Require(name, "pixel wave cache distinction", wave_ids[0] != wave_ids[1] && wave_keys[0] != wave_keys[1], "wave32 and wave64 pixel programs shared a cache key"); + + // Captured Playroom strips contain the fullscreen triangle followed by an + // out-of-bounds fetch exporting (0,0,0,0). Additive color reveals any extra triangle. + static constexpr std::array, 4> fourth_positions{{ + {0, 0, 0, 0}, {0x80000000u, 0, 0x80000000u, 0x80000000u}, + {0, 0, 0, 0x3f800000u}, {0xbf800000u, 0xbf800000u, 0, 0}}}; + static const auto position_shaders = [&] { + std::array, fourth_positions.size()> result; + for (size_t i = 0; i < result.size(); i++) { + auto &code = result[i]; + const auto position_export = std::ranges::find(native_vertex, EncodeExp0(0x0c, 0xf)); + code.assign(native_vertex.begin(), position_export); + code.push_back(EncodeVopc(0xc2, InlineU32(3), 5)); + constexpr std::array position_registers{3, 4, 0, 6}; + for (size_t component = 0; component < position_registers.size(); component++) { + AppendVMovLiteral(&code, 7, fourth_positions[i][component]); + const auto reg = position_registers[component]; + code.push_back(EncodeVop2(0x01, reg, Vgpr(reg), 7)); + } + code.insert(code.end(), position_export, native_vertex.end()); + } + return result; + }(); + auto additive_blend = registers.GetBlendControl(0); + additive_blend.enable = true; + additive_blend.color_srcblend = additive_blend.color_destblend = + additive_blend.alpha_srcblend = additive_blend.alpha_destblend = + static_cast(Prospero::BlendFactor::kOne); + registers.SetBlendControl(0, additive_blend); + auto target_info = registers.GetRenderTarget(0).info; + target_info.blend_bypass = false; + registers.SetColorInfo(0, target_info); + registers.SetPsInControl(0x8008); + user_config.SetPrimitiveType(Prospero::PrimitiveType::kTriStrip); + for (size_t i = 0; i < position_shaders.size(); i++) { + const auto &code = position_shaders[i]; + native_vertex_regs.es_regs.data_addr = reinterpret_cast(code.data()); + ShaderMapUserData(native_vertex_regs.es_regs.data_addr, + {.type = Prospero::ShaderBinaryType::kGs, + .user_data = &native_user_data, + .code_size_bytes = static_cast(code.size() * sizeof(u32))}); + const auto programs = context.GetPipelineCache().GetGraphicsPrograms( + native_vertex_regs, native_pixel_regs, registers.GetShaderRegisters(), + registers, user_config, export_mapping, true, native_vertex_info, pixel); + vertex_shader = programs.vertex[0]; + pixel_shader = programs.pixel; + vertex = native_vertex_info[0]; + draw(pipeline(true, 2, 2, false, false, vk::PrimitiveTopology::eTriangleStrip), 4); + const auto pixels = read_color(); + bool second_triangle = false; + for (size_t component = 0; component < pixels.size(); component += 4) { + Require(name, "fullscreen triangle preserved", + pixels[component] == 0x3e800000u || pixels[component] == 0x3f000000u, + "clipping-error culling removed valid fullscreen coverage"); + second_triangle |= pixels[component] == 0x3f000000u; + } + Require(name, "zero homogeneous position culling", second_triangle == (i >= 2), + "zero positions drew an extra triangle, or a valid position was culled"); + } + vertex_shader = owned_vertex_shader; pixel_shader = owned_pixel_shader; } diff --git a/tests/shaderCfgTests.cpp b/tests/shaderCfgTests.cpp index e7222e7b0..929198468 100644 --- a/tests/shaderCfgTests.cpp +++ b/tests/shaderCfgTests.cpp @@ -10106,10 +10106,10 @@ void TestNewShaderRecompilerAuxPositionExports() { std::vector({UINT32_MAX}), "MISC point-size/layer stores are missing"); Check(SpirvStoredBuiltInElements(all.spirv, 3u) == - std::vector({0u, 3u}) && + std::vector({0u, 3u, 4u}) && SpirvStoredBuiltInElements(all.spirv, 4u) == std::vector({0u, 1u, 2u}), - "partial clip/cull exports used the wrong dense elements"); + "partial clip/cull exports or the appended clipping-error plane used the wrong elements"); Check(SpirvHasDecorationValue(all.spirv, 11u, 1u) && SpirvHasDecorationValue(all.spirv, 11u, 3u) && SpirvHasDecorationValue(all.spirv, 11u, 4u) && @@ -10143,7 +10143,7 @@ void TestNewShaderRecompilerAuxPositionExports() { CheckSpirvBinaryValidates(dense.spirv); Check(SpirvStoredBuiltInElements(dense.spirv, 0u).size() == 1u && SpirvStoredBuiltInElements(dense.spirv, 3u) == - std::vector({1u}) && + std::vector({1u, 2u}) && SpirvStoredBuiltInElements(dense.spirv, 4u) == std::vector({0u}), "CCDIST1 was not densely packed into POS1"); @@ -10158,11 +10158,24 @@ void TestNewShaderRecompilerAuxPositionExports() { CheckSpirvBinaryValidates(shifted.spirv); Check(SpirvStoredBuiltInElements(shifted.spirv, 1u).size() == 1u && SpirvStoredBuiltInElements(shifted.spirv, 3u) == - std::vector({0u}) && + std::vector({0u, 1u}) && SpirvStoredBuiltInElements(shifted.spirv, 4u) == std::vector({0u}), "CCDIST1 did not shift across a disabled CCDIST0 vector"); + const uint32_t full_distances[] = { + EncodeExp0(0x0c, 0xf, false), EncodeExp1(0, 1, 2, 3), + EncodeExp0(0x0d, 0xf, false), EncodeExp1(4, 5, 6, 7), + EncodeExp0(0x0e, 0xf), EncodeExp1(8, 9, 10, 11), 0xbf810000u, + }; + const auto full = compile(full_distances, 0x00c0f00fu); + CheckSpirvBinaryValidates(full.spirv); + Check(SpirvStoredBuiltInElements(full.spirv, 3u) == + std::vector({0u, 1u, 2u, 3u}) && + SpirvStoredBuiltInElements(full.spirv, 4u) == + std::vector({0u, 1u, 2u, 3u}), + "clipping-error plane exceeded eight combined components or replaced a guest distance"); + const uint32_t unmapped[] = { EncodeExp0(0x0d, 0x4, false), EncodeExp1(0, 0, 8, 0), EncodeExp0(0x0c, 0xf), EncodeExp1(0, 1, 2, 3), @@ -13035,7 +13048,8 @@ void TestRepeatedExportsHaveOneInterface() { } offset += result.spirv[offset] >> spv::WordCountShift; } - Check(outputs == 1, "repeated exports produced duplicate output variables"); + Check(outputs == 2 && SpirvHasDecorationValue(result.spirv, 11u, 3u), + "repeated exports duplicated Position or its clipping-error plane"); } void TestNewShaderRecompilerSpirvSizeBaselines() { @@ -13251,10 +13265,10 @@ void TestNewShaderRecompilerSpirvSizeBaselines() { }; const auto wqm_result = compile("wqm", wqm, {.words = 407, - .instructions = 98, + .instructions = 99, .variables = 4, .loads = 3, - .stores = 1, + .stores = 2, .labels = 6, .selection_merges = 1, .branches = 4,