Vulkan: Fix handling of inactive fragment outputs

src/compiler/translator/TranslatorVulkan.cpp

@@ -734,7 +734,8 @@ bool TranslatorVulkan::translateImpl(TIntermBlock *root,
     // every field of an active uniform that's of struct type as active, i.e. no extracted sampler
     // is inactive.
     if (!RemoveInactiveInterfaceVariables(this, root, getAttributes(), getInputVaryings(),
-                                          getUniforms(), getInterfaceBlocks()))
+                                          getOutputVariables(), getUniforms(),
+                                          getInterfaceBlocks()))
     {
         return false;
     }

src/compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.cpp

@@ -26,6 +26,7 @@ class RemoveInactiveInterfaceVariablesTraverser : public TIntermTraverser
     RemoveInactiveInterfaceVariablesTraverser(
         const std::vector<sh::ShaderVariable> &attributes,
         const std::vector<sh::ShaderVariable> &inputVaryings,
+        const std::vector<sh::ShaderVariable> &outputVariables,
         const std::vector<sh::ShaderVariable> &uniforms,
         const std::vector<sh::InterfaceBlock> &interfaceBlocks);
 
@@ -34,6 +35,7 @@ class RemoveInactiveInterfaceVariablesTraverser : public TIntermTraverser
   private:
     const std::vector<sh::ShaderVariable> &mAttributes;
     const std::vector<sh::ShaderVariable> &mInputVaryings;
+    const std::vector<sh::ShaderVariable> &mOutputVariables;
     const std::vector<sh::ShaderVariable> &mUniforms;
     const std::vector<sh::InterfaceBlock> &mInterfaceBlocks;
 };
@@ -41,11 +43,13 @@ class RemoveInactiveInterfaceVariablesTraverser : public TIntermTraverser
 RemoveInactiveInterfaceVariablesTraverser::RemoveInactiveInterfaceVariablesTraverser(
     const std::vector<sh::ShaderVariable> &attributes,
     const std::vector<sh::ShaderVariable> &inputVaryings,
+    const std::vector<sh::ShaderVariable> &outputVariables,
     const std::vector<sh::ShaderVariable> &uniforms,
     const std::vector<sh::InterfaceBlock> &interfaceBlocks)
     : TIntermTraverser(true, false, false),
       mAttributes(attributes),
       mInputVaryings(inputVaryings),
+      mOutputVariables(outputVariables),
       mUniforms(uniforms),
       mInterfaceBlocks(interfaceBlocks)
 {}
