Vulkan: fix handling of inactive atomic counters

src/compiler.gni

@@ -158,6 +158,8 @@ angle_translator_sources = [
   "src/compiler/translator/tree_ops/RemoveInvariantDeclaration.h",
   "src/compiler/translator/tree_ops/RemovePow.cpp",
   "src/compiler/translator/tree_ops/RemovePow.h",
+  "src/compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.cpp",
+  "src/compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.h",
   "src/compiler/translator/tree_ops/RemoveUnreferencedVariables.cpp",
   "src/compiler/translator/tree_ops/RemoveUnreferencedVariables.h",
   "src/compiler/translator/tree_ops/RewriteAtomicCounters.cpp",

src/compiler/translator/TranslatorVulkan.cpp

@@ -19,6 +19,7 @@
 #include "compiler/translator/StaticType.h"
 #include "compiler/translator/tree_ops/NameEmbeddedUniformStructs.h"
 #include "compiler/translator/tree_ops/NameNamelessUniformBuffers.h"
+#include "compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.h"
 #include "compiler/translator/tree_ops/RewriteAtomicCounters.h"
 #include "compiler/translator/tree_ops/RewriteCubeMapSamplersAs2DArray.h"
 #include "compiler/translator/tree_ops/RewriteDfdy.h"
@@ -712,12 +713,20 @@ bool TranslatorVulkan::translateImpl(TIntermBlock *root,
             ++aggregateTypesUsedForUniforms;
         }
 
-        if (gl::IsAtomicCounterType(uniform.type))
+        if (uniform.active && gl::IsAtomicCounterType(uniform.type))
         {
             ++atomicCounterCount;
         }
     }
 
+    // Remove declarations of inactive shader interface variables so glslang wrapper doesn't need to
+    // replace them.  Note: this is done before extracting samplers from structs, as removing such
+    // inactive samplers is not yet supported.
+    if (!RemoveInactiveInterfaceVariables(this, root, getUniforms(), getInterfaceBlocks()))
+    {
+        return false;
+    }
+
     // TODO(lucferron): Refactor this function to do fewer tree traversals.
     // http://anglebug.com/2461
     if (aggregateTypesUsedForUniforms > 0)

src/compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.cpp

