Fold ternary and comma ops only after parsing is done

src/compiler.gypi

@@ -65,6 +65,8 @@
             'compiler/translator/FindSymbolNode.h',
             'compiler/translator/FlagStd140Structs.cpp',
             'compiler/translator/FlagStd140Structs.h',
+            'compiler/translator/FoldExpressions.cpp',
+            'compiler/translator/FoldExpressions.h',
             'compiler/translator/HashNames.cpp',
             'compiler/translator/HashNames.h',
             'compiler/translator/InfoSink.cpp',

src/compiler/translator/Compiler.cpp

@@ -18,6 +18,7 @@
 #include "compiler/translator/DeferGlobalInitializers.h"
 #include "compiler/translator/EmulateGLFragColorBroadcast.h"
 #include "compiler/translator/EmulatePrecision.h"
+#include "compiler/translator/FoldExpressions.h"
 #include "compiler/translator/Initialize.h"
 #include "compiler/translator/InitializeVariables.h"
 #include "compiler/translator/IntermNodePatternMatcher.h"
@@ -399,6 +400,18 @@ bool TCompiler::checkAndSimplifyAST(TIntermBlock *root,
         return false;
     }
 
+    if (shouldRunLoopAndIndexingValidation(compileOptions) &&
+        !ValidateLimitations(root, shaderType, &symbolTable, shaderVersion, &mDiagnostics))
+    {
+        return false;
+    }
+
+    // Fold expressions that could not be folded before validation that was done as a part of
+    // parsing.
+    FoldExpressions(root, &mDiagnostics);
+    // Folding should only be able to generate warnings.
+    ASSERT(mDiagnostics.numErrors() == 0);
+
     // We prune no-ops to work around driver bugs and to keep AST processing and output simple.
     // The following kinds of no-ops are pruned:
     //   1. Empty declarations "int;".
@@ -454,12 +467,6 @@ bool TCompiler::checkAndSimplifyAST(TIntermBlock *root,
         return false;
     }
 
-    if (shouldRunLoopAndIndexingValidation(compileOptions) &&
-        !ValidateLimitations(root, shaderType, &symbolTable, shaderVersion, &mDiagnostics))
-    {
-        return false;
-    }
-
     // Fail compilation if precision emulation not supported.
     if (getResources().WEBGL_debug_shader_precision && getPragma().debugShaderPrecision &&
         !EmulatePrecision::SupportedInLanguage(outputType))

src/compiler/translator/FoldExpressions.cpp

+//
+// Copyright (c) 2018 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.
+//
+// FoldExpressions.cpp: Fold expressions. This may fold expressions so that the qualifier of the
+// folded node differs from the qualifier of the original expression, so it needs to be done after
+// parsing and validation of qualifiers is complete. Expressions that are folded:
+//  1. Ternary ops with a constant condition.
+//  2. Sequence aka comma ops where the left side has no side effects.
+//  3. Any expressions containing any of the above.
+
+#include "compiler/translator/FoldExpressions.h"
+
+#include "compiler/translator/Diagnostics.h"
+#include "compiler/translator/IntermNode.h"
+#include "compiler/translator/IntermTraverse.h"
+
+namespace sh
+{
+
+namespace
+{
+
+class FoldExpressionsTraverser : public TIntermTraverser
+{
+  public:
+    FoldExpressionsTraverser(TDiagnostics *diagnostics)
+        : TIntermTraverser(true, false, false), mDiagnostics(diagnostics), mDidReplace(false)
+    {
+    }
+
+    bool didReplace() { return mDidReplace; }
+
+    void nextIteration() { mDidReplace = false; }
+
+  protected:
+    bool visitTernary(Visit visit, TIntermTernary *node) override
+    {
+        TIntermTyped *folded = node->fold(mDiagnostics);
+        if (folded != node)
+        {
+            queueReplacement(folded, OriginalNode::IS_DROPPED);
+            mDidReplace = true;
+            return false;
+        }
+        return true;
+    }
+
+    bool visitAggregate(Visit visit, TIntermAggregate *node) override
+    {
+        TIntermTyped *folded = node->fold(mDiagnostics);
+        if (folded != node)
+        {
+            queueReplacement(folded, OriginalNode::IS_DROPPED);
+            mDidReplace = true;
+            return false;
+        }
+        return true;
+    }
+
+    bool visitBinary(Visit visit, TIntermBinary *node) override
+    {
+        TIntermTyped *folded = node->fold(mDiagnostics);
+        if (folded != node)
+        {
+            queueReplacement(folded, OriginalNode::IS_DROPPED);
+            mDidReplace = true;
+            return false;
+        }
+        return true;
+    }
+
+    bool visitUnary(Visit visit, TIntermUnary *node) override
+    {
+        TIntermTyped *folded = node->fold(mDiagnostics);
+        if (folded != node)
+        {
+            queueReplacement(folded, OriginalNode::IS_DROPPED);
+            mDidReplace = true;
+            return false;
+        }
+        return true;
+    }
+
+    bool visitSwizzle(Visit visit, TIntermSwizzle *node) override
+    {
+        TIntermTyped *folded = node->fold(mDiagnostics);
+        if (folded != node)
+        {
+            queueReplacement(folded, OriginalNode::IS_DROPPED);
+            mDidReplace = true;
+            return false;
+        }
+        return true;
+    }
+
+  private:
+    TDiagnostics *mDiagnostics;
+    bool mDidReplace;
+};
+
+}  // anonymous namespace
+
+void FoldExpressions(TIntermBlock *root, TDiagnostics *diagnostics)
+{
+    FoldExpressionsTraverser traverser(diagnostics);
+    do
+    {
+        traverser.nextIteration();
+        root->traverse(&traverser);
+        traverser.updateTree();
+    } while (traverser.didReplace());
+}
+
+}  // namespace sh

