Translator: Ensure structs and blocks are uniquely defined

src/compiler/translator/OutputGLSLBase.cpp

@@ -395,8 +395,8 @@ void TOutputGLSLBase::writeVariableType(const TType &type,
         out << getMemoryQualifiers(type);
     }
 
-    // Declare the struct if we have not done so already.
-    if (type.getBasicType() == EbtStruct && !structDeclared(type.getStruct()))
+    // Declare the struct.
+    if (type.isStructSpecifier())
     {
         const TStructure *structure = type.getStruct();
 
@@ -1279,17 +1279,6 @@ ImmutableString TOutputGLSLBase::hashFunctionNameIfNeeded(const TFunction *func)
     }
 }
 
-bool TOutputGLSLBase::structDeclared(const TStructure *structure) const
-{
-    ASSERT(structure);
-    if (structure->symbolType() == SymbolType::Empty)
-    {
-        return false;
-    }
-
-    return (mDeclaredStructs.count(structure->uniqueId().get()) > 0);
-}
-
 void TOutputGLSLBase::declareStruct(const TStructure *structure)
 {
     TInfoSinkBase &out = objSink();
@@ -1313,11 +1302,6 @@ void TOutputGLSLBase::declareStruct(const TStructure *structure)
         out << ";\n";
     }
     out << "}";
-
-    if (structure->symbolType() != SymbolType::Empty)
-    {
-        mDeclaredStructs.insert(structure->uniqueId().get());
-    }
 }
 
 void TOutputGLSLBase::declareInterfaceBlockLayout(const TType &type)

src/compiler/translator/OutputGLSLBase.h

@@ -89,7 +89,6 @@ class TOutputGLSLBase : public TIntermTraverser
 
     void declareStruct(const TStructure *structure);
     void writeQualifier(TQualifier qualifier, const TType &type, const TSymbol *symbol);
-    bool structDeclared(const TStructure *structure) const;
 
     const char *mapQualifierToString(TQualifier qualifier);
 
@@ -104,9 +103,6 @@ class TOutputGLSLBase : public TIntermTraverser
     TInfoSinkBase &mObjSink;
     bool mDeclaringVariable;
 
-    // This set contains all the ids of the structs from every scope.
-    std::set<int> mDeclaredStructs;
-
     ShArrayIndexClampingStrategy mClampingStrategy;
 
     // name hashing.

src/compiler/translator/OutputVulkanGLSL.cpp

@@ -174,15 +174,6 @@ void TOutputVulkanGLSL::writeVariableType(const TType &type,
     TOutputGLSL::writeVariableType(overrideType, symbol, isFunctionArgument);
 }
 
-void TOutputVulkanGLSL::writeStructType(const TStructure *structure)
-{
-    if (!structDeclared(structure))
-    {
-        declareStruct(structure);
-        objSink() << ";\n";
-    }
-}
-
 bool TOutputVulkanGLSL::writeVariablePrecision(TPrecision precision)
 {
     if ((precision == EbpUndefined) || !mEnablePrecision)

src/compiler/translator/OutputVulkanGLSL.h

@@ -32,8 +32,6 @@ class TOutputVulkanGLSL : public TOutputGLSL
                       bool enablePrecision,
                       ShCompileOptions compileOptions);
 
-    void writeStructType(const TStructure *structure);
-
     uint32_t nextUnusedBinding() { return mNextUnusedBinding++; }
     uint32_t nextUnusedInputLocation(uint32_t consumedCount)
     {

src/compiler/translator/ParseContext.cpp

@@ -2997,6 +2997,15 @@ TIntermDeclaration *TParseContext::parseSingleInitDeclaration(const TPublicType
         {
             declaration->appendDeclarator(initNode);
         }
+        else if (publicType.isStructSpecifier())
+        {
+            // The initialization got constant folded.  If it's a struct, declare the struct anyway.
+            TVariable *emptyVariable =
+                new TVariable(&symbolTable, kEmptyImmutableString, type, SymbolType::Empty);
+            TIntermSymbol *symbol = new TIntermSymbol(emptyVariable);
+            symbol->setLine(publicType.getLine());
+            declaration->appendDeclarator(symbol);
+        }
     }
     return declaration;
 }

src/compiler/translator/TranslatorMetal.cpp

