Set writemask to 0 when no fs output matches color state on Metal and Vulkan

This patch fixes an undefined behaviour on Metal and Vulkan when there
is a color state whose corresponding fragment output is not declared in
the fragment shader.

According to Vulkan SPEC (Chapter 14.3), the input values to blending or
color attachment writes are undefined for components which do not
correspond to a fragment shader output. Vulkan validation layer follows
the SPEC that it only allows the shader to not produce a matching output
if the writemask is 0, or it will report a warning when the application
is against this rule.

When no fragment output matches the color state in a render pipeline,
the output differs on different Metal devices. On some Metal devices the
fragment output will be (0, 0, 0, 0) even if it is not declared in the
shader, while on others there will be no fragment outputs and the content
in the color attachments is not changed.

This patch fixes this issue by setting the color write mask to 0 to
prevent the undefined values being written into the color attachments.

With this patch, the following end2end tests will not report warnings
any more when we enable the Vulkan validation layer:
ObjectCachingTest.RenderPipelineDeduplicationOnLayout/Vulkan
ObjectCachingTest.RenderPipelineDeduplicationOnVertexModule/Vulkan
ObjectCachingTest.RenderPipelineDeduplicationOnFragmentModule/Vulkan

BUG=dawn:209
TEST=dawn_end2end_tests

Change-Id: I5613daa1b9a45349ea1459fbdfe4a12d6149f0f7
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/11581
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Jiawei Shao <jiawei.shao@intel.com>

src/dawn_native/metal/RenderPipelineMTL.mm

@@ -182,7 +182,12 @@ namespace dawn_native { namespace metal {
             }
         }
 