src/compiler/translator/FoldExpressions.h

+//
+// Copyright (c) 2018 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.
+//
+// FoldExpressions.h: Fold expressions. This may fold expressions so that the qualifier of the
+// folded node differs from the qualifier of the original expression, so it needs to be done after
+// parsing and validation of qualifiers is complete. Expressions that are folded: 1. Ternary ops
+// with a constant condition.
+
+#ifndef COMPILER_TRANSLATOR_FOLDEXPRESSIONS_H_
+#define COMPILER_TRANSLATOR_FOLDEXPRESSIONS_H_
+
+namespace sh
+{
+
+class TIntermBlock;
+class TDiagnostics;
+
+void FoldExpressions(TIntermBlock *root, TDiagnostics *diagnostics);
+
+}  // namespace sh
+
+#endif  // COMPILER_TRANSLATOR_FOLDEXPRESSIONS_H_

src/compiler/translator/IntermNode.cpp

@@ -138,6 +138,42 @@ void SetUnionArrayFromMatrix(const angle::Matrix<float> &m, TConstantUnion *resu
         resultArray[i].setFConst(resultElements[i]);
 }
 
+bool CanFoldAggregateBuiltInOp(TOperator op)
+{
+    switch (op)
+    {
+        case EOpAtan:
+        case EOpPow:
+        case EOpMod:
+        case EOpMin:
+        case EOpMax:
+        case EOpClamp:
+        case EOpMix:
+        case EOpStep:
+        case EOpSmoothStep:
+        case EOpLdexp:
+        case EOpMulMatrixComponentWise:
+        case EOpOuterProduct:
+        case EOpEqualComponentWise:
+        case EOpNotEqualComponentWise:
+        case EOpLessThanComponentWise:
+        case EOpLessThanEqualComponentWise:
+        case EOpGreaterThanComponentWise:
+        case EOpGreaterThanEqualComponentWise:
+        case EOpDistance:
+        case EOpDot:
+        case EOpCross:
+        case EOpFaceforward:
+        case EOpReflect:
+        case EOpRefract:
+        case EOpBitfieldExtract:
+        case EOpBitfieldInsert:
+            return true;
+        default:
+            return false;
+    }
+}
+
 }  // namespace anonymous
 
 ////////////////////////////////////////////////////////////////
@@ -958,18 +994,16 @@ TQualifier TIntermTernary::DetermineQualifier(TIntermTyped *cond,
     return EvqTemporary;
 }
 
-TIntermTyped *TIntermTernary::fold()
+TIntermTyped *TIntermTernary::fold(TDiagnostics * /* diagnostics */)
 {
     if (mCondition->getAsConstantUnion())
     {
         if (mCondition->getAsConstantUnion()->getBConst(0))
         {
-            mTrueExpression->getTypePointer()->setQualifier(mType.getQualifier());
             return mTrueExpression;
         }
         else
         {
-            mFalseExpression->getTypePointer()->setQualifier(mType.getQualifier());
             return mFalseExpression;
         }
     }
@@ -1280,7 +1314,7 @@ const TConstantUnion *TIntermConstantUnion::foldIndexing(int index)
     }
 }
 