@@ -88,6 +92,8 @@ bool RemoveInactiveInterfaceVariablesTraverser::visitDeclaration(Visit visit,
     // is allowed, though the value of the varying is undefined.  If the varying is removed here,
     // the situation is changed to VS not declaring the varying, but the FS reading from it, which
     // is not allowed.  That's why inactive shader outputs are not removed.
+    //
+    // Inactive fragment shader outputs can be removed though, as there is no next stage.
     bool removeDeclaration     = false;
     const TQualifier qualifier = type.getQualifier();
 
@@ -107,6 +113,10 @@ bool RemoveInactiveInterfaceVariablesTraverser::visitDeclaration(Visit visit,
     {
         removeDeclaration = !IsVariableActive(mInputVaryings, asSymbol->getName());
     }
+    else if (qualifier == EvqFragmentOut)
+    {
+        removeDeclaration = !IsVariableActive(mOutputVariables, asSymbol->getName());
+    }
 
     if (removeDeclaration)
     {
@@ -123,11 +133,12 @@ bool RemoveInactiveInterfaceVariables(TCompiler *compiler,
                                       TIntermBlock *root,
                                       const std::vector<sh::ShaderVariable> &attributes,
                                       const std::vector<sh::ShaderVariable> &inputVaryings,
+                                      const std::vector<sh::ShaderVariable> &outputVariables,
                                       const std::vector<sh::ShaderVariable> &uniforms,
                                       const std::vector<sh::InterfaceBlock> &interfaceBlocks)
 {
-    RemoveInactiveInterfaceVariablesTraverser traverser(attributes, inputVaryings, uniforms,
-                                                        interfaceBlocks);
+    RemoveInactiveInterfaceVariablesTraverser traverser(attributes, inputVaryings, outputVariables,
+                                                        uniforms, interfaceBlocks);
     root->traverse(&traverser);
     return traverser.updateTree(compiler, root);
 }

src/compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.h

@@ -31,6 +31,7 @@ ANGLE_NO_DISCARD bool RemoveInactiveInterfaceVariables(
     TIntermBlock *root,
     const std::vector<sh::ShaderVariable> &attributes,
     const std::vector<sh::ShaderVariable> &inputVaryings,
+    const std::vector<sh::ShaderVariable> &outputVariables,
     const std::vector<sh::ShaderVariable> &uniforms,
     const std::vector<sh::InterfaceBlock> &interfaceBlocks);
 

src/tests/deqp_support/deqp_khr_gles31_test_expectations.txt

@@ -61,7 +61,6 @@
 // Image related failures
 
 4312 VULKAN : KHR-GLES31.core.shader_image_load_store.advanced-sync-imageAccess = SKIP
-4313 VULKAN : KHR-GLES31.core.shader_image_load_store.advanced-sync-imageAccess2 = SKIP
 4315 VULKAN : KHR-GLES31.core.shader_image_load_store.advanced-memory-order-vsfs = FAIL
 4108 VULKAN : KHR-GLES31.core.shader_image_size.*-nonMS-* = SKIP
 

src/tests/gl_tests/FramebufferTest.cpp

@@ -362,10 +362,6 @@ TEST_P(FramebufferFormatsTest, ReadDrawCompleteness)
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::red);
 }
 
-// Use this to select which configurations (e.g. which renderer, which GLES major version) these
-// tests should be run against.
-ANGLE_INSTANTIATE_TEST_ES2_AND_ES3(FramebufferFormatsTest);
-
 class FramebufferTest_ES3 : public ANGLETest
 {};
 
@@ -818,7 +814,61 @@ TEST_P(FramebufferTest_ES3, ResizeTextureSmallToLarge)
     EXPECT_PIXEL_COLOR_EQ(getWindowWidth() - 1, getWindowHeight() - 1, GLColor::green);
 }
 
-ANGLE_INSTANTIATE_TEST_ES3(FramebufferTest_ES3);
+// Test that fewer outputs than framebuffer attachments doesn't crash.  This causes a Vulkan
+// validation warning, but should not be fatal.
+TEST_P(FramebufferTest_ES3, FewerShaderOutputsThanAttachments)
+{
+    constexpr char kFS[] = R"(#version 300 es
+precision highp float;
+
+layout(location = 0) out vec4 color0;
+layout(location = 1) out vec4 color1;
+layout(location = 2) out vec4 color2;
+
+void main()
+{
+    color0 = vec4(1.0, 0.0, 0.0, 1.0);
+    color1 = vec4(0.0, 1.0, 0.0, 1.0);
+    color2 = vec4(0.0, 0.0, 1.0, 1.0);
+}
+)";
+
+    ANGLE_GL_PROGRAM(program, essl3_shaders::vs::Simple(), kFS);
+
+    constexpr GLint kDrawBufferCount = 4;
+
+    GLint maxDrawBuffers;
+    glGetIntegerv(GL_MAX_DRAW_BUFFERS, &maxDrawBuffers);
+    ASSERT_GE(maxDrawBuffers, kDrawBufferCount);
+
+    GLTexture textures[kDrawBufferCount];
+
+    for (GLint texIndex = 0; texIndex < kDrawBufferCount; ++texIndex)
+    {
+        glBindTexture(GL_TEXTURE_2D, textures[texIndex]);
+        glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, getWindowWidth(), getWindowHeight(), 0, GL_RGBA,
+                     GL_UNSIGNED_BYTE, nullptr);
+    }
+
+    GLenum allBufs[kDrawBufferCount] = {GL_COLOR_ATTACHMENT0, GL_COLOR_ATTACHMENT1,
+                                        GL_COLOR_ATTACHMENT2, GL_COLOR_ATTACHMENT3};
+
+    GLFramebuffer fbo;
+    glBindFramebuffer(GL_DRAW_FRAMEBUFFER, fbo);
+
+    // Enable all draw buffers.
+    for (GLint texIndex = 0; texIndex < kDrawBufferCount; ++texIndex)
+    {
+        glBindTexture(GL_TEXTURE_2D, textures[texIndex]);
+        glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0 + texIndex, GL_TEXTURE_2D,
+                               textures[texIndex], 0);
+    }
+    glDrawBuffers(kDrawBufferCount, allBufs);
+
+    // Draw with simple program.
+    drawQuad(program, essl3_shaders::PositionAttrib(), 0.5f, 1.0f, true);
+    ASSERT_GL_NO_ERROR();
+}
 
 class FramebufferTest_ES31 : public ANGLETest
 {
@@ -1124,8 +1174,6 @@ void main()
     ASSERT_GL_NO_ERROR();
 }
 
