Vulkan: Remove unused atomic counter builtins

src/compiler.gni

@@ -148,6 +148,8 @@ angle_translator_sources = [
   "src/compiler/translator/tree_ops/RegenerateStructNames.h",
   "src/compiler/translator/tree_ops/RemoveArrayLengthMethod.cpp",
   "src/compiler/translator/tree_ops/RemoveArrayLengthMethod.h",
+  "src/compiler/translator/tree_ops/RemoveAtomicCounterBuiltins.cpp",
+  "src/compiler/translator/tree_ops/RemoveAtomicCounterBuiltins.h",
   "src/compiler/translator/tree_ops/RemoveDynamicIndexing.cpp",
   "src/compiler/translator/tree_ops/RemoveDynamicIndexing.h",
   "src/compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.cpp",

src/compiler/translator/TranslatorVulkan.cpp

@@ -19,6 +19,7 @@
 #include "compiler/translator/OutputVulkanGLSL.h"
 #include "compiler/translator/StaticType.h"
 #include "compiler/translator/tree_ops/NameEmbeddedUniformStructs.h"
+#include "compiler/translator/tree_ops/RemoveAtomicCounterBuiltins.h"
 #include "compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.h"
 #include "compiler/translator/tree_ops/RewriteAtomicCounters.h"
 #include "compiler/translator/tree_ops/RewriteCubeMapSamplersAs2DArray.h"
@@ -864,6 +865,16 @@ bool TranslatorVulkan::translateImpl(TIntermBlock *root,
             return false;
         }
     }
+    else if (getShaderVersion() >= 310)
+    {
+        // Vulkan doesn't support Atomic Storage as a Storage Class, but we've seen
+        // cases where builtins are using it even with no active atomic counters.
+        // This pass simply removes those builtins in that scenario.
+        if (!RemoveAtomicCounterBuiltins(this, root))
+        {
+            return false;
+        }
+    }
 
     if (getShaderType() != GL_COMPUTE_SHADER)
     {

src/compiler/translator/tree_ops/RemoveAtomicCounterBuiltins.cpp

+//
+// Copyright 2020 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.
+//
+// RemoveAtomicCounterBuiltins: Remove atomic counter builtins.
+//
+
+#include "compiler/translator/tree_ops/RemoveAtomicCounterBuiltins.h"
+
+#include "compiler/translator/Compiler.h"
+#include "compiler/translator/tree_util/IntermTraverse.h"
+
+namespace sh
+{
+namespace
+{
+
+bool IsAtomicCounterDecl(const TIntermDeclaration *node)
+{
+    const TIntermSequence &sequence = *(node->getSequence());
+    TIntermTyped *variable          = sequence.front()->getAsTyped();
+    const TType &type               = variable->getType();
+    return type.getQualifier() == EvqUniform && type.isAtomicCounter();
+}
+
+// Traverser that removes all GLSL built-ins that use AtomicCounters
+// Only called when the builtins are in use, but no atomic counters have been declared
+class RemoveAtomicCounterBuiltinsTraverser : public TIntermTraverser
+{
+  public:
+    RemoveAtomicCounterBuiltinsTraverser() : TIntermTraverser(true, false, false) {}
+
+    bool visitDeclaration(Visit visit, TIntermDeclaration *node) override
+    {
+        ASSERT(visit == PreVisit);
+
+        // Active atomic counters should have been removed by RewriteAtomicCounters, and this
+        // traversal should not have been invoked
+        ASSERT(!IsAtomicCounterDecl(node));
+        return false;
+    }
+
+    bool visitAggregate(Visit visit, TIntermAggregate *node) override
+    {
+        if (node->getOp() == EOpMemoryBarrierAtomicCounter)
+        {
+            // Vulkan does not support atomic counters, so if this builtin finds its way here,
+            // we need to remove it.
+            TIntermSequence emptySequence;
+            mMultiReplacements.emplace_back(getParentNode()->getAsBlock(), node, emptySequence);
+            return true;
+        }
+
+        // We shouldn't see any other builtins because they cannot be present without an active
+        // atomic counter, and should have been removed by RewriteAtomicCounters. If this fires,
+        // this traversal should not have been called.
+        ASSERT(!(node->getOp() == EOpCallBuiltInFunction &&
+                 node->getFunction()->isAtomicCounterFunction()));
+
+        return false;
+    }
+};
+
+}  // anonymous namespace
+
+bool RemoveAtomicCounterBuiltins(TCompiler *compiler, TIntermBlock *root)
+{
+    RemoveAtomicCounterBuiltinsTraverser traverser;
+    root->traverse(&traverser);
+    return traverser.updateTree(compiler, root);
+}
+}  // namespace sh

src/compiler/translator/tree_ops/RemoveAtomicCounterBuiltins.h

+//
+// Copyright 2020 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.
+//
+// RemoveAtomicCounterBuiltins: Remove atomic counter builtins.
+// Normally handled by RewriteAtomicCounters, but that is only invoked when
+// atomic counters are actually in use. This pass removes the builtins and
+// asserts no atomic counters are declared.
+
+#ifndef COMPILER_TRANSLATOR_TREEOPS_REMOVEATOMICCOUNTERBUILTINS_H_
+#define COMPILER_TRANSLATOR_TREEOPS_REMOVEATOMICCOUNTERBUILTINS_H_
+
+#include "common/angleutils.h"
+
+namespace sh
+{
+class TCompiler;
+class TIntermBlock;
+
+ANGLE_NO_DISCARD bool RemoveAtomicCounterBuiltins(TCompiler *compiler, TIntermBlock *root);
+}  // namespace sh
+
+#endif  // COMPILER_TRANSLATOR_TREEOPS_REMOVEATOMICCOUNTERBUILTINS_H_

src/tests/deqp_support/deqp_khr_gles31_test_expectations.txt

@@ -89,7 +89,6 @@
 
 // Dispatch indirect:
 4188 VULKAN : KHR-GLES31.core.compute_shader.resource-image = FAIL
-4189 VULKAN : KHR-GLES31.core.compute_shader.shared-struct = 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

src/tests/gl_tests/AtomicCounterBufferTest.cpp

@@ -433,6 +433,57 @@ void main()
     }
 }
 