-TIntermTyped *TIntermSwizzle::fold()
+TIntermTyped *TIntermSwizzle::fold(TDiagnostics * /* diagnostics */)
 {
     TIntermConstantUnion *operandConstant = mOperand->getAsConstantUnion();
     if (operandConstant == nullptr)
@@ -1308,7 +1342,6 @@ TIntermTyped *TIntermBinary::fold(TDiagnostics *diagnostics)
             {
                 return this;
             }
-            mRight->getTypePointer()->setQualifier(mType.getQualifier());
             return mRight;
         }
         case EOpIndexDirect:
@@ -1432,13 +1465,20 @@ TIntermTyped *TIntermAggregate::fold(TDiagnostics *diagnostics)
     TConstantUnion *constArray = nullptr;
     if (isConstructor())
     {
-        constArray = TIntermConstantUnion::FoldAggregateConstructor(this);
+        // TODO(oetuaho@nvidia.com): Add support for folding array constructors.
+        if (!isArray())
+        {
+            constArray = TIntermConstantUnion::FoldAggregateConstructor(this);
+        }
     }
-    else
+    else if (CanFoldAggregateBuiltInOp(mOp))
     {
-        ASSERT(CanFoldAggregateBuiltInOp(mOp));
         constArray = TIntermConstantUnion::FoldAggregateBuiltIn(this, diagnostics);
     }
+    if (constArray == nullptr)
+    {
+        return this;
+    }
     return CreateFoldedNode(constArray, this);
 }
 
@@ -2585,42 +2625,6 @@ TConstantUnion *TIntermConstantUnion::FoldAggregateConstructor(TIntermAggregate
     return resultArray;
 }
 
-bool TIntermAggregate::CanFoldAggregateBuiltInOp(TOperator op)
-{
-    switch (op)
-    {
-        case EOpAtan:
-        case EOpPow:
-        case EOpMod:
-        case EOpMin:
-        case EOpMax:
-        case EOpClamp:
-        case EOpMix:
-        case EOpStep:
-        case EOpSmoothStep:
-        case EOpLdexp:
-        case EOpMulMatrixComponentWise:
-        case EOpOuterProduct:
-        case EOpEqualComponentWise:
-        case EOpNotEqualComponentWise:
-        case EOpLessThanComponentWise:
-        case EOpLessThanEqualComponentWise:
-        case EOpGreaterThanComponentWise:
-        case EOpGreaterThanEqualComponentWise:
-        case EOpDistance:
-        case EOpDot:
-        case EOpCross:
-        case EOpFaceforward:
-        case EOpReflect:
-        case EOpRefract:
-        case EOpBitfieldExtract:
-        case EOpBitfieldInsert:
-            return true;
-        default:
-            return false;
-    }
-}
-
 // static
 TConstantUnion *TIntermConstantUnion::FoldAggregateBuiltIn(TIntermAggregate *aggregate,
                                                            TDiagnostics *diagnostics)

src/compiler/translator/IntermNode.h

@@ -129,6 +129,8 @@ class TIntermTyped : public TIntermNode
 
     TIntermTyped *getAsTyped() override { return this; }
 
+    virtual TIntermTyped *fold(TDiagnostics *diagnostics) { return this; }
+
     // True if executing the expression represented by this node affects state, like values of
     // variables. False if the executing the expression only computes its return value without
     // affecting state. May return true conservatively.
@@ -231,10 +233,7 @@ class TIntermBranch : public TIntermNode
 
 // Nodes that correspond to variable symbols in the source code. These may be regular variables or
 // interface block instances. In declarations that only declare a struct type but no variables, a
-// TIntermSymbol node with an empty variable is used to store the type. In case the node is the
-// result of folding a more complex expression such as a ternary operator, the node takes on the
-// type of the expression. In this case the qualifier of the node may be different from the variable
-// it refers to.
+// TIntermSymbol node with an empty variable is used to store the type.
 class TIntermSymbol : public TIntermTyped
 {
   public:
@@ -408,7 +407,7 @@ class TIntermSwizzle : public TIntermTyped
     bool hasDuplicateOffsets() const;
     bool offsetsMatch(int offset) const;
 
-    TIntermTyped *fold();
+    TIntermTyped *fold(TDiagnostics *diagnostics) override;
 
   protected:
     TIntermTyped *mOperand;
@@ -448,7 +447,7 @@ class TIntermBinary : public TIntermOperator
 
     TIntermTyped *getLeft() const { return mLeft; }
     TIntermTyped *getRight() const { return mRight; }
-    TIntermTyped *fold(TDiagnostics *diagnostics);
+    TIntermTyped *fold(TDiagnostics *diagnostics) override;
 
     void setAddIndexClamp() { mAddIndexClamp = true; }
     bool getAddIndexClamp() { return mAddIndexClamp; }
@@ -483,7 +482,7 @@ class TIntermUnary : public TIntermOperator
     bool hasSideEffects() const override { return isAssignment() || mOperand->hasSideEffects(); }
 
     TIntermTyped *getOperand() { return mOperand; }
-    TIntermTyped *fold(TDiagnostics *diagnostics);
+    TIntermTyped *fold(TDiagnostics *diagnostics) override;
 
     void setUseEmulatedFunction() { mUseEmulatedFunction = true; }
     bool getUseEmulatedFunction() { return mUseEmulatedFunction; }
@@ -560,8 +559,7 @@ class TIntermAggregate : public TIntermOperator, public TIntermAggregateBase
 
     bool hasSideEffects() const override;
 
-    static bool CanFoldAggregateBuiltInOp(TOperator op);
-    TIntermTyped *fold(TDiagnostics *diagnostics);
+    TIntermTyped *fold(TDiagnostics *diagnostics) override;
 
     TIntermSequence *getSequence() override { return &mArguments; }
     const TIntermSequence *getSequence() const override { return &mArguments; }
@@ -760,7 +758,7 @@ class TIntermTernary : public TIntermTyped
                mFalseExpression->hasSideEffects();
     }
 
