Vulkan: Fix SPIR-V transformation name-info association

include/GLSLANG/ShaderLang.h

@@ -26,7 +26,7 @@
 
 // Version number for shader translation API.
 // It is incremented every time the API changes.
-#define ANGLE_SH_VERSION 224
+#define ANGLE_SH_VERSION 225
 
 enum ShShaderSpec
 {
@@ -763,12 +763,12 @@ extern const char kDefaultUniformsNameGS[];
 extern const char kDefaultUniformsNameFS[];
 extern const char kDefaultUniformsNameCS[];
 
-// Interface block and variable name containing driver uniforms
+// Interface block and variable names containing driver uniforms
 extern const char kDriverUniformsBlockName[];
 extern const char kDriverUniformsVarName[];
 
-// Interface block array variable name used for atomic counter emulation
-extern const char kAtomicCountersVarName[];
+// Interface block array name used for atomic counter emulation
+extern const char kAtomicCountersBlockName[];
 
 // Line raster emulation varying
 extern const char kLineRasterEmulationPosition[];

src/compiler/translator/ShaderLang.cpp

@@ -713,12 +713,12 @@ const char kDefaultUniformsNameGS[]  = "defaultUniformsGS";
 const char kDefaultUniformsNameFS[]  = "defaultUniformsFS";
 const char kDefaultUniformsNameCS[]  = "defaultUniformsCS";
 
-// Interface block and variable name containing driver uniforms
+// Interface block and variable names containing driver uniforms
 const char kDriverUniformsBlockName[] = "ANGLEUniformBlock";
 const char kDriverUniformsVarName[]   = "ANGLEUniforms";
 
-// Interface block array variable name used for atomic counter emulation
-const char kAtomicCountersVarName[] = "atomicCounters";
+// Interface block array name used for atomic counter emulation
+const char kAtomicCountersBlockName[] = "ANGLEAtomicCounters";
 
 const char kLineRasterEmulationPosition[] = "ANGLEPosition";
 

src/compiler/translator/tree_ops/RewriteAtomicCounters.cpp

@@ -21,7 +21,7 @@ namespace sh
 namespace
 {
 constexpr ImmutableString kAtomicCounterTypeName  = ImmutableString("ANGLE_atomic_uint");
-constexpr ImmutableString kAtomicCounterBlockName = ImmutableString("ANGLEAtomicCounters");
+constexpr ImmutableString kAtomicCountersVarName  = ImmutableString("atomicCounters");
 constexpr ImmutableString kAtomicCounterFieldName = ImmutableString("counters");
 
 // DeclareAtomicCountersBuffer adds a storage buffer array that's used with atomic counters.
@@ -45,9 +45,9 @@ const TVariable *DeclareAtomicCountersBuffers(TIntermBlock *root, TSymbolTable *
     constexpr uint32_t kMaxAtomicCounterBuffers = 8;
 
     // Define a storage block "ANGLEAtomicCounters" with instance name "atomicCounters".
-    return DeclareInterfaceBlock(root, symbolTable, fieldList, EvqBuffer, coherentMemory,
-                                 kMaxAtomicCounterBuffers, kAtomicCounterBlockName,
-                                 ImmutableString(vk::kAtomicCountersVarName));
+    return DeclareInterfaceBlock(
+        root, symbolTable, fieldList, EvqBuffer, coherentMemory, kMaxAtomicCounterBuffers,
+        ImmutableString(vk::kAtomicCountersBlockName), kAtomicCountersVarName);
 }
 
 TIntermConstantUnion *CreateUIntConstant(uint32_t value)

src/libANGLE/renderer/glslang_wrapper_utils.cpp

@@ -748,7 +748,7 @@ void AssignUniformBindings(const GlslangSourceOptions &options,
     }
 
     // Assign binding to the driver uniforms block
-    AddResourceInfo(variableInfoMapOut, sh::vk::kDriverUniformsVarName,
+    AddResourceInfo(variableInfoMapOut, sh::vk::kDriverUniformsBlockName,
                     options.driverUniformsDescriptorSetIndex, 0);
 }
 
@@ -781,7 +781,7 @@ uint32_t AssignAtomicCounterBufferBindings(const GlslangSourceOptions &options,
         return bindingStart;
     }
 
-    AddResourceInfo(variableInfoMapOut, sh::vk::kAtomicCountersVarName,
+    AddResourceInfo(variableInfoMapOut, sh::vk::kAtomicCountersBlockName,
                     options.shaderResourceDescriptorSetIndex, bindingStart);
 
     return bindingStart + 1;
@@ -1050,6 +1050,12 @@ class SpirvTransformer final : angle::NonCopyable
 
     // Transformation state:
 
+    // Names associated with ids through OpName.  The same name may be assigned to multiple ids, but
+    // not all names are interesting (for example function arguments).  When the variable
+    // declaration is met (OpVariable), the variable info is matched with the corresponding id's
+    // name based on the Storage Class.
+    std::vector<const char *> mNamesById;
+
     // Shader variable info per id, if id is a shader variable.
     std::vector<const ShaderInterfaceVariableInfo *> mVariableInfoById;
 
@@ -1123,6 +1129,11 @@ void SpirvTransformer::resolveVariableIds()
 {
     size_t indexBound = mSpirvBlobIn[kHeaderIndexIndexBound];
 
+    // Allocate storage for id-to-name map.  Used to associate ShaderInterfaceVariableInfo with ids
+    // based on name, but only when it's determined that the name corresponds to a shader interface
+    // variable.
+    mNamesById.resize(indexBound + 1, nullptr);
+
     // Allocate storage for id-to-info map.  If %i is the id of a name in mVariableInfoMap, index i
     // in this vector will hold a pointer to the ShaderInterfaceVariableInfo object associated with
     // that name in mVariableInfoMap.
@@ -1255,37 +1266,10 @@ void SpirvTransformer::visitName(const uint32_t *instruction)
     const char *name  = reinterpret_cast<const char *>(&instruction[kNameIndex]);
 
     // The names and ids are unique
-    ASSERT(id < mVariableInfoById.size());
-    ASSERT(mVariableInfoById[id] == nullptr);
-
-    bool isBuiltin = angle::BeginsWith(name, "gl_");
-    if (isBuiltin)
-    {
-        // Make all builtins point to this no-op info.  Adding this entry allows us to ASSERT that
-        // every shader interface variable is processed during the SPIR-V transformation.  This is
-        // done when iterating the ids provided by OpEntryPoint.
-        mVariableInfoById[id] = &mBuiltinVariableInfo;
-        return;
-    }
-
-    auto infoIter = mVariableInfoMap.find(name);
-    if (infoIter == mVariableInfoMap.end())
-    {
-        return;
-    }
+    ASSERT(id < mNamesById.size());
+    ASSERT(mNamesById[id] == nullptr);
 
-    const ShaderInterfaceVariableInfo *info = &infoIter->second;
-
-    // Associate the id of this name with its info.
-    mVariableInfoById[id] = info;
-
-    // Note if the variable is captured by transform feedback.  In that case, the TransformFeedback
-    // capability needs to be added.
-    if (mShaderType != gl::ShaderType::Fragment &&
-        info->xfbBuffer != ShaderInterfaceVariableInfo::kInvalid && info->activeStages[mShaderType])
-    {
-        mHasTransformFeedbackOutput = true;
-    }
+    mNamesById[id] = name;
 }
 
 void SpirvTransformer::visitTypeHelper(const uint32_t *instruction,
@@ -1296,19 +1280,17 @@ void SpirvTransformer::visitTypeHelper(const uint32_t *instruction,
     const uint32_t typeId = instruction[typeIdIndex];
 
     // Every type id is declared only once.
-    ASSERT(typeId < mVariableInfoById.size());
+    ASSERT(id < mNamesById.size());
+    ASSERT(mNamesById[id] == nullptr);
 
-    if (mVariableInfoById[typeId] != nullptr)
-    {
-        // Carry the info forward from the base type.  This is only necessary for interface blocks,
-        // as the variable info is associated with the block name instead of the variable name (to
-        // support nameless interface blocks).  In that case, the variable itself doesn't yet have
-        // an associated info.
-        ASSERT(id < mVariableInfoById.size());
-        ASSERT(mVariableInfoById[id] == nullptr);
-
-        mVariableInfoById[id] = mVariableInfoById[typeId];
-    }
+    // Carry the name forward from the base type.  This is only necessary for interface blocks,
+    // as the variable info is associated with the block name instead of the variable name (to
+    // support nameless interface blocks).  When the variable declaration is met, either the
+    // type name or the variable name is used to associate with info based on the variable's
+    // storage class.
+    ASSERT(typeId < mNamesById.size());
+
+    mNamesById[id] = mNamesById[typeId];
 }
 
 void SpirvTransformer::visitTypeArray(const uint32_t *instruction)
@@ -1336,15 +1318,66 @@ void SpirvTransformer::visitVariable(const uint32_t *instruction)
     constexpr size_t kIdIndex           = 2;
     constexpr size_t kStorageClassIndex = 3;
 
-    visitTypeHelper(instruction, kIdIndex, kTypeIdIndex);
-
     // All resources that take set/binding should be transformed.
+    const uint32_t typeId       = instruction[kTypeIdIndex];
     const uint32_t id           = instruction[kIdIndex];
     const uint32_t storageClass = instruction[kStorageClassIndex];
 
-    ASSERT((storageClass != spv::StorageClassUniform && storageClass != spv::StorageClassImage &&
-            storageClass != spv::StorageClassStorageBuffer) ||
-           mVariableInfoById[id] != nullptr);
+    ASSERT(typeId < mNamesById.size());
+    ASSERT(id < mNamesById.size());
+
+    // If storage class indicates that this is not a shader interface variable, ignore it.
+    const bool isInterfaceBlockVariable =
+        storageClass == spv::StorageClassUniform || storageClass == spv::StorageClassStorageBuffer;
+    const bool isOpaqueUniform = storageClass == spv::StorageClassUniformConstant;
+    const bool isInOut =
+        storageClass == spv::StorageClassInput || storageClass == spv::StorageClassOutput;
+
+    if (!isInterfaceBlockVariable && !isOpaqueUniform && !isInOut)
+    {
+        return;
+    }
+
+    // The ids are unique.
+    ASSERT(id < mVariableInfoById.size());
+    ASSERT(mVariableInfoById[id] == nullptr);
+
+    // For interface block variables, the name that's used to associate info is the block name
+    // rather than the variable name.
+    const char *name = mNamesById[isInterfaceBlockVariable ? typeId : id];
+    ASSERT(name != nullptr);
+
+    // Handle builtins, which all start with "gl_".  Either the variable name could be an indication
+    // of a builtin variable (such as with gl_FragCoord) or the type name (such as with
+    // gl_PerVertex).
+    const bool isNameBuiltin = isInOut && angle::BeginsWith(name, "gl_");
+    const bool isTypeBuiltin =
+        isInOut && mNamesById[typeId] != nullptr && angle::BeginsWith(mNamesById[typeId], "gl_");
+    if (isNameBuiltin || isTypeBuiltin)
+    {
+        // Make all builtins point to this no-op info.  Adding this entry allows us to ASSERT that
+        // every shader interface variable is processed during the SPIR-V transformation.  This is
+        // done when iterating the ids provided by OpEntryPoint.
+        mVariableInfoById[id] = &mBuiltinVariableInfo;
+        return;
+    }
+
+    // Every shader interface variable should have an associated data.
+    auto infoIter = mVariableInfoMap.find(name);
+    ASSERT(infoIter != mVariableInfoMap.end());
+
+    const ShaderInterfaceVariableInfo *info = &infoIter->second;
+
+    // Associate the id of this name with its info.
+    mVariableInfoById[id] = info;
+
+    // Note if the variable is captured by transform feedback.  In that case, the TransformFeedback
+    // capability needs to be added.
+    if (mShaderType != gl::ShaderType::Fragment &&
+        info->xfbBuffer != ShaderInterfaceVariableInfo::kInvalid && info->activeStages[mShaderType])
+    {
+        mHasTransformFeedbackOutput = true;
+    }
 }
 
 bool SpirvTransformer::transformDecorate(const uint32_t *instruction, size_t wordCount)
@@ -1566,9 +1599,11 @@ bool SpirvTransformer::transformTypePointer(const uint32_t *instruction, size_t
     // SPIR-V 1.0 Section 3.32 Instructions, OpTypePointer
     constexpr size_t kIdIndex           = 1;
     constexpr size_t kStorageClassIndex = 2;
+    constexpr size_t kTypeIdIndex       = 3;
 
     const uint32_t id           = instruction[kIdIndex];
     const uint32_t storageClass = instruction[kStorageClassIndex];
+    const uint32_t typeId       = instruction[kTypeIdIndex];
 
     // If the storage class is output, this may be used to create a variable corresponding to an
     // inactive varying, or if that varying is a struct, an Op*AccessChain retrieving a field of
@@ -1584,14 +1619,9 @@ bool SpirvTransformer::transformTypePointer(const uint32_t *instruction, size_t
         return false;
     }
 
-    // Cannot create a Private type declaration from the builtin gl_PerVertex.  Note that
-    // mVariableInfoById is only ever set for variables, except for nameless interface blocks and
-    // the builtin gl_PerVertex.  Since the storage class is Output, if mVariableInfoById for this
-    // type is not nullptr, this must be a builtin.
-    const ShaderInterfaceVariableInfo *info = mVariableInfoById[id];
-    if (info)
+    // Cannot create a Private type declaration from builtins such as gl_PerVertex.
+    if (mNamesById[typeId] != nullptr && angle::BeginsWith(mNamesById[typeId], "gl_"))
     {
-        ASSERT(info == &mBuiltinVariableInfo);
         return false;
     }
 

src/tests/gl_tests/GLSLTest.cpp

@@ -7933,6 +7933,72 @@ void main() { v_varying = a_position.x; gl_Position = a_position; })";
     EXPECT_EQ(0u, program);
 }
 
+// Test that reusing the same variable name for different uses across stages links fine.  Glslang
+// wrapper's SPIR-V transformation should ignore all names for non-shader-interface variables and
+// not get confused by them.
+TEST_P(GLSLTest_ES31, VariableNameReuseAcrossStages)
+{
+    // Fails to compile the fragment shader with error "undeclared identifier '_g'"
+    // http://anglebug.com/4404
+    ANGLE_SKIP_TEST_IF(IsD3D11());
+
+    constexpr char kVS[] = R"(#version 310 es
+precision mediump float;
+uniform highp vec4 a;
+in highp vec4 b;
+in highp vec4 c;
+in highp vec4 d;
+out highp vec4 e;
+
+vec4 f(vec4 a)
+{
+    return a;
+}
+
+vec4 g(vec4 f)
+{
+    return f + f;
+}
+
+void main() {
+    e = f(b) + a;
+    gl_Position = g(c) + f(d);
+}
+)";
+
+    constexpr char kFS[] = R"(#version 310 es
+precision mediump float;
+in highp vec4 e;
+uniform sampler2D f;
+layout(rgba8) uniform highp readonly image2D g;
+uniform A
+{
+    vec4 x;
+} c;
+layout(std140, binding=0) buffer B
+{
+    vec4 x;
+} d[2];
+out vec4 col;
+
+vec4 h(vec4 c)
+{
+    return texture(f, c.xy) + imageLoad(g, ivec2(c.zw));
+}
+
+vec4 i(vec4 x, vec4 y)
+{
+    return vec4(x.xy, y.zw);
+}
+
+void main() {
+    col = h(e) + i(c.x, d[0].x) + d[1].x;
+}
+)";
+
+    GLuint program = CompileProgram(kVS, kFS);
+    EXPECT_NE(0u, program);
+}
 }  // anonymous namespace
 
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these