+//
+// Copyright 2019 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// RemoveInactiveInterfaceVariables.h:
+//  Drop shader interface variable declarations for those that are inactive.
+//
+
+#include "compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.h"
+
+#include "compiler/translator/SymbolTable.h"
+#include "compiler/translator/tree_util/IntermTraverse.h"
+#include "compiler/translator/util.h"
+
+namespace sh
+{
+
+namespace
+{
+
+// Traverser that removes all declarations that correspond to inactive variables.
+class RemoveInactiveInterfaceVariablesTraverser : public TIntermTraverser
+{
+  public:
+    RemoveInactiveInterfaceVariablesTraverser(
+        const std::vector<sh::ShaderVariable> &uniforms,
+        const std::vector<sh::InterfaceBlock> &interfaceBlocks);
+
+    bool visitDeclaration(Visit visit, TIntermDeclaration *node) override;
+
+  private:
+    const std::vector<sh::ShaderVariable> &mUniforms;
+    const std::vector<sh::InterfaceBlock> &mInterfaceBlocks;
+};
+
+RemoveInactiveInterfaceVariablesTraverser::RemoveInactiveInterfaceVariablesTraverser(
+    const std::vector<sh::ShaderVariable> &uniforms,
+    const std::vector<sh::InterfaceBlock> &interfaceBlocks)
+    : TIntermTraverser(true, false, false), mUniforms(uniforms), mInterfaceBlocks(interfaceBlocks)
+{}
+
+template <typename Variable>
+bool isVariableActive(const std::vector<Variable> &mVars, const ImmutableString &name)
+{
+    for (const Variable &var : mVars)
+    {
+        if (name == var.name)
+        {
+            return var.active;
+        }
+    }
+    UNREACHABLE();
+    return true;
+}
+
+bool RemoveInactiveInterfaceVariablesTraverser::visitDeclaration(Visit visit,
+                                                                 TIntermDeclaration *node)
+{
+    // SeparateDeclarations should have already been run.
+    ASSERT(node->getSequence()->size() == 1u);
+
+    TIntermTyped *declarator = node->getSequence()->front()->getAsTyped();
+    ASSERT(declarator);
+
+    TIntermSymbol *asSymbol = declarator->getAsSymbolNode();
+    if (!asSymbol)
+    {
+        return false;
+    }
+
+    const TType &type = declarator->getType();
+
+    // Only remove opaque uniform and interface block declarations.
+    //
+    // Note: Don't remove varyings.  Imagine a situation where the VS doesn't write to a varying
+    // but the FS reads from it.  This 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.
+    bool removeDeclaration = false;
+
+    if (type.isInterfaceBlock())
+    {
+        removeDeclaration = !isVariableActive(mInterfaceBlocks, type.getInterfaceBlock()->name());
+    }
+    else if (type.getQualifier() == EvqUniform && IsOpaqueType(type.getBasicType()))
+    {
+        removeDeclaration = !isVariableActive(mUniforms, asSymbol->getName());
+    }
+
+    if (removeDeclaration)
+    {
+        TIntermSequence emptySequence;
+        mMultiReplacements.emplace_back(getParentNode()->getAsBlock(), node, emptySequence);
+    }
+
+    return false;
+}
+
+}  // namespace
+
+bool RemoveInactiveInterfaceVariables(TCompiler *compiler,
+                                      TIntermBlock *root,
+                                      const std::vector<sh::ShaderVariable> &uniforms,
+                                      const std::vector<sh::InterfaceBlock> &interfaceBlocks)
+{
+    RemoveInactiveInterfaceVariablesTraverser traverser(uniforms, interfaceBlocks);
+    root->traverse(&traverser);
+    return traverser.updateTree(compiler, root);
+}
+
+}  // namespace sh

src/compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.h

+//
+// Copyright 2019 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// RemoveInactiveInterfaceVariables.h:
+//  Drop shader interface variable declarations for those that are inactive.  This step needs to be
+//  done after CollectVariables.  This avoids having to emulate them (e.g. atomic counters for
+//  Vulkan) or remove them in glslang wrapper (again, for Vulkan).
+//
+//  Shouldn't be run for the GL backend, as it queries shader interface variables from GL itself,
+//  instead of relying on what was gathered during CollectVariables.
+//
+
+#ifndef COMPILER_TRANSLATOR_TREEOPS_REMOVEINACTIVEVARIABLES_H_
+#define COMPILER_TRANSLATOR_TREEOPS_REMOVEINACTIVEVARIABLES_H_
+
+#include "common/angleutils.h"
+
+namespace sh
+{
+
+struct InterfaceBlock;
+struct ShaderVariable;
+class TCompiler;
+class TIntermBlock;
+class TSymbolTable;
+
+ANGLE_NO_DISCARD bool RemoveInactiveInterfaceVariables(
+    TCompiler *compiler,
+    TIntermBlock *root,
+    const std::vector<sh::ShaderVariable> &uniforms,
+    const std::vector<sh::InterfaceBlock> &interfaceBlocks);
+
+}  // namespace sh
+
+#endif  // COMPILER_TRANSLATOR_TREEOPS_REMOVEINACTIVEVARIABLES_H_

src/tests/compiler_tests/ShaderImage_test.cpp

@@ -243,6 +243,9 @@ TEST_F(ShaderImageTest, ImageMemoryQualifiers)
         "layout(rgba32f) uniform highp volatile writeonly image2D image2;\n"
         "layout(rgba32f) uniform highp volatile restrict readonly writeonly image2D image3;\n"
         "void main() {\n"
