Fold ternary and comma ops only after parsing is done
In case folding a ternary op or a comma op would change the qualifier
of the expression, the folding is deferred to a separate traversal
step.
After this there are no more cases where the type of a TIntermSymbol
node needs to differ from the type of the variable it is referring to.
There are still some cases where some parts of TIntermSymbol type are
changed while keeping the TVariable type the same though, like when
assigning array size to gl_PerVertex nodes or sanitizing qualifiers of
struct declarations.
BUG=angleproject:2267
TEST=angle_unittests, angle_end2end_tests
Change-Id: I1501c8d361f5f765f43ca810d1b7248d9e2c5986
Reviewed-on: https://chromium-review.googlesource.com/850672
Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/compiler/translator/Compiler.cpp b/src/compiler/translator/Compiler.cpp
index c4ded7a..a50b125 100644
--- a/src/compiler/translator/Compiler.cpp
+++ b/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 @@
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 @@
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))
diff --git a/src/compiler/translator/FoldExpressions.cpp b/src/compiler/translator/FoldExpressions.cpp
new file mode 100644
index 0000000..71e04d0
--- /dev/null
+++ b/src/compiler/translator/FoldExpressions.cpp
@@ -0,0 +1,116 @@
+//
+// 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
diff --git a/src/compiler/translator/FoldExpressions.h b/src/compiler/translator/FoldExpressions.h
new file mode 100644
index 0000000..357547a
--- /dev/null
+++ b/src/compiler/translator/FoldExpressions.h
@@ -0,0 +1,24 @@
+//
+// 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_
diff --git a/src/compiler/translator/IntermNode.cpp b/src/compiler/translator/IntermNode.cpp
index 1f5dfa8..9bbd4e2 100644
--- a/src/compiler/translator/IntermNode.cpp
+++ b/src/compiler/translator/IntermNode.cpp
@@ -138,6 +138,42 @@
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 @@
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 @@
}
}
-TIntermTyped *TIntermSwizzle::fold()
+TIntermTyped *TIntermSwizzle::fold(TDiagnostics * /* diagnostics */)
{
TIntermConstantUnion *operandConstant = mOperand->getAsConstantUnion();
if (operandConstant == nullptr)
@@ -1308,7 +1342,6 @@
{
return this;
}
- mRight->getTypePointer()->setQualifier(mType.getQualifier());
return mRight;
}
case EOpIndexDirect:
@@ -1432,13 +1465,20 @@
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 @@
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)
diff --git a/src/compiler/translator/IntermNode.h b/src/compiler/translator/IntermNode.h
index bea6ca2..79dc3d6 100644
--- a/src/compiler/translator/IntermNode.h
+++ b/src/compiler/translator/IntermNode.h
@@ -129,6 +129,8 @@
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 @@
// 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 @@
bool hasDuplicateOffsets() const;
bool offsetsMatch(int offset) const;
- TIntermTyped *fold();
+ TIntermTyped *fold(TDiagnostics *diagnostics) override;
protected:
TIntermTyped *mOperand;
@@ -448,7 +447,7 @@
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 @@
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 @@
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 @@
mFalseExpression->hasSideEffects();
}
- TIntermTyped *fold();
+ TIntermTyped *fold(TDiagnostics *diagnostics) override;
private:
TIntermTernary(const TIntermTernary &node); // Note: not deleted, just private!
diff --git a/src/compiler/translator/ParseContext.cpp b/src/compiler/translator/ParseContext.cpp
index c00a77b..748b523 100644
--- a/src/compiler/translator/ParseContext.cpp
+++ b/src/compiler/translator/ParseContext.cpp
@@ -3605,12 +3605,7 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
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);
}
//
diff --git a/src/compiler/translator/ParseContext.h b/src/compiler/translator/ParseContext.h
index 9e713a0..443da5b 100644
--- a/src/compiler/translator/ParseContext.h
+++ b/src/compiler/translator/ParseContext.h
@@ -560,6 +560,10 @@
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,