Remove unreferenced struct types from the AST

src/compiler/translator/OutputGLSLBase.cpp

@@ -86,7 +86,7 @@ TOutputGLSLBase::TOutputGLSLBase(TInfoSinkBase &objSink,
                                  ShCompileOptions compileOptions)
     : TIntermTraverser(true, true, true, symbolTable),
       mObjSink(objSink),
-      mDeclaringVariables(false),
+      mDeclaringVariable(false),
       mClampingStrategy(clampingStrategy),
       mHashFunction(hashFunction),
       mNameMap(nameMap),
@@ -435,7 +435,7 @@ void TOutputGLSLBase::visitSymbol(TIntermSymbol *node)
     TInfoSinkBase &out = objSink();
     out << hashVariableName(node->getName());
 
-    if (mDeclaringVariables && node->getType().isArray())
+    if (mDeclaringVariable && node->getType().isArray())
         out << ArrayString(node->getType());
 }
 
@@ -469,7 +469,7 @@ bool TOutputGLSLBase::visitBinary(Visit visit, TIntermBinary *node)
             {
                 out << " = ";
                 // RHS of initialize is not being declared.
-                mDeclaringVariables = false;
+                mDeclaringVariable = false;
             }
             break;
         case EOpAssign:
@@ -995,17 +995,20 @@ bool TOutputGLSLBase::visitDeclaration(Visit visit, TIntermDeclaration *node)
         TIntermTyped *variable          = sequence.front()->getAsTyped();
         writeLayoutQualifier(variable);
         writeVariableType(variable->getType());
+        if (variable->getAsSymbolNode() == nullptr ||
+            !variable->getAsSymbolNode()->getSymbol().empty())
+        {
             out << " ";
-        mDeclaringVariables = true;
+        }
+        mDeclaringVariable = true;
     }
     else if (visit == InVisit)
     {
-        out << ", ";
-        mDeclaringVariables = true;
+        UNREACHABLE();
     }
     else
     {
-        mDeclaringVariables = false;
+        mDeclaringVariable = false;
     }
     return true;
 }

src/compiler/translator/OutputGLSLBase.h

@@ -88,7 +88,7 @@ class TOutputGLSLBase : public TIntermTraverser
     const char *mapQualifierToString(TQualifier qialifier);
 
     TInfoSinkBase &mObjSink;
-    bool mDeclaringVariables;
+    bool mDeclaringVariable;
 
     // This set contains all the ids of the structs from every scope.
     std::set<int> mDeclaredStructs;

src/compiler/translator/RemoveUnreferencedVariables.cpp

@@ -5,7 +5,7 @@
 //
 // RemoveUnreferencedVariables.cpp:
 //  Drop variables that are declared but never referenced in the AST. This avoids adding unnecessary
-//  initialization code for them.
+//  initialization code for them. Also removes unreferenced struct types.
 //
 
 #include "compiler/translator/RemoveUnreferencedVariables.h"
@@ -26,11 +26,24 @@ class CollectVariableRefCountsTraverser : public TIntermTraverser
 
     using RefCountMap = std::unordered_map<int, unsigned int>;
     RefCountMap &getSymbolIdRefCounts() { return mSymbolIdRefCounts; }
+    RefCountMap &getStructIdRefCounts() { return mStructIdRefCounts; }
 
     void visitSymbol(TIntermSymbol *node) override;
+    bool visitAggregate(Visit visit, TIntermAggregate *node) override;
+    bool visitFunctionPrototype(Visit visit, TIntermFunctionPrototype *node) override;
 
   private:
+    void incrementStructTypeRefCount(const TType &type);
+
     RefCountMap mSymbolIdRefCounts;
+
+    // Structure reference counts are counted from symbols, constructors, function calls, function
+    // return values and from interface block and structure fields. We need to track both function
+    // calls and function return values since there's a compiler option not to prune unused
+    // functions. The type of a constant union may also be a struct, but statements that are just a
+    // constant union are always pruned, and if the constant union is used somehow it will get
+    // counted by something else.
+    RefCountMap mStructIdRefCounts;
 };
 
 CollectVariableRefCountsTraverser::CollectVariableRefCountsTraverser()
@@ -38,8 +51,47 @@ CollectVariableRefCountsTraverser::CollectVariableRefCountsTraverser()
 {
 }
 