-ANGLE_INSTANTIATE_TEST_ES31(FramebufferTest_ES31);
-
 class AddDummyTextureNoRenderTargetTest : public ANGLETest
 {
   public:
@@ -1161,3 +1209,6 @@ TEST_P(AddDummyTextureNoRenderTargetTest, NoProgramOutputWorkaround)
 }
 
 ANGLE_INSTANTIATE_TEST_ES2(AddDummyTextureNoRenderTargetTest);
+ANGLE_INSTANTIATE_TEST_ES2_AND_ES3(FramebufferFormatsTest);
+ANGLE_INSTANTIATE_TEST_ES3(FramebufferTest_ES3);
+ANGLE_INSTANTIATE_TEST_ES31(FramebufferTest_ES31);

src/tests/gl_tests/GLSLTest.cpp

@@ -616,6 +616,61 @@ void main()
     verifyAttachment2DColor(3, textures[3], GL_TEXTURE_2D, 0, GLColor::white);
 }
 
+// Test that inactive fragment shader outputs don't cause a crash.
+TEST_P(GLSLTest_ES3, InactiveFragmentShaderOutput)
+{
+    constexpr char kFS[] = R"(#version 300 es
+precision highp float;
+
+// Make color0 inactive but specify color1 first.  The Vulkan backend assigns bogus locations when
+// compiling and fixes it up in SPIR-V.  If color0's location is not fixed, it will return location
+// 1 (aliasing color1).  This will lead to a Vulkan validation warning about attachment 0 not being
+// written to, which shouldn't be fatal.
+layout(location = 1) out vec4 color1;
+layout(location = 0) out vec4 color0;
+
+void main()
+{
+    color1 = vec4(0.0, 1.0, 0.0, 1.0);
+}
+)";
+
+    ANGLE_GL_PROGRAM(program, essl3_shaders::vs::Simple(), kFS);
+
+    constexpr GLint kDrawBufferCount = 2;
+
+    GLint maxDrawBuffers;
+    glGetIntegerv(GL_MAX_DRAW_BUFFERS, &maxDrawBuffers);
+    ASSERT_GE(maxDrawBuffers, kDrawBufferCount);
+
+    GLTexture textures[kDrawBufferCount];
+
+    for (GLint texIndex = 0; texIndex < kDrawBufferCount; ++texIndex)
+    {
+        glBindTexture(GL_TEXTURE_2D, textures[texIndex]);
+        glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, getWindowWidth(), getWindowHeight(), 0, GL_RGBA,
+                     GL_UNSIGNED_BYTE, nullptr);
+    }
+
+    GLenum allBufs[kDrawBufferCount] = {GL_COLOR_ATTACHMENT0, GL_COLOR_ATTACHMENT1};
+
+    GLFramebuffer fbo;
+    glBindFramebuffer(GL_DRAW_FRAMEBUFFER, fbo);
+
+    // Enable all draw buffers.
+    for (GLint texIndex = 0; texIndex < kDrawBufferCount; ++texIndex)
+    {
+        glBindTexture(GL_TEXTURE_2D, textures[texIndex]);
+        glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0 + texIndex, GL_TEXTURE_2D,
+                               textures[texIndex], 0);
+    }
+    glDrawBuffers(kDrawBufferCount, allBufs);
+
+    // Draw with simple program.
+    drawQuad(program, essl3_shaders::PositionAttrib(), 0.5f, 1.0f, true);
+    ASSERT_GL_NO_ERROR();
+}
+
 TEST_P(GLSLTest, ScopedStructsOrderBug)
 {
     // TODO(geofflang): Find out why this doesn't compile on Apple OpenGL drivers