Split sequence operator when necessary

src/compiler.gypi

@@ -184,6 +184,8 @@
             'compiler/translator/SeparateDeclarations.h',
             'compiler/translator/SeparateExpressionsReturningArrays.cpp',
             'compiler/translator/SeparateExpressionsReturningArrays.h',
+            'compiler/translator/SplitSequenceOperator.cpp',
+            'compiler/translator/SplitSequenceOperator.h',
             'compiler/translator/StructureHLSL.cpp',
             'compiler/translator/StructureHLSL.h',
             'compiler/translator/TextureFunctionHLSL.cpp',

src/compiler/translator/SplitSequenceOperator.cpp

+//
+// Copyright (c) 2016 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.
+//
+// SplitSequenceOperator is an AST traverser that detects sequence operator expressions that
+// go through further AST transformations that generate statements, and splits them so that
+// possible side effects of earlier parts of the sequence operator expression are guaranteed to be
+// evaluated before the latter parts of the sequence operator expression are evaluated.
+//
+
+#include "compiler/translator/SplitSequenceOperator.h"
+
+#include "compiler/translator/IntermNode.h"
+#include "compiler/translator/IntermNodePatternMatcher.h"
+
+namespace
+{
+
+class SplitSequenceOperatorTraverser : public TIntermTraverser
+{
+  public:
+    SplitSequenceOperatorTraverser(unsigned int patternsToSplitMask);
+
+    bool visitBinary(Visit visit, TIntermBinary *node) override;
+    bool visitAggregate(Visit visit, TIntermAggregate *node) override;
+    bool visitSelection(Visit visit, TIntermSelection *node) override;
+
+    void nextIteration();
+    bool foundExpressionToSplit() const { return mFoundExpressionToSplit; }
+
+  protected:
+    // Marked to true once an operation that needs to be hoisted out of the expression has been
+    // found. After that, no more AST updates are performed on that traversal.
+    bool mFoundExpressionToSplit;
+    int mInsideSequenceOperator;
+
+    IntermNodePatternMatcher mPatternToSplitMatcher;
+};
+
+SplitSequenceOperatorTraverser::SplitSequenceOperatorTraverser(unsigned int patternsToSplitMask)
+    : TIntermTraverser(true, false, true),
+      mFoundExpressionToSplit(false),
+      mInsideSequenceOperator(0),
+      mPatternToSplitMatcher(patternsToSplitMask)
+{
+}
+
+void SplitSequenceOperatorTraverser::nextIteration()
+{
+    mFoundExpressionToSplit = false;
+    mInsideSequenceOperator = 0;
+    nextTemporaryIndex();
+}
+
+bool SplitSequenceOperatorTraverser::visitBinary(Visit visit, TIntermBinary *node)
+{
+    if (mFoundExpressionToSplit)
+        return false;
+
+    if (mInsideSequenceOperator > 0 && visit == PreVisit)
+    {
+        // Detect expressions that need to be simplified
+        mFoundExpressionToSplit = mPatternToSplitMatcher.match(node, getParentNode());
+        return !mFoundExpressionToSplit;
+    }
+
+    return true;
+}
+
+bool SplitSequenceOperatorTraverser::visitAggregate(Visit visit, TIntermAggregate *node)
+{
+    if (node->getOp() == EOpComma)
+    {
+        if (visit == PreVisit)
+        {
+            if (mFoundExpressionToSplit)
+            {
+                return false;
+            }
+            mInsideSequenceOperator++;
+        }
+        else if (visit == PostVisit)
+        {
+            if (mFoundExpressionToSplit)
+            {
+                // Move all operands of the sequence operation except the last one into separate
+                // statements in the parent block.
+                TIntermSequence insertions;
+                for (auto *sequenceChild : *node->getSequence())
+                {
+                    if (sequenceChild != node->getSequence()->back())
+                    {
+                        insertions.push_back(sequenceChild);
+                    }
+                }
+                insertStatementsInParentBlock(insertions);
+                // Replace the sequence with its last operand
+                NodeUpdateEntry replaceSequence(getParentNode(), node, node->getSequence()->back(),
+                                                false);
+                mReplacements.push_back(replaceSequence);
+            }
+            mInsideSequenceOperator--;
+        }
+        return true;
+    }
+
+    if (mFoundExpressionToSplit)
+        return false;
+
+    if (mInsideSequenceOperator > 0 && visit == PreVisit)
+    {
+        // Detect expressions that need to be simplified
+        mFoundExpressionToSplit = mPatternToSplitMatcher.match(node, getParentNode());
+        return !mFoundExpressionToSplit;
+    }
+
+    return true;
+}
+
+bool SplitSequenceOperatorTraverser::visitSelection(Visit visit, TIntermSelection *node)
+{
+    if (mFoundExpressionToSplit)
+        return false;
+
+    if (mInsideSequenceOperator > 0 && visit == PreVisit)
+    {
+        // Detect expressions that need to be simplified
+        mFoundExpressionToSplit = mPatternToSplitMatcher.match(node);
+        return !mFoundExpressionToSplit;
+    }
+
+    return true;
+}
+
+}  // namespace
+
+void SplitSequenceOperator(TIntermNode *root, int patternsToSplitMask, unsigned int *temporaryIndex)
+{
+    SplitSequenceOperatorTraverser traverser(patternsToSplitMask);
+    ASSERT(temporaryIndex != nullptr);
+    traverser.useTemporaryIndex(temporaryIndex);
+    // Separate one expression at a time, and reset the traverser between iterations.
+    do
+    {
+        traverser.nextIteration();
+        root->traverse(&traverser);
+        if (traverser.foundExpressionToSplit())
+            traverser.updateTree();
+    } while (traverser.foundExpressionToSplit());
+}