-    TIntermTyped *fold();
+    TIntermTyped *fold(TDiagnostics *diagnostics) override;
 
   private:
     TIntermTernary(const TIntermTernary &node);  // Note: not deleted, just private!

src/compiler/translator/ParseContext.cpp

@@ -3605,12 +3605,7 @@ TIntermTyped *TParseContext::addConstructor(TIntermSequence *arguments,
     TIntermAggregate *constructorNode = TIntermAggregate::CreateConstructor(type, arguments);
     constructorNode->setLine(line);
 
-    // TODO(oetuaho@nvidia.com): Add support for folding array constructors.
-    if (!constructorNode->isArray())
-    {
-        return constructorNode->fold(mDiagnostics);
-    }
-    return constructorNode;
+    return constructorNode->fold(mDiagnostics);
 }
 
 //
@@ -4125,7 +4120,7 @@ TIntermTyped *TParseContext::addFieldSelectionExpression(TIntermTyped *baseExpre
         TIntermSwizzle *node = new TIntermSwizzle(baseExpression, fieldOffsets);
         node->setLine(dotLocation);
 
-        return node->fold();
+        return node->fold(mDiagnostics);
     }
     else if (baseExpression->getBasicType() == EbtStruct)
     {
@@ -4977,6 +4972,30 @@ TIntermTyped *TParseContext::addUnaryMathLValue(TOperator op,
     return addUnaryMath(op, child, loc);
 }
 
+TIntermTyped *TParseContext::expressionOrFoldedResult(TIntermTyped *expression)
+{
+    // If we can, we should return the folded version of the expression for subsequent parsing. This
+    // enables folding the containing expression during parsing as well, instead of the separate
+    // FoldExpressions() step where folding nested expressions requires multiple full AST
+    // traversals.
+
+    // Even if folding fails the fold() functions return some node representing the expression,
+    // typically the original node. So "folded" can be assumed to be non-null.
+    TIntermTyped *folded = expression->fold(mDiagnostics);
+    ASSERT(folded != nullptr);
+    if (folded->getQualifier() == expression->getQualifier())
+    {
+        // We need this expression to have the correct qualifier when validating the consuming
+        // expression. So we can only return the folded node from here in case it has the same
+        // qualifier as the original expression. In this kind of a cases the qualifier of the folded
+        // node is EvqConst, whereas the qualifier of the expression is EvqTemporary:
+        //  1. (true ? 1.0 : non_constant)
+        //  2. (non_constant, 1.0)
+        return folded;
+    }
+    return expression;
+}
+
 bool TParseContext::binaryOpCommonCheck(TOperator op,
                                         TIntermTyped *left,
                                         TIntermTyped *right,
@@ -5426,7 +5445,8 @@ TIntermTyped *TParseContext::addComma(TIntermTyped *left,
     TIntermBinary *commaNode   = new TIntermBinary(EOpComma, left, right);
     TQualifier resultQualifier = TIntermBinary::GetCommaQualifier(mShaderVersion, left, right);
     commaNode->getTypePointer()->setQualifier(resultQualifier);
-    return commaNode->fold(mDiagnostics);
+
+    return expressionOrFoldedResult(commaNode);
 }
 
 TIntermBranch *TParseContext::addBranch(TOperator op, const TSourceLoc &loc)
@@ -5868,16 +5888,9 @@ TIntermTyped *TParseContext::addNonConstructorFunctionCall(const TString &name,
                     // Some built-in functions have out parameters too.
                     functionCallRValueLValueErrorCheck(fnCandidate, callNode);
 
-                    if (TIntermAggregate::CanFoldAggregateBuiltInOp(callNode->getOp()))
-                    {
-                        // See if we can constant fold a built-in. Note that this may be possible
-                        // even if it is not const-qualified.
-                        return callNode->fold(mDiagnostics);
-                    }
-                    else
-                    {
-                        return callNode;
-                    }
+                    // See if we can constant fold a built-in. Note that this may be possible
+                    // even if it is not const-qualified.
+                    return callNode->fold(mDiagnostics);
                 }
             }
             else
@@ -5979,12 +5992,9 @@ TIntermTyped *TParseContext::addTernarySelection(TIntermTyped *cond,
         return falseExpression;
     }
 
-    // Note that the node resulting from here can be a constant union without being qualified as
-    // constant.
     TIntermTernary *node = new TIntermTernary(cond, trueExpression, falseExpression);
     node->setLine(loc);
-
-    return node->fold();
+    return expressionOrFoldedResult(node);
 }
 
 //