+        "    imageSize(image1);\n"
+        "    imageSize(image2);\n"
+        "    imageSize(image3);\n"
         "}";
     if (!compile(shaderString))
     {

src/tests/deqp_support/deqp_khr_gles31_test_expectations.txt

@@ -46,9 +46,9 @@
 3570 VULKAN : KHR-GLES31.core.compute_shader.sso-case3 = SKIP
 4188 VULKAN : KHR-GLES31.core.compute_shader.resource-image = FAIL
 4189 VULKAN : KHR-GLES31.core.compute_shader.shared-struct = SKIP
-4190 VULKAN : KHR-GLES31.core.compute_shader.pipeline-compute-chain = SKIP
 4191 VULKAN : KHR-GLES31.core.compute_shader.resources-max = FAIL
 4192 VULKAN : KHR-GLES31.core.compute_shader.pipeline-post-xfb = FAIL
+3726 VULKAN PIXEL2ORXL : KHR-GLES31.core.compute_shader.pipeline-compute-chain = FAIL
 4194 VULKAN PIXEL2ORXL : KHR-GLES31.core.compute_shader.resource-ubo = FAIL
 4194 VULKAN PIXEL2ORXL : KHR-GLES31.core.compute_shader.built-in-variables = FAIL
 

src/tests/gl_tests/GLSLTest.cpp

@@ -2982,6 +2982,62 @@ void main() {
     glUnmapBuffer(GL_SHADER_STORAGE_BUFFER);
 }
 
+// Test that inactive images don't cause any errors.
+TEST_P(GLSLTest_ES31, InactiveImages)
+{
+    ANGLE_SKIP_TEST_IF(IsD3D11());
+
+    constexpr char kCS[] = R"(#version 310 es
+layout(local_size_x=1, local_size_y=1, local_size_z=1) in;
+layout(rgba32ui) uniform highp readonly uimage2D image1;
+layout(rgba32ui) uniform highp readonly uimage2D image2[4];
+void main()
+{
+})";
+
+    ANGLE_GL_COMPUTE_PROGRAM(program, kCS);
+
+    glUseProgram(program.get());
+    glDispatchCompute(1, 1, 1);
+    EXPECT_GL_NO_ERROR();
+
+    // Verify that the images are indeed inactive.
+    GLuint index = glGetProgramResourceIndex(program, GL_UNIFORM, "image1");
+    EXPECT_GL_NO_ERROR();
+    EXPECT_EQ(GL_INVALID_INDEX, index);
+
+    index = glGetProgramResourceIndex(program, GL_UNIFORM, "image2");
+    EXPECT_GL_NO_ERROR();
+    EXPECT_EQ(GL_INVALID_INDEX, index);
+}
+
+// Test that inactive atomic counters don't cause any errors.
+TEST_P(GLSLTest_ES31, InactiveAtomicCounters)
+{
+    constexpr char kCS[] = R"(#version 310 es
+layout(local_size_x=1, local_size_y=1, local_size_z=1) in;
+layout(binding = 0, offset = 0) uniform atomic_uint ac1;
+layout(binding = 0, offset = 4) uniform atomic_uint ac2[5];
+void main()
+{
+})";
+
+    ANGLE_GL_COMPUTE_PROGRAM(program, kCS);
+
+    glUseProgram(program.get());
+    glDispatchCompute(1, 1, 1);
+    EXPECT_GL_NO_ERROR();
+
+    // Verify that the atomic counters are indeed inactive.
+    GLuint index = glGetProgramResourceIndex(program, GL_UNIFORM, "ac1");
+    EXPECT_GL_NO_ERROR();
+    EXPECT_EQ(GL_INVALID_INDEX, index);
+
+    index = glGetProgramResourceIndex(program, GL_UNIFORM, "ac2");
+    EXPECT_GL_NO_ERROR();
+    EXPECT_EQ(GL_INVALID_INDEX, index);
+}
+
 // Test that array indices for arrays of arrays of basic types work as expected.
 TEST_P(GLSLTest_ES31, ArraysOfArraysBasicType)
 {