Revert "Cover vector dynamic indexing case in SplitSequenceOperator"
This CL was causing inverted rendering in a WebGL application.
This reverts commit 7da9850643f55335a13a4663d226c73d0ac4d3b1.
Vectors or matrices that are dynamically indexed as a part of an
l-value generate new statements in the RemoveDynamicIndexing AST
transformation step. SplitSequenceOperator needs to detect this case
and split the sequence operator before statements are generated from
its operands to ensure the correct order of execution.
BUG=angleproject:1341
TEST=angle_end2end_tests
Change-Id: I854f8cce2d46107afa62f48edf3d32c6d5c97eda
Reviewed-on: https://chromium-review.googlesource.com/371643
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Jamie Madill <jmadill@chromium.org>
diff --git a/src/compiler/translator/IntermNodePatternMatcher.cpp b/src/compiler/translator/IntermNodePatternMatcher.cpp
index b614d83..a0cddac 100644
--- a/src/compiler/translator/IntermNodePatternMatcher.cpp
+++ b/src/compiler/translator/IntermNodePatternMatcher.cpp
@@ -27,14 +27,7 @@
{
}
-// static
-bool IntermNodePatternMatcher::IsDynamicIndexingOfVectorOrMatrix(TIntermBinary *node)
-{
- return node->getOp() == EOpIndexIndirect && !node->getLeft()->isArray() &&
- node->getLeft()->getBasicType() != EbtStruct;
-}
-
-bool IntermNodePatternMatcher::matchInternal(TIntermBinary *node, TIntermNode *parentNode)
+bool IntermNodePatternMatcher::match(TIntermBinary *node, TIntermNode *parentNode)
{
if ((mMask & kExpressionReturningArray) != 0)
{
@@ -56,33 +49,6 @@
return false;
}
-bool IntermNodePatternMatcher::match(TIntermBinary *node, TIntermNode *parentNode)
-{
- // L-value tracking information is needed to check for dynamic indexing in L-value.
- // Traversers that don't track l-values can still use this class and match binary nodes with
- // this variation of this method if they don't need to check for dynamic indexing in l-values.
- ASSERT((mMask & kDynamicIndexingOfVectorOrMatrixInLValue) == 0);
- return matchInternal(node, parentNode);
-}
-
-bool IntermNodePatternMatcher::match(TIntermBinary *node,
- TIntermNode *parentNode,
- bool isLValueRequiredHere)
-{
- if (matchInternal(node, parentNode))
- {
- return true;
- }
- if ((mMask & kDynamicIndexingOfVectorOrMatrixInLValue) != 0)
- {
- if (isLValueRequiredHere && IsDynamicIndexingOfVectorOrMatrix(node))
- {
- return true;
- }
- }
- return false;
-}
-
bool IntermNodePatternMatcher::match(TIntermAggregate *node, TIntermNode *parentNode)
{
if ((mMask & kExpressionReturningArray) != 0)
diff --git a/src/compiler/translator/IntermNodePatternMatcher.h b/src/compiler/translator/IntermNodePatternMatcher.h
index be6fc61..b088633 100644
--- a/src/compiler/translator/IntermNodePatternMatcher.h
+++ b/src/compiler/translator/IntermNodePatternMatcher.h
@@ -19,35 +19,22 @@
class IntermNodePatternMatcher
{
public:
- static bool IsDynamicIndexingOfVectorOrMatrix(TIntermBinary *node);
-
enum PatternType
{
- // Matches expressions that are unfolded to if statements by UnfoldShortCircuitToIf
kUnfoldedShortCircuitExpression = 0x0001,
// Matches expressions that return arrays with the exception of simple statements where a
// constructor or function call result is assigned.
- kExpressionReturningArray = 0x0002,
-
- // Matches dynamic indexing of vectors or matrices in l-values.
- kDynamicIndexingOfVectorOrMatrixInLValue = 0x0004
+ kExpressionReturningArray = 0x0002
};
IntermNodePatternMatcher(const unsigned int mask);
bool match(TIntermBinary *node, TIntermNode *parentNode);
-
- // Use this version for checking binary node matches in case you're using flag
- // kDynamicIndexingOfVectorOrMatrixInLValue.
- bool match(TIntermBinary *node, TIntermNode *parentNode, bool isLValueRequiredHere);
-
bool match(TIntermAggregate *node, TIntermNode *parentNode);
bool match(TIntermSelection *node);
private:
const unsigned int mMask;
-
- bool matchInternal(TIntermBinary *node, TIntermNode *parentNode);
};
#endif
diff --git a/src/compiler/translator/RemoveDynamicIndexing.cpp b/src/compiler/translator/RemoveDynamicIndexing.cpp
index 37955e7..5b6b41b 100644
--- a/src/compiler/translator/RemoveDynamicIndexing.cpp
+++ b/src/compiler/translator/RemoveDynamicIndexing.cpp
@@ -11,7 +11,6 @@
#include "compiler/translator/InfoSink.h"
#include "compiler/translator/IntermNode.h"
-#include "compiler/translator/IntermNodePatternMatcher.h"
#include "compiler/translator/SymbolTable.h"
namespace
@@ -406,18 +405,10 @@
TIntermSymbol *tempIndex = createTempSymbol(node->getRight()->getType());
queueReplacementWithParent(node, node->getRight(), tempIndex, OriginalNode::IS_DROPPED);
}
- else if (IntermNodePatternMatcher::IsDynamicIndexingOfVectorOrMatrix(node))
+ else if (!node->getLeft()->isArray() && node->getLeft()->getBasicType() != EbtStruct)
{
bool write = isLValueRequiredHere();
-#if defined(ANGLE_ENABLE_ASSERTS)
- // Make sure that IntermNodePatternMatcher is consistent with the slightly differently
- // implemented checks in this traverser.
- IntermNodePatternMatcher matcher(
- IntermNodePatternMatcher::kDynamicIndexingOfVectorOrMatrixInLValue);
- ASSERT(matcher.match(node, getParentNode(), isLValueRequiredHere()) == write);
-#endif
-
TType type = node->getLeft()->getType();
mIndexedVecAndMatrixTypes.insert(type);
diff --git a/src/compiler/translator/SimplifyLoopConditions.cpp b/src/compiler/translator/SimplifyLoopConditions.cpp
index 61a68fc..a53adc4 100644
--- a/src/compiler/translator/SimplifyLoopConditions.cpp
+++ b/src/compiler/translator/SimplifyLoopConditions.cpp
@@ -83,7 +83,7 @@
if (!mInsideLoopConditionOrExpression)
return false;
- mFoundLoopToChange = mConditionsToSimplify.match(node, getParentNode(), isLValueRequiredHere());
+ mFoundLoopToChange = mConditionsToSimplify.match(node, getParentNode());
return !mFoundLoopToChange;
}
diff --git a/src/compiler/translator/SplitSequenceOperator.cpp b/src/compiler/translator/SplitSequenceOperator.cpp
index 3ea29b9..036ed04 100644
--- a/src/compiler/translator/SplitSequenceOperator.cpp
+++ b/src/compiler/translator/SplitSequenceOperator.cpp
@@ -17,12 +17,10 @@
namespace
{
-class SplitSequenceOperatorTraverser : public TLValueTrackingTraverser
+class SplitSequenceOperatorTraverser : public TIntermTraverser
{
public:
- SplitSequenceOperatorTraverser(unsigned int patternsToSplitMask,
- const TSymbolTable &symbolTable,
- int shaderVersion);
+ SplitSequenceOperatorTraverser(unsigned int patternsToSplitMask);
bool visitBinary(Visit visit, TIntermBinary *node) override;
bool visitAggregate(Visit visit, TIntermAggregate *node) override;
@@ -40,10 +38,8 @@
IntermNodePatternMatcher mPatternToSplitMatcher;
};
-SplitSequenceOperatorTraverser::SplitSequenceOperatorTraverser(unsigned int patternsToSplitMask,
- const TSymbolTable &symbolTable,
- int shaderVersion)
- : TLValueTrackingTraverser(true, false, true, symbolTable, shaderVersion),
+SplitSequenceOperatorTraverser::SplitSequenceOperatorTraverser(unsigned int patternsToSplitMask)
+ : TIntermTraverser(true, false, true),
mFoundExpressionToSplit(false),
mInsideSequenceOperator(0),
mPatternToSplitMatcher(patternsToSplitMask)
@@ -65,8 +61,7 @@
if (mInsideSequenceOperator > 0 && visit == PreVisit)
{
// Detect expressions that need to be simplified
- mFoundExpressionToSplit =
- mPatternToSplitMatcher.match(node, getParentNode(), isLValueRequiredHere());
+ mFoundExpressionToSplit = mPatternToSplitMatcher.match(node, getParentNode());
return !mFoundExpressionToSplit;
}
@@ -138,13 +133,9 @@
} // namespace
-void SplitSequenceOperator(TIntermNode *root,
- int patternsToSplitMask,
- unsigned int *temporaryIndex,
- const TSymbolTable &symbolTable,
- int shaderVersion)
+void SplitSequenceOperator(TIntermNode *root, int patternsToSplitMask, unsigned int *temporaryIndex)
{
- SplitSequenceOperatorTraverser traverser(patternsToSplitMask, symbolTable, shaderVersion);
+ SplitSequenceOperatorTraverser traverser(patternsToSplitMask);
ASSERT(temporaryIndex != nullptr);
traverser.useTemporaryIndex(temporaryIndex);
// Separate one expression at a time, and reset the traverser between iterations.
diff --git a/src/compiler/translator/SplitSequenceOperator.h b/src/compiler/translator/SplitSequenceOperator.h
index 4a46fe3..30e13e2 100644
--- a/src/compiler/translator/SplitSequenceOperator.h
+++ b/src/compiler/translator/SplitSequenceOperator.h
@@ -13,12 +13,9 @@
#define COMPILER_TRANSLATOR_SPLITSEQUENCEOPERATOR_H_
class TIntermNode;
-class TSymbolTable;
void SplitSequenceOperator(TIntermNode *root,
int patternsToSplitMask,
- unsigned int *temporaryIndex,
- const TSymbolTable &symbolTable,
- int shaderVersion);
+ unsigned int *temporaryIndex);
#endif // COMPILER_TRANSLATOR_SPLITSEQUENCEOPERATOR_H_
diff --git a/src/compiler/translator/TranslatorHLSL.cpp b/src/compiler/translator/TranslatorHLSL.cpp
index cbeeebb..15ab7af 100644
--- a/src/compiler/translator/TranslatorHLSL.cpp
+++ b/src/compiler/translator/TranslatorHLSL.cpp
@@ -37,17 +37,16 @@
// Note that SimplifyLoopConditions needs to be run before any other AST transformations that
// may need to generate new statements from loop conditions or loop expressions.
- SimplifyLoopConditions(root,
- IntermNodePatternMatcher::kExpressionReturningArray |
- IntermNodePatternMatcher::kUnfoldedShortCircuitExpression |
- IntermNodePatternMatcher::kDynamicIndexingOfVectorOrMatrixInLValue,
+ SimplifyLoopConditions(root, IntermNodePatternMatcher::kExpressionReturningArray |
+ IntermNodePatternMatcher::kUnfoldedShortCircuitExpression,
getTemporaryIndex(), getSymbolTable(), getShaderVersion());
- SplitSequenceOperator(root,
- IntermNodePatternMatcher::kExpressionReturningArray |
- IntermNodePatternMatcher::kUnfoldedShortCircuitExpression |
- IntermNodePatternMatcher::kDynamicIndexingOfVectorOrMatrixInLValue,
- getTemporaryIndex(), getSymbolTable(), getShaderVersion());
+ // TODO (oetuaho): Sequence operators should also be split in case there is dynamic indexing of
+ // a vector or matrix as an l-value inside (RemoveDynamicIndexing transformation step generates
+ // statements in this case).
+ SplitSequenceOperator(root, IntermNodePatternMatcher::kExpressionReturningArray |
+ IntermNodePatternMatcher::kUnfoldedShortCircuitExpression,
+ getTemporaryIndex());
// Note that SeparateDeclarations needs to be run before UnfoldShortCircuitToIf.
UnfoldShortCircuitToIf(root, getTemporaryIndex());