-        MTLColorWriteMask MetalColorWriteMask(dawn::ColorWriteMask writeMask) {
+        MTLColorWriteMask MetalColorWriteMask(dawn::ColorWriteMask writeMask,
+                                              bool isDeclaredInFragmentShader) {
+            if (!isDeclaredInFragmentShader) {
+                return MTLColorWriteMaskNone;
+            }
+
             MTLColorWriteMask mask = MTLColorWriteMaskNone;
 
             if (writeMask & dawn::ColorWriteMask::Red) {
@@ -202,7 +207,8 @@ namespace dawn_native { namespace metal {
         }
 
         void ComputeBlendDesc(MTLRenderPipelineColorAttachmentDescriptor* attachment,
-                              const ColorStateDescriptor* descriptor) {
+                              const ColorStateDescriptor* descriptor,
+                              bool isDeclaredInFragmentShader) {
             attachment.blendingEnabled = BlendEnabled(descriptor);
             attachment.sourceRGBBlendFactor =
                 MetalBlendFactor(descriptor->colorBlend.srcFactor, false);
@@ -214,7 +220,8 @@ namespace dawn_native { namespace metal {
             attachment.destinationAlphaBlendFactor =
                 MetalBlendFactor(descriptor->alphaBlend.dstFactor, true);
             attachment.alphaBlendOperation = MetalBlendOperation(descriptor->alphaBlend.operation);
-            attachment.writeMask = MetalColorWriteMask(descriptor->writeMask);
+            attachment.writeMask =
+                MetalColorWriteMask(descriptor->writeMask, isDeclaredInFragmentShader);
         }
 
         MTLStencilOperation MetalStencilOperation(dawn::StencilOperation stencilOperation) {
@@ -339,11 +346,15 @@ namespace dawn_native { namespace metal {
             descriptorMTL.stencilAttachmentPixelFormat = MetalPixelFormat(depthStencilFormat);
         }
 
+        const ShaderModuleBase::FragmentOutputBaseTypes& fragmentOutputBaseTypes =
+            descriptor->fragmentStage->module->GetFragmentOutputBaseTypes();
         for (uint32_t i : IterateBitSet(GetColorAttachmentsMask())) {
             descriptorMTL.colorAttachments[i].pixelFormat =
                 MetalPixelFormat(GetColorAttachmentFormat(i));
             const ColorStateDescriptor* descriptor = GetColorStateDescriptor(i);
-            ComputeBlendDesc(descriptorMTL.colorAttachments[i], descriptor);
+            bool isDeclaredInFragmentShader = fragmentOutputBaseTypes[i] != Format::Other;
+            ComputeBlendDesc(descriptorMTL.colorAttachments[i], descriptor,
+                             isDeclaredInFragmentShader);
         }
 
         descriptorMTL.inputPrimitiveTopology = MTLInputPrimitiveTopology(GetPrimitiveTopology());

src/dawn_native/vulkan/RenderPipelineVk.cpp

@@ -207,7 +207,8 @@ namespace dawn_native { namespace vulkan {
             }
         }
 
-        VkColorComponentFlagBits VulkanColorWriteMask(dawn::ColorWriteMask mask) {
+        VkColorComponentFlags VulkanColorWriteMask(dawn::ColorWriteMask mask,
+                                                   bool isDeclaredInFragmentShader) {
             // Vulkan and Dawn color write masks match, static assert it and return the mask
             static_assert(static_cast<VkColorComponentFlagBits>(dawn::ColorWriteMask::Red) ==
                               VK_COLOR_COMPONENT_R_BIT,
@@ -222,11 +223,16 @@ namespace dawn_native { namespace vulkan {
                               VK_COLOR_COMPONENT_A_BIT,
                           "");
 
-            return static_cast<VkColorComponentFlagBits>(mask);
+            // According to Vulkan SPEC (Chapter 14.3): "The input values to blending or color
+            // attachment writes are undefined for components which do not correspond to a fragment
+            // shader outputs", we set the color write mask to 0 to prevent such undefined values
+            // being written into the color attachments.
+            return isDeclaredInFragmentShader ? static_cast<VkColorComponentFlags>(mask)
+                                              : static_cast<VkColorComponentFlags>(0);
         }
 
-        VkPipelineColorBlendAttachmentState ComputeColorDesc(
+        VkPipelineColorBlendAttachmentState ComputeColorDesc(const ColorStateDescriptor* descriptor,
-            const ColorStateDescriptor* descriptor) {
+                                                             bool isDeclaredInFragmentShader) {
             VkPipelineColorBlendAttachmentState attachment;
             attachment.blendEnable = BlendEnabled(descriptor) ? VK_TRUE : VK_FALSE;
             attachment.srcColorBlendFactor = VulkanBlendFactor(descriptor->colorBlend.srcFactor);
@@ -235,7 +241,8 @@ namespace dawn_native { namespace vulkan {
             attachment.srcAlphaBlendFactor = VulkanBlendFactor(descriptor->alphaBlend.srcFactor);
             attachment.dstAlphaBlendFactor = VulkanBlendFactor(descriptor->alphaBlend.dstFactor);
             attachment.alphaBlendOp = VulkanBlendOperation(descriptor->alphaBlend.operation);
-            attachment.colorWriteMask = VulkanColorWriteMask(descriptor->writeMask);
+            attachment.colorWriteMask =
+                VulkanColorWriteMask(descriptor->writeMask, isDeclaredInFragmentShader);
             return attachment;
         }
 
@@ -400,9 +407,13 @@ namespace dawn_native { namespace vulkan {
         // Initialize the "blend state info" that will be chained in the "create info" from the data
         // pre-computed in the ColorState
         std::array<VkPipelineColorBlendAttachmentState, kMaxColorAttachments> colorBlendAttachments;
+        const ShaderModuleBase::FragmentOutputBaseTypes& fragmentOutputBaseTypes =
+            descriptor->fragmentStage->module->GetFragmentOutputBaseTypes();
         for (uint32_t i : IterateBitSet(GetColorAttachmentsMask())) {
-            const ColorStateDescriptor* descriptor = GetColorStateDescriptor(i);
+            const ColorStateDescriptor* colorStateDescriptor = GetColorStateDescriptor(i);
-            colorBlendAttachments[i] = ComputeColorDesc(descriptor);
+            bool isDeclaredInFragmentShader = fragmentOutputBaseTypes[i] != Format::Other;
+            colorBlendAttachments[i] =
+                ComputeColorDesc(colorStateDescriptor, isDeclaredInFragmentShader);
         }
         VkPipelineColorBlendStateCreateInfo colorBlend;
         colorBlend.sType = VK_STRUCTURE_TYPE_PIPELINE_COLOR_BLEND_STATE_CREATE_INFO;

src/tests/end2end/RenderPassTests.cpp

@@ -26,8 +26,7 @@ protected:
         DawnTest::SetUp();
 
         // Shaders to draw a bottom-left triangle in blue.
-        dawn::ShaderModule vsModule =
+        mVSModule = utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, R"(
-            utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, R"(
                 #version 450
                 void main() {
                     const vec2 pos[3] = vec2[3](
@@ -44,7 +43,7 @@ protected:
                 })");
 
         utils::ComboRenderPipelineDescriptor descriptor(device);
-        descriptor.vertexStage.module = vsModule;
+        descriptor.vertexStage.module = mVSModule;
         descriptor.cFragmentStage.module = fsModule;
         descriptor.primitiveTopology = dawn::PrimitiveTopology::TriangleStrip;
         descriptor.cColorStates[0].format = kFormat;
@@ -66,6 +65,7 @@ protected:
         return device.CreateTexture(&descriptor);
     }
 
+    dawn::ShaderModule mVSModule;
     dawn::RenderPipeline pipeline;
 };
 
@@ -119,4 +119,56 @@ TEST_P(RenderPassTest, TwoRenderPassesInOneCommandBuffer) {
     EXPECT_PIXEL_RGBA8_EQ(kGreen, renderTarget2, kRTSize - 1, 1);
 }
 
+// Verify that the content in the color attachment will not be changed if there is no corresponding
+// fragment shader outputs in the render pipeline, the load operation is LoadOp::Load and the store
+// operation is StoreOp::Store.
+TEST_P(RenderPassTest, NoCorrespondingFragmentShaderOutputs) {
+    dawn::Texture renderTarget = CreateDefault2DTexture();
+    dawn::CommandEncoder encoder = device.CreateCommandEncoder();
+
+    dawn::TextureView renderTargetView = renderTarget.CreateView();
+
+    utils::ComboRenderPassDescriptor renderPass({renderTargetView});
+    renderPass.cColorAttachments[0].clearColor = {1.0f, 0.0f, 0.0f, 1.0f};
+    renderPass.cColorAttachments[0].loadOp = dawn::LoadOp::Clear;
+    renderPass.cColorAttachments[0].storeOp = dawn::StoreOp::Store;
+    dawn::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass);
+
+    {
+        // First we draw a blue triangle in the bottom left of renderTarget.
+        pass.SetPipeline(pipeline);
+        pass.Draw(3, 1, 0, 0);
+    }
+
+    {
+        // Next we use a pipeline whose fragment shader has no outputs.
+        dawn::ShaderModule fsModule =
+            utils::CreateShaderModule(device, utils::SingleShaderStage::Fragment, R"(
+                #version 450
+                void main() {
+                })");
+        utils::ComboRenderPipelineDescriptor descriptor(device);
+        descriptor.vertexStage.module = mVSModule;
+        descriptor.cFragmentStage.module = fsModule;
+        descriptor.primitiveTopology = dawn::PrimitiveTopology::TriangleStrip;
+        descriptor.cColorStates[0].format = kFormat;
+
+        dawn::RenderPipeline pipelineWithNoFragmentOutput =
+            device.CreateRenderPipeline(&descriptor);
+
+        pass.SetPipeline(pipelineWithNoFragmentOutput);
+        pass.Draw(3, 1, 0, 0);
+    }
+
+    pass.EndPass();
+
+    dawn::CommandBuffer commands = encoder.Finish();
+    queue.Submit(1, &commands);
+
+    constexpr RGBA8 kRed(255, 0, 0, 255);
+    constexpr RGBA8 kBlue(0, 0, 255, 255);
+    EXPECT_PIXEL_RGBA8_EQ(kBlue, renderTarget, 2, kRTSize - 1);
+    EXPECT_PIXEL_RGBA8_EQ(kRed, renderTarget, kRTSize - 1, 1);
+}
+
 DAWN_INSTANTIATE_TEST(RenderPassTest, D3D12Backend, MetalBackend, OpenGLBackend, VulkanBackend);