src/compiler/translator/SplitSequenceOperator.h

+//
+// Copyright (c) 2016 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.
+//
+// SplitSequenceOperator is an AST traverser that detects sequence operator expressions that
+// go through further AST transformations that generate statements, and splits them so that
+// possible side effects of earlier parts of the sequence operator expression are guaranteed to be
+// evaluated before the latter parts of the sequence operator expression are evaluated.
+//
+
+#ifndef COMPILER_TRANSLATOR_SPLITSEQUENCEOPERATOR_H_
+#define COMPILER_TRANSLATOR_SPLITSEQUENCEOPERATOR_H_
+
+class TIntermNode;
+
+void SplitSequenceOperator(TIntermNode *root,
+                           int patternsToSplitMask,
+                           unsigned int *temporaryIndex);
+
+#endif  // COMPILER_TRANSLATOR_SPLITSEQUENCEOPERATOR_H_

src/compiler/translator/TranslatorHLSL.cpp

@@ -8,12 +8,14 @@
 
 #include "compiler/translator/ArrayReturnValueToOutParameter.h"
 #include "compiler/translator/EmulatePrecision.h"
+#include "compiler/translator/IntermNodePatternMatcher.h"
 #include "compiler/translator/OutputHLSL.h"
 #include "compiler/translator/RemoveDynamicIndexing.h"
 #include "compiler/translator/RewriteElseBlocks.h"
 #include "compiler/translator/SeparateArrayInitialization.h"
 #include "compiler/translator/SeparateDeclarations.h"
 #include "compiler/translator/SeparateExpressionsReturningArrays.h"
+#include "compiler/translator/SplitSequenceOperator.h"
 #include "compiler/translator/UnfoldShortCircuitToIf.h"
 
 TranslatorHLSL::TranslatorHLSL(sh::GLenum type, ShShaderSpec spec, ShShaderOutput output)
@@ -28,6 +30,13 @@ void TranslatorHLSL::translate(TIntermNode *root, int compileOptions)
 
     SeparateDeclarations(root);
 
+    // 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

@@ -436,6 +436,18 @@ class GLSLTest : public ANGLETest
 
 class GLSLTest_ES3 : public GLSLTest
 {
+    void SetUp() override
+    {
+        ANGLETest::SetUp();
+
+        mSimpleVSSource =
+            "#version 300 es\n"
+            "in vec4 inputAttribute;"
+            "void main()"
+            "{"
+            "    gl_Position = inputAttribute;"
+            "}";
+    }
 };
 
 TEST_P(GLSLTest, NamelessScopedStructs)
@@ -1888,6 +1900,59 @@ TEST_P(GLSLTest, PragmaDirective)
     EXPECT_NE(0u, program);
 }
 
+// Sequence operator evaluates operands from left to right (ESSL 3.00 section 5.9).
+// The function call that returns the array needs to be evaluated after ++j for the expression to
+// return the correct value (true).
+TEST_P(GLSLTest_ES3, SequenceOperatorEvaluationOrderArray)
+{
+    const std::string &fragmentShaderSource =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 my_FragColor; \n"
+        "int[2] func(int param) {\n"
+        "    return int[2](param, param);\n"
+        "}\n"
+        "void main() {\n"
+        "    int a[2]; \n"
+        "    for (int i = 0; i < 2; ++i) {\n"
+        "        a[i] = 1;\n"
+        "    }\n"
+        "    int j = 0; \n"
+        "    bool result = ((++j), (a == func(j)));\n"
+        "    my_FragColor = vec4(0.0, (result ? 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);
+}
+
+// Sequence operator evaluates operands from left to right (ESSL 3.00 section 5.9).
+// The short-circuiting expression needs to be evaluated after ++j for the expression to return the
+// correct value (true).
+TEST_P(GLSLTest_ES3, SequenceOperatorEvaluationOrderShortCircuit)
+{
+    const std::string &fragmentShaderSource =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 my_FragColor; \n"
+        "void main() {\n"
+        "    int j = 0; \n"
+        "    bool result = ((++j), (j == 1 ? true : (++j == 3)));\n"
+        "    my_FragColor = vec4(0.0, ((result && j == 1) ? 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);
+}
+
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these tests should be run against.
 ANGLE_INSTANTIATE_TEST(GLSLTest,
                        ES2_D3D9(),