Revert "Cover vector dynamic indexing case in SplitSequenceOperator"

src/compiler/translator/IntermNodePatternMatcher.cpp

@@ -27,14 +27,7 @@ IntermNodePatternMatcher::IntermNodePatternMatcher(const unsigned int mask) : mM
 {
 }
 
-// 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 @@ bool IntermNodePatternMatcher::matchInternal(TIntermBinary *node, TIntermNode *p
     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)

src/compiler/translator/IntermNodePatternMatcher.h

@@ -19,35 +19,22 @@ class TIntermSelection;
 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

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 @@ bool RemoveDynamicIndexingTraverser::visitBinary(Visit visit, TIntermBinary *nod
             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);
 

src/compiler/translator/SimplifyLoopConditions.cpp

@@ -83,7 +83,7 @@ bool SimplifyLoopConditionsTraverser::visitBinary(Visit visit, TIntermBinary *no
     if (!mInsideLoopConditionOrExpression)
         return false;
 
-    mFoundLoopToChange = mConditionsToSimplify.match(node, getParentNode(), isLValueRequiredHere());
+    mFoundLoopToChange = mConditionsToSimplify.match(node, getParentNode());
     return !mFoundLoopToChange;
 }
 

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 @@ class SplitSequenceOperatorTraverser : public TLValueTrackingTraverser
     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 @@ bool SplitSequenceOperatorTraverser::visitBinary(Visit visit, TIntermBinary *nod
     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 @@ bool SplitSequenceOperatorTraverser::visitSelection(Visit visit, TIntermSelectio
 
 }  // 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.

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_

src/compiler/translator/TranslatorHLSL.cpp

@@ -37,17 +37,16 @@ void TranslatorHLSL::translate(TIntermNode *root, int compileOptions)
 
     // 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());

src/tests/gl_tests/GLSLTest.cpp

@@ -2130,35 +2130,6 @@ TEST_P(GLSLTest_ES3, SequenceOperatorEvaluationOrderShortCircuit)
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
 }
 
-// Sequence operator evaluates operands from left to right (ESSL 3.00 section 5.9).
-// Indexing the vector needs to be evaluated after func() for the right result.
-TEST_P(GLSLTest_ES3, SequenceOperatorEvaluationOrderDynamicVectorIndexingInLValue)
-{
-    const std::string &fragmentShaderSource =
-        "#version 300 es\n"
-        "precision mediump float;\n"
-        "out vec4 my_FragColor;\n"
-        "uniform int u_zero;\n"
-        "int sideEffectCount = 0;\n"
-        "float func() {\n"
-        "    ++sideEffectCount;\n"
-        "    return -1.0;\n"
-        "}\n"
-        "void main() {\n"
-        "    vec4 v = vec4(0.0, 2.0, 4.0, 6.0); \n"
-        "    float f = (func(), (++v[u_zero + sideEffectCount]));\n"
-        "    bool green = abs(f - 3.0) < 0.01 && abs(v[1] - 3.0) < 0.01 && sideEffectCount == 1;\n"
-        "    my_FragColor = vec4(0.0, (green ? 1.0 : 0.0), 0.0, 1.0);\n"
-        "}\n";
-
-    GLuint program = CompileProgram(mSimpleVSSource, fragmentShaderSource);
-    ASSERT_NE(0u, program);
-
-    drawQuad(program, "inputAttribute", 0.5f);
-
-    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
-}
-
 // Test that using gl_PointCoord with GL_TRIANGLES doesn't produce a link error.
 // From WebGL test conformance/rendering/point-specific-shader-variables.html
 // See http://anglebug.com/1380