src/compiler/translator/ParseContext.h

@@ -560,6 +560,10 @@ class TParseContext : angle::NonCopyable
                                                 TIntermSequence *arguments,
                                                 const TSourceLoc &loc);
 
+    // Return either the original expression or the folded version of the expression in case the
+    // folded node will validate the same way during subsequent parsing.
+    TIntermTyped *expressionOrFoldedResult(TIntermTyped *expression);
+
     // Return true if the checks pass
     bool binaryOpCommonCheck(TOperator op,
                              TIntermTyped *left,

src/tests/compiler_tests/ConstantFolding_test.cpp

@@ -1417,3 +1417,37 @@ TEST_F(ConstantFoldingExpressionTest, FoldLdexp)
     evaluateFloat(floatString);
     ASSERT_TRUE(constantFoundInAST(1.25f));
 }
+
+// Fold a ternary operator.
+TEST_F(ConstantFoldingTest, FoldTernary)
+{
+    const std::string &shaderString =
+        R"(#version 300 es
+        precision highp int;
+        uniform int u;
+        out int my_FragColor;
+        void main()
+        {
+            my_FragColor = (true ? 1 : u);
+        })";
+    compileAssumeSuccess(shaderString);
+    ASSERT_TRUE(constantFoundInAST(1));
+    ASSERT_FALSE(symbolFoundInMain("u"));
+}
+
+// Fold a ternary operator inside a consuming expression.
+TEST_F(ConstantFoldingTest, FoldTernaryInsideExpression)
+{
+    const std::string &shaderString =
+        R"(#version 300 es
+        precision highp int;
+        uniform int u;
+        out int my_FragColor;
+        void main()
+        {
+            my_FragColor = ivec2((true ? 1 : u) + 2, 4).x;
+        })";
+    compileAssumeSuccess(shaderString);
+    ASSERT_TRUE(constantFoundInAST(3));
+    ASSERT_FALSE(symbolFoundInMain("u"));
+}

src/tests/compiler_tests/ShaderValidation_test.cpp

@@ -5659,3 +5659,24 @@ TEST_F(WebGL1FragmentShaderValidationTest, StructNestingLimitWithNestedStructDef
         FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
     }
 }
+
+// Test that the result of a sequence operator is not a constant-expression.
+// ESSL 3.00 section 12.43.
+TEST_F(FragmentShaderValidationTest, CommaReturnsNonConstant)
+{
+    const std::string &shaderString =
+        R"(#version 300 es
+
+        precision highp float;
+        out vec4 my_FragColor;
+
+        void main(void)
+        {
+            const int i = (0, 0);
+            my_FragColor = vec4(i);
+        })";
+    if (compile(shaderString))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
+    }
+}

src/tests/test_utils/ConstantFoldingTest.h

@@ -13,6 +13,8 @@
 #include <vector>
 
 #include "common/mathutil.h"
+#include "compiler/translator/FindMain.h"
+#include "compiler/translator/FindSymbolNode.h"
 #include "compiler/translator/IntermTraverse.h"
 #include "tests/test_utils/ShaderCompileTreeTest.h"
 
@@ -169,6 +171,11 @@ class ConstantFoldingTest : public ShaderCompileTreeTest
         mASTRoot->traverse(&finder);
         return finder.found();
     }
+
+    bool symbolFoundInMain(const char *symbolName)
+    {
+        return FindSymbolNode(FindMain(mASTRoot), TString(symbolName)) != nullptr;
+    }
 };
 
 class ConstantFoldingExpressionTest : public ConstantFoldingTest