+void CollectVariableRefCountsTraverser::incrementStructTypeRefCount(const TType &type)
+{
+    if (type.isInterfaceBlock())
+    {
+        const auto *block = type.getInterfaceBlock();
+        ASSERT(block);
+
+        // We can end up incrementing ref counts of struct types referenced from an interface block
+        // multiple times for the same block. This doesn't matter, because interface blocks can't be
+        // pruned so we'll never do the reverse operation.
+        for (const auto &field : block->fields())
+        {
+            ASSERT(!field->type()->isInterfaceBlock());
+            incrementStructTypeRefCount(*field->type());
+        }
+        return;
+    }
+
+    const auto *structure = type.getStruct();
+    if (structure != nullptr)
+    {
+        auto structIter = mStructIdRefCounts.find(structure->uniqueId());
+        if (structIter == mStructIdRefCounts.end())
+        {
+            mStructIdRefCounts[structure->uniqueId()] = 1u;
+
+            for (const auto &field : structure->fields())
+            {
+                incrementStructTypeRefCount(*field->type());
+            }
+
+            return;
+        }
+        ++(structIter->second);
+    }
+}
+
 void CollectVariableRefCountsTraverser::visitSymbol(TIntermSymbol *node)
 {
+    incrementStructTypeRefCount(node->getType());
+
     auto iter = mSymbolIdRefCounts.find(node->getId());
     if (iter == mSymbolIdRefCounts.end())
     {
@@ -49,16 +101,32 @@ void CollectVariableRefCountsTraverser::visitSymbol(TIntermSymbol *node)
     ++(iter->second);
 }
 
+bool CollectVariableRefCountsTraverser::visitAggregate(Visit visit, TIntermAggregate *node)
+{
+    // This tracks struct references in both function calls and constructors.
+    incrementStructTypeRefCount(node->getType());
+    return true;
+}
+
+bool CollectVariableRefCountsTraverser::visitFunctionPrototype(Visit visit,
+                                                               TIntermFunctionPrototype *node)
+{
+    incrementStructTypeRefCount(node->getType());
+    return true;
+}
+
 // Traverser that removes all unreferenced variables on one traversal.
 class RemoveUnreferencedVariablesTraverser : public TIntermTraverser
 {
   public:
     RemoveUnreferencedVariablesTraverser(
         CollectVariableRefCountsTraverser::RefCountMap *symbolIdRefCounts,
+        CollectVariableRefCountsTraverser::RefCountMap *structIdRefCounts,
         TSymbolTable *symbolTable);
 
     bool visitDeclaration(Visit visit, TIntermDeclaration *node) override;
     void visitSymbol(TIntermSymbol *node) override;
+    bool visitAggregate(Visit visit, TIntermAggregate *node) override;
 
     // Traverse loop and block nodes in reverse order. Note that this traverser does not track
     // parent block positions, so insertStatementInParentBlock is unusable!
@@ -66,35 +134,71 @@ class RemoveUnreferencedVariablesTraverser : public TIntermTraverser
     void traverseLoop(TIntermLoop *loop) override;
 
   private:
-    void removeDeclaration(TIntermDeclaration *node, TIntermTyped *declarator);
+    void removeVariableDeclaration(TIntermDeclaration *node, TIntermTyped *declarator);
+    void decrementStructTypeRefCount(const TType &type);
 
     CollectVariableRefCountsTraverser::RefCountMap *mSymbolIdRefCounts;
+    CollectVariableRefCountsTraverser::RefCountMap *mStructIdRefCounts;
     bool mRemoveReferences;
 };
 
 RemoveUnreferencedVariablesTraverser::RemoveUnreferencedVariablesTraverser(
     CollectVariableRefCountsTraverser::RefCountMap *symbolIdRefCounts,
+    CollectVariableRefCountsTraverser::RefCountMap *structIdRefCounts,
     TSymbolTable *symbolTable)
     : TIntermTraverser(true, false, true, symbolTable),
       mSymbolIdRefCounts(symbolIdRefCounts),
+      mStructIdRefCounts(structIdRefCounts),
       mRemoveReferences(false)
 {
 }
 
-void RemoveUnreferencedVariablesTraverser::removeDeclaration(TIntermDeclaration *node,
+void RemoveUnreferencedVariablesTraverser::decrementStructTypeRefCount(const TType &type)
+{
+    auto *structure = type.getStruct();
+    if (structure != nullptr)
+    {
+        ASSERT(mStructIdRefCounts->find(structure->uniqueId()) != mStructIdRefCounts->end());
+        unsigned int structRefCount = --(*mStructIdRefCounts)[structure->uniqueId()];
+
+        if (structRefCount == 0)
+        {
+            for (const auto &field : structure->fields())
+            {
+                decrementStructTypeRefCount(*field->type());
+            }
+        }
+    }
+}
+
+void RemoveUnreferencedVariablesTraverser::removeVariableDeclaration(TIntermDeclaration *node,
                                                                      TIntermTyped *declarator)
 {
     if (declarator->getType().isStructSpecifier() && !declarator->getType().isNamelessStruct())
     {
-        // We don't count references to struct types, so if this declaration declares a named struct
-        // type, we'll keep it. We can still change the declarator though so that it doesn't declare
-        // a variable.
-        queueReplacementWithParent(
-            node, declarator,
-            new TIntermSymbol(mSymbolTable->getEmptySymbolId(), TString(""), declarator->getType()),
+        unsigned int structId = declarator->getType().getStruct()->uniqueId();
+        if ((*mStructIdRefCounts)[structId] > 1u)
+        {
+            // If this declaration declares a named struct type that is used elsewhere, we need to
+            // keep it. We can still change the declarator though so that it doesn't declare an
+            // unreferenced variable.
+
+            // Note that since we're not removing the entire declaration, the struct's reference
+            // count will end up being one less than the correct refcount. But since the struct
+            // declaration is kept, the incorrect refcount can't cause any other problems.
+
+            if (declarator->getAsSymbolNode() && declarator->getAsSymbolNode()->getSymbol().empty())
+            {
+                // Already an empty declaration - nothing to do.
+                return;
+            }
+            queueReplacementWithParent(node, declarator,
+                                       new TIntermSymbol(mSymbolTable->getEmptySymbolId(),
+                                                         TString(""), declarator->getType()),
                                        OriginalNode::IS_DROPPED);
             return;
         }
+    }
 
     if (getParentNode()->getAsBlock())
     {
@@ -126,24 +230,25 @@ bool RemoveUnreferencedVariablesTraverser::visitDeclaration(Visit visit, TInterm
             return true;
         }
 
-        bool canRemove            = false;
+        bool canRemoveVariable    = false;
         TIntermSymbol *symbolNode = declarator->getAsSymbolNode();
         if (symbolNode != nullptr)
         {
-            canRemove = (*mSymbolIdRefCounts)[symbolNode->getId()] == 1u;
+            canRemoveVariable =
+                (*mSymbolIdRefCounts)[symbolNode->getId()] == 1u || symbolNode->getSymbol().empty();
         }
         TIntermBinary *initNode = declarator->getAsBinaryNode();
         if (initNode != nullptr)
         {
             ASSERT(initNode->getLeft()->getAsSymbolNode());
             int symbolId = initNode->getLeft()->getAsSymbolNode()->getId();
-            canRemove =
+            canRemoveVariable =
                 (*mSymbolIdRefCounts)[symbolId] == 1u && !initNode->getRight()->hasSideEffects();
         }
 
-        if (canRemove)
+        if (canRemoveVariable)
         {
-            removeDeclaration(node, declarator);
+            removeVariableDeclaration(node, declarator);
             mRemoveReferences = true;
         }
         return true;
@@ -159,9 +264,20 @@ void RemoveUnreferencedVariablesTraverser::visitSymbol(TIntermSymbol *node)
     {
         ASSERT(mSymbolIdRefCounts->find(node->getId()) != mSymbolIdRefCounts->end());
         --(*mSymbolIdRefCounts)[node->getId()];
+
+        decrementStructTypeRefCount(node->getType());
     }
 }
 
+bool RemoveUnreferencedVariablesTraverser::visitAggregate(Visit visit, TIntermAggregate *node)
+{
+    if (mRemoveReferences)
+    {
+        decrementStructTypeRefCount(node->getType());
+    }
+    return true;
+}
+
 void RemoveUnreferencedVariablesTraverser::traverseBlock(TIntermBlock *node)
 {
     // We traverse blocks in reverse order.  This way reference counts can be decremented when
@@ -233,7 +349,8 @@ void RemoveUnreferencedVariables(TIntermBlock *root, TSymbolTable *symbolTable)
 {
     CollectVariableRefCountsTraverser collector;
     root->traverse(&collector);
-    RemoveUnreferencedVariablesTraverser traverser(&collector.getSymbolIdRefCounts(), symbolTable);
+    RemoveUnreferencedVariablesTraverser traverser(&collector.getSymbolIdRefCounts(),
+                                                   &collector.getStructIdRefCounts(), symbolTable);
     root->traverse(&traverser);
     traverser.updateTree();
 }

src/compiler/translator/RemoveUnreferencedVariables.h

@@ -5,7 +5,7 @@
 //
 // RemoveUnreferencedVariables.h:
 //  Drop variables that are declared but never referenced in the AST. This avoids adding unnecessary
-//  initialization code for them.
+//  initialization code for them. Also removes unreferenced struct types.
 //
 
 #ifndef COMPILER_TRANSLATOR_REMOVEUNREFERENCEDVARIABLES_H_

src/tests/compiler_tests/RemoveUnreferencedVariables_test.cpp

@@ -390,3 +390,299 @@ TEST_F(RemoveUnreferencedVariablesTest, VariableOnlyReferencedInLengthMethod)
 
     ASSERT_TRUE(notFoundInCode("onlyReferencedInLengthMethodCall"));
 }
+
+// Test that an unreferenced user-defined type is removed.
+TEST_F(RemoveUnreferencedVariablesTest, UserDefinedTypeUnreferenced)
+{
+    const std::string &shaderString =
+        R"(
+        struct myStructType
+        {
+            int i;
+        } myStructVariable;
+
+        void main()
+        {
+        })";
+    compile(shaderString);
+
+    ASSERT_TRUE(notFoundInCode("myStructType"));
+}
+
+// Test that a user-defined type that's only referenced in an unreferenced variable is removed.
+TEST_F(RemoveUnreferencedVariablesTest, UserDefinedTypeReferencedInUnreferencedVariable)
+{
+    const std::string &shaderString =
+        R"(
+        struct myStructType
+        {
+            int i;
+        };
+
+        void main()
+        {
+            myStructType myStructVariable;
+        })";
+    compile(shaderString);
+
+    ASSERT_TRUE(notFoundInCode("myStructType"));
+}
+
+// Test that a user-defined type that's declared in an empty declaration and that is only referenced
+// in an unreferenced variable is removed also when the shader contains another independent
+// user-defined type that's declared in an empty declaration. This tests special case handling of
+// reference counting of empty symbols.
+TEST_F(RemoveUnreferencedVariablesTest,
+       TwoUserDefinedTypesDeclaredInEmptyDeclarationsWithOneOfThemUnreferenced)
+{
+    const std::string &shaderString =
+        R"(
+        struct myStructTypeA
+        {
+            int i;
+        };
+
+        struct myStructTypeB
+        {
+            int j;
+        };
+
+        uniform myStructTypeB myStructVariableB;
+
+        void main()
+        {
+            myStructTypeA myStructVariableA;
+        })";
+    compile(shaderString);
+
+    ASSERT_TRUE(notFoundInCode("myStructTypeA"));
+    ASSERT_TRUE(foundInCode("myStructTypeB"));
+}
+
+// Test that a user-defined type that is only referenced in another unreferenced type is removed.
+TEST_F(RemoveUnreferencedVariablesTest, UserDefinedTypeChain)
+{
+    const std::string &shaderString =
+        R"(
+        struct myInnerStructType
+        {
+            int i;
+        };
+
+        struct myOuterStructType
+        {
+            myInnerStructType inner;
+        } myStructVariable;
+
+        void main()
+        {
+            myOuterStructType myStructVariable2;
+        })";
+    compile(shaderString);
+
+    ASSERT_TRUE(notFoundInCode("myInnerStructType"));
+}
+
+// Test that a user-defined type that is referenced in another user-defined type that is used is
+// kept.
+TEST_F(RemoveUnreferencedVariablesTest, UserDefinedTypeChainReferenced)
+{
+    const std::string &shaderString =
+        R"(
+        precision mediump float;
+
+        struct myInnerStructType
+        {
+            int i;
+        };
+
+        uniform struct
+        {
+            myInnerStructType inner;
+        } myStructVariable;
+
+        void main()
+        {
+            if (myStructVariable.inner.i > 0)
+            {
+                gl_FragColor = vec4(0, 1, 0, 1);
+            }
+        })";
+    compile(shaderString);
+
+    ASSERT_TRUE(foundInCode("struct _umyInnerStructType"));
+}
+
+// Test that a struct type that is only referenced in a constructor and function call is kept.
+TEST_F(RemoveUnreferencedVariablesTest, UserDefinedTypeReferencedInConstructorAndCall)
+{
+    const std::string &shaderString =
+        R"(
+        precision mediump float;
+
+        uniform int ui;
+
+        struct myStructType
+        {
+            int iMember;
+        };
+
+        void func(myStructType myStructParam)
+        {
+            if (myStructParam.iMember > 0)
+            {
+                gl_FragColor = vec4(0, 1, 0, 1);
+            }
+        }
+
+        void main()
+        {
+            func(myStructType(ui));
+        })";
+    compile(shaderString);
+
+    ASSERT_TRUE(foundInCode("struct _umyStructType"));
+}
+
+// Test that a struct type that is only referenced in a constructor is kept. This assumes that there
+// isn't more sophisticated folding of struct field access going on.
+TEST_F(RemoveUnreferencedVariablesTest, UserDefinedTypeReferencedInConstructor)
+{
+    const std::string &shaderString =
+        R"(
+        precision mediump float;
+
+        uniform int ui;
+
+        struct myStructType
+        {
+            int iMember;
+        };
+
+        void main()
+        {
+            if (myStructType(ui).iMember > 0)
+            {
+                gl_FragColor = vec4(0, 1, 0, 1);
+            }
+        })";
+    compile(shaderString);
+
+    ASSERT_TRUE(foundInCode("struct _umyStructType"));
+}
+
+// Test that a struct type that is only referenced in an unused function is removed.
+TEST_F(RemoveUnreferencedVariablesTest, UserDefinedTypeReferencedInUnusedFunction)
+{
+    const std::string &shaderString =
+        R"(
+        precision mediump float;
+
+        struct myStructType
+        {
+            int iMember;
+        };
+
+        void func(myStructType myStructParam)
+        {
+            if (myStructParam.iMember > 0)
+            {
+                gl_FragColor = vec4(0, 1, 0, 1);
+            }
+        }
+
+        void main()
+        {
+        })";
+    compile(shaderString);
+
+    ASSERT_TRUE(notFoundInCode("myStructType"));
+}
+
+// Test that a struct type that is only referenced in an unused function is kept in case
+// SH_DONT_PRUNE_UNUSED_FUNCTIONS is specified.
+TEST_F(RemoveUnreferencedVariablesTest, UserDefinedTypeReferencedInUnusedFunctionThatIsNotPruned)
+{
+    const std::string &shaderString =
+        R"(
+        struct myStructType
+        {
+            int iMember;
+        };
+
+        myStructType func()
+        {
+            return myStructType(0);
+        }
+
+        void main()
+        {
+        })";
+    compile(shaderString, SH_DONT_PRUNE_UNUSED_FUNCTIONS);
+
+    ASSERT_TRUE(foundInCode("struct _umyStructType"));
+
+    // Ensure that the struct isn't declared as a part of the function header.
+    ASSERT_TRUE(foundInCode("};"));
+}
+
+// Test that a struct type that is only referenced as a function return value is kept.
+TEST_F(RemoveUnreferencedVariablesTest, UserDefinedTypeReturnedFromFunction)
+{
+    const std::string &shaderString =
+        R"(
+        precision mediump float;
+
+        struct myStructType
+        {
+            int iMember;
+        };
+
+        myStructType func()
+        {
+            gl_FragColor = vec4(0, 1, 0, 1);
+            return myStructType(0);
+        }
+
+        void main()
+        {
+            func();
+        })";
+    compile(shaderString);
+
+    ASSERT_TRUE(foundInCode("struct _umyStructType"));
+
+    // Ensure that the struct isn't declared as a part of the function header.
+    ASSERT_TRUE(foundInCode("};"));
+}
+
+// Test that a struct type that is only referenced in a uniform block is kept.
+TEST_F(RemoveUnreferencedVariablesTest, UserDefinedTypeInUniformBlock)
+{
+    const std::string &shaderString =
+        R"(#version 300 es
+
+        precision highp float;
+        out vec4 my_FragColor;
+
+        struct myStructType
+        {
+            int iMember;
+        };
+
+        layout(std140) uniform myBlock {
+            myStructType uStruct;
+            int ui;
+        };
+
+        void main()
+        {
+            if (ui > 0)
+            {
+                my_FragColor = vec4(0, 1, 0, 1);
+            }
+        })";
+    compile(shaderString);
+
+    ASSERT_TRUE(foundInCode("struct _umyStructType"));
+}