Cull zero homogeneous vertex positions before rasterization

Fix Playroom top right artifact on Nvidia by clipping zero-position vertices.

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);

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();

src/graphics/host_gpu/renderer/pipeline/pipelineCache.cpp

@@ -89,12 +89,7 @@ template <typename... Args>
 void PipelineCacheLog(fmt::format_string<Args...> format, Args&&... args) {
 	auto message = fmt::format(format, std::forward<Args>(args)...);
 	message += '\n';
-	if (Log::GetDirection() != Log::Direction::Console) {
+	Log::WriteToConsoleAndLog(message);
-		std::fwrite(message.data(), 1, message.size(), stdout);
-		std::fflush(stdout);
-	}
-	Log::Write(message);
-	Log::Flush();
 }
 
 bool ReadShaderGuestMemory(void*, uint64_t address, uint32_t* value) {

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 <algorithm>
+#include <atomic>
 
 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,

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;

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) {

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<std::array<u32, 4>, 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<std::vector<u32>, 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<u32, 4> 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<uint8_t>(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<uint64_t>(code.data());
+        ShaderMapUserData(native_vertex_regs.es_regs.data_addr,
+            {.type = Prospero::ShaderBinaryType::kGs,
+             .user_data = &native_user_data,
+             .code_size_bytes = static_cast<uint32_t>(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;
     }

tests/shaderCfgTests.cpp

@@ -10106,10 +10106,10 @@ void TestNewShaderRecompilerAuxPositionExports() {
                 std::vector<uint32_t>({UINT32_MAX}),
         "MISC point-size/layer stores are missing");
   Check(SpirvStoredBuiltInElements(all.spirv, 3u) ==
-            std::vector<uint32_t>({0u, 3u}) &&
+            std::vector<uint32_t>({0u, 3u, 4u}) &&
             SpirvStoredBuiltInElements(all.spirv, 4u) ==
                 std::vector<uint32_t>({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<uint32_t>({1u}) &&
+                std::vector<uint32_t>({1u, 2u}) &&
             SpirvStoredBuiltInElements(dense.spirv, 4u) ==
                 std::vector<uint32_t>({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<uint32_t>({0u}) &&
+                std::vector<uint32_t>({0u, 1u}) &&
             SpirvStoredBuiltInElements(shifted.spirv, 4u) ==
                 std::vector<uint32_t>({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<uint32_t>({0u, 1u, 2u, 3u}) &&
+            SpirvStoredBuiltInElements(full.spirv, 4u) ==
+                std::vector<uint32_t>({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,