+// Test inactive atomic counter
+TEST_P(AtomicCounterBufferTest31, AtomicCounterInactive)
+{
+    constexpr char kFS[] =
+        "#version 310 es\n"
+        "precision highp float;\n"
+
+        // This inactive atomic counter should be removed by RemoveInactiveInterfaceVariables
+        "layout(binding = 0) uniform atomic_uint inactive;\n"
+
+        "out highp vec4 my_color;\n"
+        "void main()\n"
+        "{\n"
+        "    my_color = vec4(0.0, 1.0, 0.0, 1.0);\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(program, essl31_shaders::vs::Simple(), kFS);
+    glUseProgram(program);
+
+    drawQuad(program, essl31_shaders::PositionAttrib(), 0.0f);
+    ASSERT_GL_NO_ERROR();
+
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
+// Test inactive memoryBarrierAtomicCounter
+TEST_P(AtomicCounterBufferTest31, AtomicCounterMemoryBarrier)
+{
+    constexpr char kFS[] =
+        "#version 310 es\n"
+        "precision highp float;\n"
+        // This inactive atomic counter should be removed by RemoveInactiveInterfaceVariables
+        "layout(binding = 0) uniform atomic_uint inactive;\n"
+        "out highp vec4 my_color;\n"
+        "void main()\n"
+        "{\n"
+        "    my_color = vec4(0.0, 1.0, 0.0, 1.0);\n"
+        // This barrier should be removed by RemoveAtomicCounterBuiltins because
+        // there are no active atomic counters
+        "    memoryBarrierAtomicCounter();\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(program, essl31_shaders::vs::Simple(), kFS);
+    glUseProgram(program);
+
+    drawQuad(program, essl31_shaders::PositionAttrib(), 0.0f);
+    ASSERT_GL_NO_ERROR();
+
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
 // TODO(syoussefi): re-enable tests on Vulkan once http://anglebug.com/3738 is resolved.  The issue
 // is with WGL where if a Vulkan test is run first in the shard, it causes crashes when an OpenGL
 // test is run afterwards.  AtomicCounter* tests are alphabetically first, and having them not run