Collect static use information during parsing

src/compiler/translator/CollectVariables.cpp

@@ -68,21 +68,21 @@ VarT *FindVariable(const ImmutableString &name, std::vector<VarT> *infoList)
     return nullptr;
 }
 
-// Note that this shouldn't be called for interface blocks - static use information is collected for
+// Note that this shouldn't be called for interface blocks - active information is collected for
 // individual fields in case of interface blocks.
-void MarkStaticallyUsed(ShaderVariable *variable)
+void MarkActive(ShaderVariable *variable)
 {
-    if (!variable->staticUse)
+    if (!variable->active)
     {
         if (variable->isStruct())
         {
             // Conservatively assume all fields are statically used as well.
             for (auto &field : variable->fields)
             {
-                MarkStaticallyUsed(&field);
+                MarkActive(&field);
             }
         }
-        variable->staticUse = true;
+        ASSERT(variable->staticUse);
         variable->active    = true;
     }
 }
@@ -126,9 +126,12 @@ class CollectVariablesTraverser : public TIntermTraverser
   private:
     std::string getMappedName(const TSymbol *symbol) const;
 
-    void setFieldOrVariableProperties(const TType &type, ShaderVariable *variableOut) const;
+    void setFieldOrVariableProperties(const TType &type,
+                                      bool staticUse,
+                                      ShaderVariable *variableOut) const;
     void setFieldProperties(const TType &type,
                             const ImmutableString &name,
+                            bool staticUse,
                             ShaderVariable *variableOut) const;
     void setCommonVariableProperties(const TType &type,
                                      const TVariable &variable,
@@ -322,8 +325,6 @@ InterfaceBlock *CollectVariablesTraverser::recordGLInUsed(const TType &glInType)
         ASSERT(glInType.getQualifier() == EvqPerVertexIn);
         InterfaceBlock info;
         recordInterfaceBlock("gl_in", glInType, &info);
-        info.staticUse = true;
-        info.active    = true;
 
         mPerVertexInAdded = true;
         mInBlocks->push_back(info);
@@ -551,15 +552,18 @@ void CollectVariablesTraverser::visitSymbol(TIntermSymbol *symbol)
     }
     if (var)
     {
-        MarkStaticallyUsed(var);
+        MarkActive(var);
     }
 }
 
 void CollectVariablesTraverser::setFieldOrVariableProperties(const TType &type,
+                                                             bool staticUse,
                                                              ShaderVariable *variableOut) const
 {
     ASSERT(variableOut);
 
+    variableOut->staticUse = staticUse;
+
     const TStructure *structure = type.getStruct();
     if (!structure)
     {
@@ -582,7 +586,7 @@ void CollectVariablesTraverser::setFieldOrVariableProperties(const TType &type,
             // Regardless of the variable type (uniform, in/out etc.) its fields are always plain
             // ShaderVariable objects.
             ShaderVariable fieldVariable;
-            setFieldProperties(*field->type(), field->name(), &fieldVariable);
+            setFieldProperties(*field->type(), field->name(), staticUse, &fieldVariable);
             variableOut->fields.push_back(fieldVariable);
         }
     }
@@ -594,10 +598,11 @@ void CollectVariablesTraverser::setFieldOrVariableProperties(const TType &type,
 
 void CollectVariablesTraverser::setFieldProperties(const TType &type,
                                                    const ImmutableString &name,
+                                                   bool staticUse,
                                                    ShaderVariable *variableOut) const
 {
     ASSERT(variableOut);
-    setFieldOrVariableProperties(type, variableOut);
+    setFieldOrVariableProperties(type, staticUse, variableOut);
     variableOut->name.assign(name.data(), name.length());
     variableOut->mappedName = HashName(name, mHashFunction, nullptr).data();
 }
@@ -608,7 +613,8 @@ void CollectVariablesTraverser::setCommonVariableProperties(const TType &type,
 {
     ASSERT(variableOut);
 
-    setFieldOrVariableProperties(type, variableOut);
+    variableOut->staticUse = mSymbolTable->isStaticallyUsed(variable);
+    setFieldOrVariableProperties(type, variableOut->staticUse, variableOut);
     ASSERT(variable.symbolType() != SymbolType::Empty);
     variableOut->name.assign(variable.name().data(), variable.name().length());
     variableOut->mappedName = getMappedName(&variable);
@@ -684,6 +690,18 @@ void CollectVariablesTraverser::recordInterfaceBlock(const char *instanceName,
     if (instanceName != nullptr)
     {
         interfaceBlock->instanceName = instanceName;
+        const TSymbol *blockSymbol   = nullptr;
+        if (strncmp(instanceName, "gl_in", 5u) == 0)
+        {
+            blockSymbol = mSymbolTable->getGlInVariableWithArraySize();
+        }
+        else
+        {
+            blockSymbol = mSymbolTable->findGlobal(ImmutableString(instanceName));
+        }
+        ASSERT(blockSymbol && blockSymbol->isVariable());
+        interfaceBlock->staticUse =
+            mSymbolTable->isStaticallyUsed(*static_cast<const TVariable *>(blockSymbol));
     }
     ASSERT(!interfaceBlockType.isArrayOfArrays());  // Disallowed by GLSL ES 3.10 section 4.3.9
     interfaceBlock->arraySize = interfaceBlockType.isArray() ? interfaceBlockType.getOutermostArraySize() : 0;
@@ -699,16 +717,36 @@ void CollectVariablesTraverser::recordInterfaceBlock(const char *instanceName,
     }
 
     // Gather field information
+    bool anyFieldStaticallyUsed = false;
     for (const TField *field : blockType->fields())
     {
         const TType &fieldType = *field->type();
 
+        bool staticUse = false;
+        if (instanceName == nullptr)
+        {
+            // Static use of individual fields has been recorded, since they are present in the
+            // symbol table as variables.
+            const TSymbol *fieldSymbol = mSymbolTable->findGlobal(field->name());
+            ASSERT(fieldSymbol && fieldSymbol->isVariable());
+            staticUse =
+                mSymbolTable->isStaticallyUsed(*static_cast<const TVariable *>(fieldSymbol));
+            if (staticUse)
+            {
+                anyFieldStaticallyUsed = true;
+            }
+        }
+
         InterfaceBlockField fieldVariable;
-        setFieldProperties(fieldType, field->name(), &fieldVariable);
+        setFieldProperties(fieldType, field->name(), staticUse, &fieldVariable);
         fieldVariable.isRowMajorLayout =
             (fieldType.getLayoutQualifier().matrixPacking == EmpRowMajor);
         interfaceBlock->fields.push_back(fieldVariable);
     }
+    if (anyFieldStaticallyUsed)
+    {
+        interfaceBlock->staticUse = true;
+    }
 }
 
 Uniform CollectVariablesTraverser::recordUniform(const TIntermSymbol &variable) const
@@ -826,7 +864,7 @@ bool CollectVariablesTraverser::visitBinary(Visit, TIntermBinary *binaryNode)
 {
     if (binaryNode->getOp() == EOpIndexDirectInterfaceBlock)
     {
-        // NOTE: we do not determine static use for individual blocks of an array
+        // NOTE: we do not determine static use / activeness for individual blocks of an array.
         TIntermTyped *blockNode = binaryNode->getLeft()->getAsTyped();
         ASSERT(blockNode);
 
@@ -860,10 +898,13 @@ bool CollectVariablesTraverser::visitBinary(Visit, TIntermBinary *binaryNode)
             namedBlock = findNamedInterfaceBlock(interfaceBlock->name());
         }
         ASSERT(namedBlock);
-        namedBlock->staticUse   = true;
+        ASSERT(namedBlock->staticUse);
         namedBlock->active      = true;
         unsigned int fieldIndex = static_cast<unsigned int>(constantUnion->getIConst(0));
         ASSERT(fieldIndex < namedBlock->fields.size());
+        // TODO(oetuaho): Would be nicer to record static use of fields of named interface blocks
+        // more accurately at parse time - now we only mark the fields statically used if they are
+        // active. http://anglebug.com/2440
         namedBlock->fields[fieldIndex].staticUse = true;
         namedBlock->fields[fieldIndex].active    = true;
 

src/compiler/translator/ParseContext.h

@@ -419,6 +419,8 @@ class TParseContext : angle::NonCopyable
     TIntermBranch *addBranch(TOperator op, const TSourceLoc &loc);
     TIntermBranch *addBranch(TOperator op, TIntermTyped *expression, const TSourceLoc &loc);
 
+    void appendStatement(TIntermBlock *block, TIntermNode *statement);
+
     void checkTextureGather(TIntermAggregate *functionCall);
     void checkTextureOffsetConst(TIntermAggregate *functionCall);
     void checkImageMemoryAccessForBuiltinFunctions(TIntermAggregate *functionCall);
@@ -464,6 +466,8 @@ class TParseContext : angle::NonCopyable
     // Note that there may be tests in AtomicCounter_test that will need to be updated as well.
     constexpr static size_t kAtomicCounterArrayStride = 4;
 
+    void markStaticReadIfSymbol(TIntermNode *node);
+
     // Returns a clamped index. If it prints out an error message, the token is "[]".
     int checkIndexLessThan(bool outOfRangeIndexIsError,
                            const TSourceLoc &location,
@@ -638,10 +642,6 @@ class TParseContext : angle::NonCopyable
     int mGeometryShaderMaxVertices;
     int mMaxGeometryShaderInvocations;
     int mMaxGeometryShaderMaxVertices;
-
-    // Store gl_in variable with its array size once the array size can be determined. The array
-    // size can also be checked against latter input primitive type declaration.
-    const TVariable *mGlInVariableWithArraySize;
 };
 
 int PaParseStrings(size_t count,

src/compiler/translator/SymbolTable.cpp

@@ -79,7 +79,8 @@ TSymbol *TSymbolTable::TSymbolTableLevel::find(const ImmutableString &name) cons
         return (*it).second;
 }
 
-TSymbolTable::TSymbolTable() : mUniqueIdCounter(0), mShaderType(GL_FRAGMENT_SHADER)
+TSymbolTable::TSymbolTable()
+    : mUniqueIdCounter(0), mShaderType(GL_FRAGMENT_SHADER), mGlInVariableWithArraySize(nullptr)
 {
 }
 
@@ -137,6 +138,56 @@ const TFunction *TSymbolTable::setFunctionParameterNamesFromDefinition(const TFu
     return firstDeclaration;
 }
 
+bool TSymbolTable::setGlInArraySize(unsigned int inputArraySize)
+{
+    if (mGlInVariableWithArraySize)
+    {
+        return mGlInVariableWithArraySize->getType().getOutermostArraySize() == inputArraySize;
+    }
+    const TInterfaceBlock *glPerVertex = mVar_gl_PerVertex;
+    TType *glInType = new TType(glPerVertex, EvqPerVertexIn, TLayoutQualifier::Create());
+    glInType->makeArray(inputArraySize);
+    mGlInVariableWithArraySize =
+        new TVariable(this, ImmutableString("gl_in"), glInType, SymbolType::BuiltIn,
+                      TExtension::EXT_geometry_shader);
+    return true;
+}
+
+TVariable *TSymbolTable::getGlInVariableWithArraySize() const
+{
+    return mGlInVariableWithArraySize;
+}
+
+void TSymbolTable::markStaticWrite(const TVariable &variable)
+{
+    int id    = variable.uniqueId().get();
+    auto iter = mVariableMetadata.find(id);
+    if (iter == mVariableMetadata.end())
+    {
+        iter = mVariableMetadata.insert(std::make_pair(id, VariableMetadata())).first;
+    }
+    iter->second.staticWrite = true;
+}
+
+void TSymbolTable::markStaticRead(const TVariable &variable)
+{
+    int id    = variable.uniqueId().get();
+    auto iter = mVariableMetadata.find(id);
+    if (iter == mVariableMetadata.end())
+    {
+        iter = mVariableMetadata.insert(std::make_pair(id, VariableMetadata())).first;
+    }
+    iter->second.staticRead = true;
+}
+
+bool TSymbolTable::isStaticallyUsed(const TVariable &variable) const
+{
+    ASSERT(!variable.getConstPointer());
+    int id    = variable.uniqueId().get();
+    auto iter = mVariableMetadata.find(id);
+    return iter != mVariableMetadata.end();
+}
+
 const TSymbol *TSymbolTable::find(const ImmutableString &name, int shaderVersion) const
 {
     int userDefinedLevel = static_cast<int>(mTable.size()) - 1;
@@ -238,6 +289,8 @@ void TSymbolTable::setGlobalInvariant(bool invariant)
 void TSymbolTable::clearCompilationResults()
 {
     mUniqueIdCounter = kLastBuiltInId + 1;
+    mVariableMetadata.clear();
+    mGlInVariableWithArraySize = nullptr;
 
     // User-defined scopes should have already been cleared when the compilation finished.
     ASSERT(mTable.size() == 0u);
@@ -297,4 +350,8 @@ void TSymbolTable::initSamplerDefaultPrecision(TBasicType samplerType)
     setDefaultPrecision(samplerType, EbpLow);
 }
 
+TSymbolTable::VariableMetadata::VariableMetadata() : staticRead(false), staticWrite(false)
+{
+}
+
 }  // namespace sh

src/compiler/translator/SymbolTable.h

@@ -90,6 +90,16 @@ class TSymbolTable : angle::NonCopyable, TSymbolTableBase
     const TFunction *setFunctionParameterNamesFromDefinition(const TFunction *function,
                                                              bool *wasDefinedOut);
 
+    // Return false if the gl_in array size has already been initialized with a mismatching value.
+    bool setGlInArraySize(unsigned int inputArraySize);
+    TVariable *getGlInVariableWithArraySize() const;
+
+    void markStaticRead(const TVariable &variable);
+    void markStaticWrite(const TVariable &variable);
+
+    // Note: Should not call this for constant variables.
+    bool isStaticallyUsed(const TVariable &variable) const;
+
     // find() is guaranteed not to retain a reference to the ImmutableString, so an ImmutableString
     // with a reference to a short-lived char * is fine to pass here.
     const TSymbol *find(const ImmutableString &name, int shaderVersion) const;
@@ -154,6 +164,20 @@ class TSymbolTable : angle::NonCopyable, TSymbolTableBase
 
     sh::GLenum mShaderType;
     ShBuiltInResources mResources;
+
+    struct VariableMetadata
+    {
+        VariableMetadata();
+        bool staticRead;
+        bool staticWrite;
+    };
+
+    // Indexed by unique id. Map instead of vector since the variables are fairly sparse.
+    std::map<int, VariableMetadata> mVariableMetadata;
+
+    // Store gl_in variable with its array size once the array size can be determined. The array
+    // size can also be checked against latter input primitive type declaration.
+    TVariable *mGlInVariableWithArraySize;
 };
 
 }  // namespace sh

src/compiler/translator/glslang.y

@@ -1304,11 +1304,11 @@ compound_statement_no_new_scope
 statement_list
     : statement {
         $$ = new TIntermBlock();
-        $$->appendStatement($1);
+        context->appendStatement($$, $1);
     }
     | statement_list statement {
         $$ = $1;
-        $$->appendStatement($2);
+        context->appendStatement($$, $2);
     }
     ;
 

src/compiler/translator/glslang_tab.cpp

@@ -4630,7 +4630,7 @@ yyreduce:
 
     {
         (yyval.interm.intermBlock) = new TIntermBlock();
-        (yyval.interm.intermBlock)->appendStatement((yyvsp[0].interm.intermNode));
+        context->appendStatement((yyval.interm.intermBlock), (yyvsp[0].interm.intermNode));
     }
 
     break;
@@ -4639,7 +4639,7 @@ yyreduce:
 
     {
         (yyval.interm.intermBlock) = (yyvsp[-1].interm.intermBlock);
-        (yyval.interm.intermBlock)->appendStatement((yyvsp[0].interm.intermNode));
+        context->appendStatement((yyval.interm.intermBlock), (yyvsp[0].interm.intermNode));
     }
 
     break;

src/libANGLE/VaryingPacking.cpp

@@ -292,13 +292,13 @@ bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
         const sh::Varying *output = ref.second.fragment;
 
         // Only pack statically used varyings that have a matched input or output, plus special
-        // builtins. Note that we pack all statically used varyings even if they are not active.
-        // GLES specs are a bit vague on whether it's allowed to only pack active varyings, though
-        // GLES 3.1 spec section 11.1.2.1 says that "device-dependent optimizations" may be used to
-        // make vertex shader outputs fit.
+        // builtins. Note that we pack all statically used user-defined varyings even if they are
+        // not active. GLES specs are a bit vague on whether it's allowed to only pack active
+        // varyings, though GLES 3.1 spec section 11.1.2.1 says that "device-dependent
+        // optimizations" may be used to make vertex shader outputs fit.
         if ((input && output && output->staticUse) ||
-            (input && input->isBuiltIn() && input->staticUse) ||
-            (output && output->isBuiltIn() && output->staticUse))
+            (input && input->isBuiltIn() && input->active) ||
+            (output && output->isBuiltIn() && output->active))
         {
             const sh::Varying *varying = output ? output : input;
 

src/libANGLE/renderer/d3d/DynamicHLSL.cpp

@@ -793,8 +793,8 @@ void DynamicHLSL::generateShaderLinkHLSL(const gl::Context *context,
         ASSERT(!varying.isBuiltIn() && !varying.isStruct());
 
         // Don't reference VS-only transform feedback varyings in the PS. Note that we're relying on
-        // that the staticUse flag is set according to usage in the fragment shader.
-        if (packedVarying.vertexOnly || !varying.staticUse)
+        // that the active flag is set according to usage in the fragment shader.
+        if (packedVarying.vertexOnly || !varying.active)
             continue;
 
         pixelStream << "    ";

src/tests/compiler_tests/ShaderValidation_test.cpp

@@ -5884,3 +5884,38 @@ TEST_F(VertexShaderValidationTest, LValueSwizzleDuplicateComponents)
         FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
     }
 }
+
+// Test that a fragment shader with nested if statements without braces compiles successfully.
+TEST_F(FragmentShaderValidationTest, HandleIfInnerIfStatementAlwaysTriviallyPruned)
+{
+    const std::string &shaderString =
+        R"(precision mediump float;
+        void main()
+        {
+            if (true)
+                if (false)
+                    gl_FragColor = vec4(0.0);
+        })";
+    if (!compile(shaderString))
+    {
+        FAIL() << "Shader compilation failed, expecting success:\n" << mInfoLog;
+    }
+}
+
+// Test that a fragment shader with an if statement nested in a loop without braces compiles
+// successfully.
+TEST_F(FragmentShaderValidationTest, HandleLoopInnerIfStatementAlwaysTriviallyPruned)
+{
+    const std::string &shaderString =
+        R"(precision mediump float;
+        void main()
+        {
+            while (false)
+                if (false)
+                    gl_FragColor = vec4(0.0);
+        })";
+    if (!compile(shaderString))
+    {
+        FAIL() << "Shader compilation failed, expecting success:\n" << mInfoLog;
+    }
+}

