Revert "Revert "Cover vector dynamic indexing case in SplitSequenceOperator""

src/compiler/translator/IntermNodePatternMatcher.cpp

@@ -27,7 +27,14 @@ IntermNodePatternMatcher::IntermNodePatternMatcher(const unsigned int mask) : mM
 {
 }
 
-bool IntermNodePatternMatcher::match(TIntermBinary *node, TIntermNode *parentNode)
+// static
+bool IntermNodePatternMatcher::IsDynamicIndexingOfVectorOrMatrix(TIntermBinary *node)
+{
+    return node->getOp() == EOpIndexIndirect && !node->getLeft()->isArray() &&
+           node->getLeft()->getBasicType() != EbtStruct;
+}
+
+bool IntermNodePatternMatcher::matchInternal(TIntermBinary *node, TIntermNode *parentNode)
 {
     if ((mMask & kExpressionReturningArray) != 0)
     {
@@ -49,6 +56,33 @@ bool IntermNodePatternMatcher::match(TIntermBinary *node, TIntermNode *parentNod
     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,22 +19,35 @@ 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
+        kExpressionReturningArray = 0x0002,
+
+        // Matches dynamic indexing of vectors or matrices in l-values.
+        kDynamicIndexingOfVectorOrMatrixInLValue = 0x0004
     };
     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,6 +11,7 @@
 
 #include "compiler/translator/InfoSink.h"
 #include "compiler/translator/IntermNode.h"
+#include "compiler/translator/IntermNodePatternMatcher.h"
 #include "compiler/translator/SymbolTable.h"
 
 namespace
@@ -405,10 +406,18 @@ bool RemoveDynamicIndexingTraverser::visitBinary(Visit visit, TIntermBinary *nod
             TIntermSymbol *tempIndex = createTempSymbol(node->getRight()->getType());
             queueReplacementWithParent(node, node->getRight(), tempIndex, OriginalNode::IS_DROPPED);
         }
-        else if (!node->getLeft()->isArray() && node->getLeft()->getBasicType() != EbtStruct)
+        else if (IntermNodePatternMatcher::IsDynamicIndexingOfVectorOrMatrix(node))
         {
             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());
+    mFoundLoopToChange = mConditionsToSimplify.match(node, getParentNode(), isLValueRequiredHere());
     return !mFoundLoopToChange;
 }
 

src/compiler/translator/SplitSequenceOperator.cpp

@@ -17,10 +17,12 @@
 namespace
 {
 
-class SplitSequenceOperatorTraverser : public TIntermTraverser
+class SplitSequenceOperatorTraverser : public TLValueTrackingTraverser
 {
   public:
-    SplitSequenceOperatorTraverser(unsigned int patternsToSplitMask);
+    SplitSequenceOperatorTraverser(unsigned int patternsToSplitMask,
+                                   const TSymbolTable &symbolTable,
+                                   int shaderVersion);
 
     bool visitBinary(Visit visit, TIntermBinary *node) override;
     bool visitAggregate(Visit visit, TIntermAggregate *node) override;
@@ -38,8 +40,10 @@ class SplitSequenceOperatorTraverser : public TIntermTraverser
     IntermNodePatternMatcher mPatternToSplitMatcher;
 };
 
-SplitSequenceOperatorTraverser::SplitSequenceOperatorTraverser(unsigned int patternsToSplitMask)
-    : TIntermTraverser(true, false, true),
+SplitSequenceOperatorTraverser::SplitSequenceOperatorTraverser(unsigned int patternsToSplitMask,
+                                                               const TSymbolTable &symbolTable,
+                                                               int shaderVersion)
+    : TLValueTrackingTraverser(true, false, true, symbolTable, shaderVersion),
       mFoundExpressionToSplit(false),
       mInsideSequenceOperator(0),
       mPatternToSplitMatcher(patternsToSplitMask)
@@ -61,7 +65,8 @@ bool SplitSequenceOperatorTraverser::visitBinary(Visit visit, TIntermBinary *nod
     if (mInsideSequenceOperator > 0 && visit == PreVisit)
     {
         // Detect expressions that need to be simplified
-        mFoundExpressionToSplit = mPatternToSplitMatcher.match(node, getParentNode());
+        mFoundExpressionToSplit =
+            mPatternToSplitMatcher.match(node, getParentNode(), isLValueRequiredHere());
         return !mFoundExpressionToSplit;
     }
 
@@ -133,9 +138,13 @@ bool SplitSequenceOperatorTraverser::visitSelection(Visit visit, TIntermSelectio
 
 }  // namespace
 
-void SplitSequenceOperator(TIntermNode *root, int patternsToSplitMask, unsigned int *temporaryIndex)
+void SplitSequenceOperator(TIntermNode *root,
+                           int patternsToSplitMask,
+                           unsigned int *temporaryIndex,
+                           const TSymbolTable &symbolTable,
+                           int shaderVersion)
 {
-    SplitSequenceOperatorTraverser traverser(patternsToSplitMask);
+    SplitSequenceOperatorTraverser traverser(patternsToSplitMask, symbolTable, shaderVersion);
     ASSERT(temporaryIndex != nullptr);
     traverser.useTemporaryIndex(temporaryIndex);
     // Separate one expression at a time, and reset the traverser between iterations.

src/compiler/translator/SplitSequenceOperator.h

@@ -13,9 +13,12 @@
 #define COMPILER_TRANSLATOR_SPLITSEQUENCEOPERATOR_H_
 
 class TIntermNode;
+class TSymbolTable;
 
 void SplitSequenceOperator(TIntermNode *root,
                            int patternsToSplitMask,
-                           unsigned int *temporaryIndex);
+                           unsigned int *temporaryIndex,
+                           const TSymbolTable &symbolTable,
+                           int shaderVersion);
 
 #endif  // COMPILER_TRANSLATOR_SPLITSEQUENCEOPERATOR_H_

src/compiler/translator/TranslatorHLSL.cpp

@@ -37,16 +37,17 @@ 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,
+    SimplifyLoopConditions(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());
+    SplitSequenceOperator(root,
+                          IntermNodePatternMatcher::kExpressionReturningArray |
+                              IntermNodePatternMatcher::kUnfoldedShortCircuitExpression |
+                              IntermNodePatternMatcher::kDynamicIndexingOfVectorOrMatrixInLValue,
+                          getTemporaryIndex(), getSymbolTable(), getShaderVersion());
 
     // Note that SeparateDeclarations needs to be run before UnfoldShortCircuitToIf.
     UnfoldShortCircuitToIf(root, getTemporaryIndex());

src/tests/gl_tests/GLSLTest.cpp

@@ -2130,6 +2130,35 @@ 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