@@ -116,7 +116,7 @@ ANGLE_NO_DISCARD bool InitializeUnusedOutputs(TIntermBlock *root,
 }  // anonymous namespace
 
 // class DriverUniformMetal
-TFieldList *DriverUniformMetal::createUniformFields(TSymbolTable *symbolTable) const
+TFieldList *DriverUniformMetal::createUniformFields(TSymbolTable *symbolTable)
 {
     TFieldList *driverFieldList = DriverUniform::createUniformFields(symbolTable);
 

src/compiler/translator/TranslatorMetal.h

@@ -48,7 +48,7 @@ class DriverUniformMetal : public DriverUniform
     TIntermBinary *getCoverageMaskFieldRef() const;
 
   protected:
-    TFieldList *createUniformFields(TSymbolTable *symbolTable) const override;
+    TFieldList *createUniformFields(TSymbolTable *symbolTable) override;
 };
 
 class TranslatorMetal : public TranslatorVulkan

src/compiler/translator/TranslatorVulkan.cpp

@@ -814,8 +814,8 @@ bool TranslatorVulkan::translateImpl(TInfoSinkBase &sink,
     // inactive samplers is not yet supported.  Note also that currently, CollectVariables marks
     // 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(),
-                                          getOutputVariables(), getUniforms(),
+    if (!RemoveInactiveInterfaceVariables(this, root, &getSymbolTable(), getAttributes(),
+                                          getInputVaryings(), getOutputVariables(), getUniforms(),
                                           getInterfaceBlocks()))
     {
         return false;

src/compiler/translator/ValidateAST.h

@@ -42,9 +42,11 @@ struct ValidateASTOptions
     // Check that there is only one TFunction with each function name referenced in the nodes (no
     // two TFunctions with the same name, taking internal/non-internal namespaces into account).
     bool validateUniqueFunctions = true;  // TODO
-    // Check that references to user-defined structs are matched with the corresponding struct
+    // Check that references to structs are matched with the corresponding struct declaration.  This
+    // is only done for references to structs inside other struct or interface blocks declarations,
+    // as validateVariableReferences already ensures other references to the struct match the
     // declaration.
-    bool validateStructUsage = true;  // TODO
+    bool validateStructUsage = true;
     // Check that expression nodes have the correct type considering their operand(s).
     bool validateExpressionTypes = true;  // TODO
     // If SeparateDeclarations has been run, check for the absence of multi declarations as well.

src/compiler/translator/tree_ops/ForcePrecisionQualifier.cpp

@@ -24,29 +24,13 @@ class TPrecisionTraverser : public TIntermTraverser
   protected:
     bool visitDeclaration(Visit visit, TIntermDeclaration *node) override;
 
-    bool structDeclared(const TStructure *structure) const;
     void overwriteVariablePrecision(TType *type) const;
-
-  private:
-    // This set contains all the ids of the structs from every scope.
-    std::set<int> mDeclaredStructs;
 };
 
 TPrecisionTraverser::TPrecisionTraverser(TSymbolTable *symbolTable)
     : TIntermTraverser(true, true, true, symbolTable)
 {}
 
-bool TPrecisionTraverser::structDeclared(const TStructure *structure) const
-{
-    ASSERT(structure);
-    if (structure->symbolType() == SymbolType::Empty)
-    {
-        return false;
-    }
-
-    return (mDeclaredStructs.count(structure->uniqueId().get()) > 0);
-}
-
 void TPrecisionTraverser::overwriteVariablePrecision(TType *type) const
 {
     if (type->getPrecision() == EbpHigh)
@@ -71,8 +55,8 @@ bool TPrecisionTraverser::visitDeclaration(Visit visit, TIntermDeclaration *node
             return true;
         }
 
-        // Visit the struct if we have not done so already.
-        if (type.getBasicType() == EbtStruct && !structDeclared(type.getStruct()))
+        // Visit the struct.
+        if (type.isStructSpecifier())
         {
             const TStructure *structure = type.getStruct();
             const TFieldList &fields    = structure->fields();
@@ -82,7 +66,6 @@ bool TPrecisionTraverser::visitDeclaration(Visit visit, TIntermDeclaration *node
                 const TType *fieldType = field->type();
                 overwriteVariablePrecision((TType *)fieldType);
             }
-            mDeclaredStructs.insert(structure->uniqueId().get());
         }
         else if (type.getBasicType() == EbtInterfaceBlock)
         {

src/compiler/translator/tree_ops/vulkan/RemoveInactiveInterfaceVariables.cpp

@@ -24,6 +24,7 @@ class RemoveInactiveInterfaceVariablesTraverser : public TIntermTraverser
 {
   public:
     RemoveInactiveInterfaceVariablesTraverser(
+        TSymbolTable *symbolTable,
         const std::vector<sh::ShaderVariable> &attributes,
         const std::vector<sh::ShaderVariable> &inputVaryings,
         const std::vector<sh::ShaderVariable> &outputVariables,
@@ -41,12 +42,13 @@ class RemoveInactiveInterfaceVariablesTraverser : public TIntermTraverser
 };
 
 RemoveInactiveInterfaceVariablesTraverser::RemoveInactiveInterfaceVariablesTraverser(
+    TSymbolTable *symbolTable,
     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),
+    : TIntermTraverser(true, false, false, symbolTable),
       mAttributes(attributes),
       mInputVaryings(inputVaryings),
       mOutputVariables(outputVariables),
@@ -128,9 +130,21 @@ bool RemoveInactiveInterfaceVariablesTraverser::visitDeclaration(Visit visit,
 
     if (removeDeclaration)
     {
-        TIntermSequence emptySequence;
+        TIntermSequence replacement;
+
+        // If the declaration was of a struct, keep the struct declaration itself.
+        if (type.isStructSpecifier())
+        {
+            TType *structSpecifierType      = new TType(type.getStruct(), true);
+            TVariable *emptyVariable        = new TVariable(mSymbolTable, kEmptyImmutableString,
+                                                     structSpecifierType, SymbolType::Empty);
+            TIntermDeclaration *declaration = new TIntermDeclaration();
+            declaration->appendDeclarator(new TIntermSymbol(emptyVariable));
+            replacement.push_back(declaration);
+        }
+
         mMultiReplacements.emplace_back(getParentNode()->getAsBlock(), node,
-                                        std::move(emptySequence));
+                                        std::move(replacement));
     }
 
     return false;
@@ -140,14 +154,15 @@ bool RemoveInactiveInterfaceVariablesTraverser::visitDeclaration(Visit visit,
 
 bool RemoveInactiveInterfaceVariables(TCompiler *compiler,
                                       TIntermBlock *root,
+                                      TSymbolTable *symbolTable,
                                       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, outputVariables,
-                                                        uniforms, interfaceBlocks);
+    RemoveInactiveInterfaceVariablesTraverser traverser(symbolTable, attributes, inputVaryings,
+                                                        outputVariables, uniforms, interfaceBlocks);
     root->traverse(&traverser);
     return traverser.updateTree(compiler, root);
 }

src/compiler/translator/tree_ops/vulkan/RemoveInactiveInterfaceVariables.h

@@ -29,6 +29,7 @@ class TSymbolTable;
 ANGLE_NO_DISCARD bool RemoveInactiveInterfaceVariables(
     TCompiler *compiler,
     TIntermBlock *root,
+    TSymbolTable *symbolTable,
     const std::vector<sh::ShaderVariable> &attributes,
     const std::vector<sh::ShaderVariable> &inputVaryings,
     const std::vector<sh::ShaderVariable> &outputVariables,

src/compiler/translator/tree_util/DriverUniform.cpp

@@ -71,7 +71,7 @@ bool DriverUniform::addComputeDriverUniformsToShader(TIntermBlock *root, TSymbol
     return mDriverUniforms != nullptr;
 }
 
-TFieldList *DriverUniform::createUniformFields(TSymbolTable *symbolTable) const
+TFieldList *DriverUniform::createUniformFields(TSymbolTable *symbolTable)
 {
     constexpr size_t kNumGraphicsDriverUniforms                                                = 8;
     constexpr std::array<const char *, kNumGraphicsDriverUniforms> kGraphicsDriverUniformNames = {
@@ -104,9 +104,15 @@ TFieldList *DriverUniform::createUniformFields(TSymbolTable *symbolTable) const
     return driverFieldList;
 }
 
-TType *DriverUniform::createEmulatedDepthRangeType(TSymbolTable *symbolTable) const
+TType *DriverUniform::createEmulatedDepthRangeType(TSymbolTable *symbolTable)
 {
-    // Init the depth range type.
+    // If already defined, return it immediately.
+    if (mEmulatedDepthRangeType != nullptr)
+    {
+        return mEmulatedDepthRangeType;
+    }
+
+    // Create the depth range type.
     TFieldList *depthRangeParamsFields = new TFieldList();
     depthRangeParamsFields->push_back(new TField(new TType(EbtFloat, EbpHigh, EvqGlobal, 1, 1),
                                                  ImmutableString("near"), TSourceLoc(),
@@ -125,9 +131,11 @@ TType *DriverUniform::createEmulatedDepthRangeType(TSymbolTable *symbolTable) co
     TStructure *emulatedDepthRangeParams = new TStructure(
         symbolTable, kEmulatedDepthRangeParams, depthRangeParamsFields, SymbolType::AngleInternal);
 
-    TType *emulatedDepthRangeType = new TType(emulatedDepthRangeParams, false);
+    mEmulatedDepthRangeType = new TType(emulatedDepthRangeParams, false);
 
-    return emulatedDepthRangeType;
+    // Note: this should really return a const TType *, but one of its uses is with TField who takes
+    // a non-const TType.  See comment on that class.
+    return mEmulatedDepthRangeType;
 }
 
 // The Add*DriverUniformsToShader operation adds an internal uniform block to a shader. The driver
@@ -139,11 +147,13 @@ bool DriverUniform::addGraphicsDriverUniformsToShader(TIntermBlock *root, TSymbo
 {
     ASSERT(!mDriverUniforms);
 
-    TType *emulatedDepthRangeType = createEmulatedDepthRangeType(symbolTable);
-    // Declare a global depth range variable.
+    // Declare the depth range struct type.
+    TType *emulatedDepthRangeType     = createEmulatedDepthRangeType(symbolTable);
+    TType *emulatedDepthRangeDeclType = new TType(emulatedDepthRangeType->getStruct(), true);
+
     TVariable *depthRangeVar =
         new TVariable(symbolTable->nextUniqueId(), kEmptyImmutableString, SymbolType::Empty,
-                      TExtension::UNDEFINED, emulatedDepthRangeType);
+                      TExtension::UNDEFINED, emulatedDepthRangeDeclType);
 
     DeclareGlobalVariable(root, depthRangeVar);
 
@@ -220,7 +230,7 @@ TIntermBinary *DriverUniform::getNumSamplesRef() const
 //
 // Class DriverUniformExtended
 //
-TFieldList *DriverUniformExtended::createUniformFields(TSymbolTable *symbolTable) const
+TFieldList *DriverUniformExtended::createUniformFields(TSymbolTable *symbolTable)
 {
     TFieldList *driverFieldList = DriverUniform::createUniformFields(symbolTable);
 

src/compiler/translator/tree_util/DriverUniform.h

@@ -26,7 +26,7 @@ class TIntermBinary;
 class DriverUniform
 {
   public:
-    DriverUniform() : mDriverUniforms(nullptr) {}
+    DriverUniform() : mDriverUniforms(nullptr), mEmulatedDepthRangeType(nullptr) {}
     virtual ~DriverUniform() = default;
 
     bool addComputeDriverUniformsToShader(TIntermBlock *root, TSymbolTable *symbolTable);
@@ -50,10 +50,11 @@ class DriverUniform
 
   protected:
     TIntermBinary *createDriverUniformRef(const char *fieldName) const;
-    virtual TFieldList *createUniformFields(TSymbolTable *symbolTable) const;
-    TType *createEmulatedDepthRangeType(TSymbolTable *symbolTable) const;
+    virtual TFieldList *createUniformFields(TSymbolTable *symbolTable);
+    TType *createEmulatedDepthRangeType(TSymbolTable *symbolTable);
 
     const TVariable *mDriverUniforms;
+    TType *mEmulatedDepthRangeType;
 };
 
 class DriverUniformExtended : public DriverUniform
@@ -69,7 +70,7 @@ class DriverUniformExtended : public DriverUniform
     TIntermSwizzle *getNegFlipYRef() const override;
 
   protected:
-    virtual TFieldList *createUniformFields(TSymbolTable *symbolTable) const override;
+    virtual TFieldList *createUniformFields(TSymbolTable *symbolTable) override;
 };
 
 }  // namespace sh

src/tests/gl_tests/GLSLTest.cpp

@@ -673,12 +673,6 @@ void main()
 
 TEST_P(GLSLTest, ScopedStructsOrderBug)
 {
-    // TODO(geofflang): Find out why this doesn't compile on Apple OpenGL drivers
-    // (http://anglebug.com/1292)
-    // TODO(geofflang): Find out why this doesn't compile on AMD OpenGL drivers
-    // (http://anglebug.com/1291)
-    ANGLE_SKIP_TEST_IF(IsDesktopOpenGL() && (IsOSX() || !IsNVIDIA()));
-
     constexpr char kFS[] = R"(precision mediump float;
 
 struct T
@@ -705,6 +699,132 @@ void main()
     ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), kFS);
 }
 
+// Test that defining a struct together with an inactive uniform, then using it in a scope that has
+// another struct with the same name declared works.
+TEST_P(GLSLTest, ScopedStructsOrderBug2)
+{
+    constexpr char kFS[] = R"(precision mediump float;
+
+uniform struct T
+{
+    float f;
+} x;
+
+void main()
+{
+    T a;
+
+    struct T
+    {
+        float q;
+    };
+
+    T b;
+
+    gl_FragColor = vec4(1, 0, 0, 1);
+    gl_FragColor.a += a.f;
+    gl_FragColor.a += b.q;
+})";
+
+    ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), kFS);
+}
+
+// Regression test based on WebGL's conformance/glsl/misc/empty-declaration.html
+TEST_P(GLSLTest, StructEmptyDeclaratorBug)
+{
+    constexpr char kVS[] = R"(
+struct S {
+    float member;
+}, a;
+void main() {
+    a.member = 0.0;
+    gl_Position = vec4(a.member);
+})";
+
+    constexpr char kFS[] = R"(precision mediump float;
+precision mediump float;
+void main()
+{
+    gl_FragColor = vec4(1.0,0.0,0.0,1.0);
+})";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+}
+
+// Regression test based on WebGL's conformance/ogles/GL/build/build_001_to_008.html
+TEST_P(GLSLTest, StructConstantFoldingBug)
+{
+    constexpr char kVS[] = R"(
+void main()
+{
+
+   const struct s2 {
+       int i;
+       vec3 v3;
+       bvec4 bv4;
+   } s22  = s2(8, vec3(9, 10, 11), bvec4(true, false, true, false));
+
+   struct s4 {
+       int ii;
+       vec4 v4;
+      };
+
+   const struct s1 {
+      s2 ss;
+      int i;
+      float f;
+      mat4 m;
+      s4 s44;
+   } s11 = s1(s22, 2, 4.0, mat4(5), s4(6, vec4(7, 8, 9, 10))) ;
+
+  const int field3 = s11.i * s11.ss.i;  // constant folding (int * int)
+  const vec4 field4 = s11.s44.v4 * s11.s44.v4; // constant folding (vec4 * vec4)
+ // 49, 64, 81, 100
+  const vec4 v4 = vec4(s11.ss.v3.y, s11.m[3][3], field3, field4[2]);  // 10.0, 5.0, 16.0, 81.0
+  gl_Position = v4;
+})";
+
+    constexpr char kFS[] = R"(precision mediump float;
+precision mediump float;
+void main()
+{
+    gl_FragColor = vec4(1.0,0.0,0.0,1.0);
+})";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+}
+
+// Test that constant folding doesn't remove struct declaration.
+TEST_P(GLSLTest, StructConstantFoldingBug2)
+{
+    constexpr char kVS[] = R"(
+uniform vec4 u;
+
+void main()
+{
+
+   const struct s2 {
+       int i;
+       vec3 v3;
+       bvec4 bv4;
+   } s22  = s2(8, vec3(9, 10, 11), bvec4(true, false, true, false));
+
+   s2 x;
+   x.v3 = u.xyz;
+
+   gl_Position = vec4(x.v3, float(s22.i));
+})";
+
+    constexpr char kFS[] = R"(precision mediump float;
+precision mediump float;
+void main()
+{
+    gl_FragColor = vec4(1.0,0.0,0.0,1.0);
+})";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+}
+
 TEST_P(GLSLTest, ScopedStructsBug)
 {
     constexpr char kFS[] = R"(precision mediump float;