src/tests/gl_tests/GLSLTest.cpp

@@ -4148,6 +4148,66 @@ TEST_P(GLSLTest_ES3, ErrorMessageOfLinkInterfaceBlockStructFieldMismatch)
                                      "interface block 'S' member 'S.val1.t2'");
 }
 
+// Test a vertex shader that doesn't declare any varyings with a fragment shader that statically
+// uses a varying, but in a statement that gets trivially optimized out by the compiler.
+TEST_P(GLSLTest_ES3, FragmentShaderStaticallyUsesVaryingMissingFromVertex)
+{
+    const std::string &vertexShader =
+        R"(#version 300 es
+
+        precision mediump float;
+
+        void main()
+        {
+            gl_Position = vec4(0, 1, 0, 1);
+        })";
+
+    const std::string &fragmentShader =
+        R"(#version 300 es
+
+        precision mediump float;
+        in float foo;
+        out vec4 my_FragColor;
+
+        void main()
+        {
+            if (false)
+            {
+                float unreferenced = foo;
+            }
+            my_FragColor = vec4(0, 1, 0, 1);
+        })";
+
+    validateComponentsInErrorMessage(vertexShader, fragmentShader, "does not match any", "foo");
+}
+
+// Test a varying that is statically used but not active in the fragment shader.
+TEST_P(GLSLTest_ES3, VaryingStaticallyUsedButNotActiveInFragmentShader)
+{
+    const std::string &vertexShader =
+        R"(#version 300 es
+        precision mediump float;
+        in vec4 iv;
+        out vec4 v;
+        void main()
+        {
+            gl_Position = iv;
+            v = iv;
+        })";
+
+    const std::string &fragmentShader =
+        R"(#version 300 es
+        precision mediump float;
+        in vec4 v;
+        out vec4 color;
+        void main()
+        {
+            color = true ? vec4(0.0) : v;
+        })";
+
+    ANGLE_GL_PROGRAM(program, vertexShader, fragmentShader);
+}
+
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these tests should be run against.
 ANGLE_INSTANTIATE_TEST(GLSLTest,
                        ES2_D3D9(),