Allow length() on arbitrary array expressions

src/compiler.gypi

@@ -107,6 +107,8 @@
             'compiler/translator/RecordConstantPrecision.h',
             'compiler/translator/RegenerateStructNames.cpp',
             'compiler/translator/RegenerateStructNames.h',
+            'compiler/translator/RemoveArrayLengthMethod.cpp',
+            'compiler/translator/RemoveArrayLengthMethod.h',
             'compiler/translator/RemoveInvariantDeclaration.cpp',
             'compiler/translator/RemoveInvariantDeclaration.h',
             'compiler/translator/RemovePow.cpp',
@@ -132,6 +134,8 @@
             'compiler/translator/ShaderVars.cpp',
             'compiler/translator/SimplifyLoopConditions.cpp',
             'compiler/translator/SimplifyLoopConditions.h',
+            'compiler/translator/SplitSequenceOperator.cpp',
+            'compiler/translator/SplitSequenceOperator.h',
             'compiler/translator/SymbolTable.cpp',
             'compiler/translator/SymbolTable.h',
             'compiler/translator/Types.cpp',
@@ -212,8 +216,6 @@
             'compiler/translator/SeparateArrayInitialization.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/Compiler.cpp

@@ -27,12 +27,14 @@
 #include "compiler/translator/ParseContext.h"
 #include "compiler/translator/PruneEmptyDeclarations.h"
 #include "compiler/translator/RegenerateStructNames.h"
+#include "compiler/translator/RemoveArrayLengthMethod.h"
 #include "compiler/translator/RemoveInvariantDeclaration.h"
 #include "compiler/translator/RemovePow.h"
 #include "compiler/translator/RewriteDoWhile.h"
 #include "compiler/translator/ScalarizeVecAndMatConstructorArgs.h"
 #include "compiler/translator/SeparateDeclarations.h"
 #include "compiler/translator/SimplifyLoopConditions.h"
+#include "compiler/translator/SplitSequenceOperator.h"
 #include "compiler/translator/UnfoldShortCircuitAST.h"
 #include "compiler/translator/UseInterfaceBlockFields.h"
 #include "compiler/translator/ValidateLimitations.h"
@@ -526,26 +528,43 @@ TIntermBlock *TCompiler::compileTreeImpl(const char *const shaderStrings[],
             DeferGlobalInitializers(root, needToInitializeGlobalsInAST(), &symbolTable);
         }
 
+        // Split multi declarations and remove calls to array length().
+        if (success)
+        {
+            // 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::kMultiDeclaration |
+                                       IntermNodePatternMatcher::kArrayLengthMethod,
+                                   &getSymbolTable(), getShaderVersion());
+
+            // Note that separate declarations need to be run before other AST transformations that
+            // generate new statements from expressions.
+            SeparateDeclarations(root);
+
+            SplitSequenceOperator(root, IntermNodePatternMatcher::kArrayLengthMethod,
+                                  &getSymbolTable(), getShaderVersion());
+
+            RemoveArrayLengthMethod(root);
+        }
+
         if (success && (compileOptions & SH_INITIALIZE_UNINITIALIZED_LOCALS) && getOutputType())
         {
             // Initialize uninitialized local variables.
             // In some cases initializing can generate extra statements in the parent block, such as
             // when initializing nameless structs or initializing arrays in ESSL 1.00. In that case
-            // we need to first simplify loop conditions and separate declarations. If we don't
-            // follow the Appendix A limitations, loop init statements can declare arrays or
-            // nameless structs and have multiple declarations.
+            // we need to first simplify loop conditions. We've already separated declarations
+            // earlier, which is also required. If we don't follow the Appendix A limitations, loop
+            // init statements can declare arrays or nameless structs and have multiple
+            // declarations.
 
             if (!shouldRunLoopAndIndexingValidation(compileOptions))
             {
                 SimplifyLoopConditions(root,
-                                       IntermNodePatternMatcher::kMultiDeclaration |
                                            IntermNodePatternMatcher::kArrayDeclaration |
                                            IntermNodePatternMatcher::kNamelessStructDeclaration,
                                        &getSymbolTable(), getShaderVersion());
             }
-            // We only really need to separate array declarations and nameless struct declarations,
-            // but it's simpler to just use the regular SeparateDeclarations.
-            SeparateDeclarations(root);
             InitializeUninitializedLocals(root, getShaderVersion());
         }
 

src/compiler/translator/IntermNodePatternMatcher.cpp

@@ -48,6 +48,18 @@ bool IntermNodePatternMatcher::matchInternal(TIntermBinary *node, TIntermNode *p
     return false;
 }
 
+bool IntermNodePatternMatcher::match(TIntermUnary *node)
+{
+    if ((mMask & kArrayLengthMethod) != 0)
+    {
+        if (node->getOp() == EOpArrayLength)
+        {
+            return true;
+        }
+    }
+    return false;
+}
+
 bool IntermNodePatternMatcher::match(TIntermBinary *node, TIntermNode *parentNode)
 {
     // L-value tracking information is needed to check for dynamic indexing in L-value.

src/compiler/translator/IntermNodePatternMatcher.h

@@ -16,9 +16,10 @@ namespace sh
 
 class TIntermAggregate;
 class TIntermBinary;
+class TIntermDeclaration;
 class TIntermNode;
 class TIntermTernary;
-class TIntermDeclaration;
+class TIntermUnary;
 
 class IntermNodePatternMatcher
 {
@@ -44,10 +45,15 @@ class IntermNodePatternMatcher
         kArrayDeclaration = 0x0001 << 4,
 
         // Matches declarations of structs where the struct type does not have a name.
-        kNamelessStructDeclaration = 0x0001 << 5
+        kNamelessStructDeclaration = 0x0001 << 5,
+
+        // Matches array length() method.
+        kArrayLengthMethod = 0x0001 << 6
     };
     IntermNodePatternMatcher(const unsigned int mask);
 
+    bool match(TIntermUnary *node);
+
     bool match(TIntermBinary *node, TIntermNode *parentNode);
 
     // Use this version for checking binary node matches in case you're using flag

src/compiler/translator/OutputGLSLBase.cpp

@@ -734,6 +734,10 @@ bool TOutputGLSLBase::visitUnary(Visit visit, TIntermUnary *node)
         case EOpPreDecrement:
             preString = "(--";
             break;
+        case EOpArrayLength:
+            preString  = "((";
+            postString = ").length())";
+            break;
 
         case EOpRadians:
         case EOpDegrees:

src/compiler/translator/ParseContext.cpp

@@ -5461,11 +5461,6 @@ TIntermTyped *TParseContext::addMethod(TFunction *fnCall,
     {
         error(loc, "missing input primitive declaration before calling length on gl_in", "length");
     }
-    else if (typedThis->hasSideEffects())
-    {
-        error(loc, "length method not supported on expressions with possible side effects",
-              "length");
-    }
     else
     {
         TIntermUnary *node = new TIntermUnary(EOpArrayLength, typedThis);

src/compiler/translator/RemoveArrayLengthMethod.cpp

+//
+// Copyright (c) 2017 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.
+//
+// RemoveArrayLengthMethod.cpp:
+//   Fold array length expressions, including cases where the "this" node has side effects.
+//   Example:
+//     int i = (a = b).length();
+//     int j = (func()).length();
+//   becomes:
+//     (a = b);
+//     int i = <constant array length>;
+//     func();
+//     int j = <constant array length>;
+//
+//   Must be run after SplitSequenceOperator, SimplifyLoopConditions and SeparateDeclarations steps
+//   have been done to expressions containing calls of the array length method.
+
+#include "compiler/translator/RemoveArrayLengthMethod.h"
+
+#include "compiler/translator/IntermNode.h"
+#include "compiler/translator/IntermTraverse.h"
+
+namespace sh
+{
+
+namespace
+{
+
+class RemoveArrayLengthTraverser : public TIntermTraverser
+{
+  public:
+    RemoveArrayLengthTraverser() : TIntermTraverser(true, false, false), mFoundArrayLength(false) {}
+
+    bool visitUnary(Visit visit, TIntermUnary *node) override;
+
+    void nextIteration() { mFoundArrayLength = false; }
+
+    bool foundArrayLength() const { return mFoundArrayLength; }
+
+  private:
+    bool mFoundArrayLength;
+};
+
+bool RemoveArrayLengthTraverser::visitUnary(Visit visit, TIntermUnary *node)
+{
+    if (node->getOp() == EOpArrayLength)
+    {
+        mFoundArrayLength = true;
+        if (!node->getOperand()->hasSideEffects())
+        {
+            queueReplacement(node->fold(nullptr), OriginalNode::IS_DROPPED);
+            return false;
+        }
+        insertStatementInParentBlock(node->getOperand()->deepCopy());
+        TConstantUnion *constArray = new TConstantUnion[1];
+        constArray->setIConst(node->getOperand()->getOutermostArraySize());
+        queueReplacement(new TIntermConstantUnion(constArray, node->getType()),
+                         OriginalNode::IS_DROPPED);
+        return false;
+    }
+    return true;
+}
+
+}  // anonymous namespace
+
+void RemoveArrayLengthMethod(TIntermBlock *root)
+{
+    RemoveArrayLengthTraverser traverser;
+    do
+    {
+        traverser.nextIteration();
+        root->traverse(&traverser);
+        if (traverser.foundArrayLength())
+            traverser.updateTree();
+    } while (traverser.foundArrayLength());
+}
+
+}  // namespace sh

src/compiler/translator/RemoveArrayLengthMethod.h

+//
+// Copyright (c) 2017 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.
+//
+// RemoveArrayLengthMethod.h:
+//   Fold array length expressions, including cases where the "this" node has side effects.
+//   Example:
+//     int i = (a = b).length();
+//     int j = (func()).length();
+//   becomes:
+//     (a = b);
+//     int i = <constant array length>;
+//     func();
+//     int j = <constant array length>;
+//
+//   Must be run after SplitSequenceOperator, SimplifyLoopConditions and SeparateDeclarations steps
+//   have been done to expressions containing calls of the array length method.
+
+namespace sh
+{
+
+class TIntermBlock;
+
+void RemoveArrayLengthMethod(TIntermBlock *root);
+
+}  // namespace sh

src/compiler/translator/SimplifyLoopConditions.cpp

@@ -29,6 +29,7 @@ class SimplifyLoopConditionsTraverser : public TLValueTrackingTraverser
 
     void traverseLoop(TIntermLoop *node) override;
 
+    bool visitUnary(Visit visit, TIntermUnary *node) override;
     bool visitBinary(Visit visit, TIntermBinary *node) override;
     bool visitAggregate(Visit visit, TIntermAggregate *node) override;
     bool visitTernary(Visit visit, TIntermTernary *node) override;
@@ -61,6 +62,18 @@ SimplifyLoopConditionsTraverser::SimplifyLoopConditionsTraverser(
 // If we're not inside loop initialization, condition, or expression, we only need to traverse nodes
 // that may contain loops.
 
+bool SimplifyLoopConditionsTraverser::visitUnary(Visit visit, TIntermUnary *node)
+{
+    if (!mInsideLoopInitConditionOrExpression)
+        return false;
+
+    if (mFoundLoopToChange)
+        return false;  // Already decided to change this loop.
+
+    mFoundLoopToChange = mConditionsToSimplify.match(node);
+    return !mFoundLoopToChange;
+}
+
 bool SimplifyLoopConditionsTraverser::visitBinary(Visit visit, TIntermBinary *node)
 {
     if (!mInsideLoopInitConditionOrExpression)

src/compiler/translator/SplitSequenceOperator.cpp

@@ -27,6 +27,7 @@ class SplitSequenceOperatorTraverser : public TLValueTrackingTraverser
                                    TSymbolTable *symbolTable,
                                    int shaderVersion);
 
+    bool visitUnary(Visit visit, TIntermUnary *node) override;
     bool visitBinary(Visit visit, TIntermBinary *node) override;
     bool visitAggregate(Visit visit, TIntermAggregate *node) override;
     bool visitTernary(Visit visit, TIntermTernary *node) override;
@@ -75,6 +76,21 @@ bool SplitSequenceOperatorTraverser::visitAggregate(Visit visit, TIntermAggregat
     return true;
 }
 
+bool SplitSequenceOperatorTraverser::visitUnary(Visit visit, TIntermUnary *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;
+}
+
 bool SplitSequenceOperatorTraverser::visitBinary(Visit visit, TIntermBinary *node)
 {
     if (node->getOp() == EOpComma)

src/compiler/translator/TranslatorHLSL.cpp

@@ -41,17 +41,13 @@ void TranslatorHLSL::translate(TIntermBlock *root, ShCompileOptions compileOptio
 
     // 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.
+    // Note that SeparateDeclarations has already been run in TCompiler::compileTreeImpl().
     SimplifyLoopConditions(root,
                            IntermNodePatternMatcher::kExpressionReturningArray |
                                IntermNodePatternMatcher::kUnfoldedShortCircuitExpression |
-                               IntermNodePatternMatcher::kDynamicIndexingOfVectorOrMatrixInLValue |
-                               IntermNodePatternMatcher::kMultiDeclaration,
+                               IntermNodePatternMatcher::kDynamicIndexingOfVectorOrMatrixInLValue,
                            &getSymbolTable(), getShaderVersion());
 
-    // Note that separate declarations need to be run before other AST transformations that
-    // generate new statements from expressions.
-    SeparateDeclarations(root);
-
     SplitSequenceOperator(root,
                           IntermNodePatternMatcher::kExpressionReturningArray |
                               IntermNodePatternMatcher::kUnfoldedShortCircuitExpression |

src/tests/gl_tests/GLSLTest.cpp

@@ -3403,6 +3403,111 @@ TEST_P(WebGLGLSLTest, GlobalVariableDeclaredAfterMain)
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
 }
 
+// Test calling array length() with a "this" expression having side effects inside a loop condition.
+// The spec says that sequence operator operands need to run in sequence.
+TEST_P(GLSLTest_ES3, ArrayLengthOnExpressionWithSideEffectsInLoopCondition)
+{
+    // "a" gets doubled three times in the below program.
+    const std::string &fragmentShader =
+        R"(#version 300 es
+        precision highp float;
+        out vec4 my_FragColor;
+        uniform int u_zero;
+        int a;
+        int[2] doubleA()
+        {
+            a *= 2;
+            return int[2](a, a);
+        }
+        void main()
+        {
+            a = u_zero + 1;
+            for (int i = 0; i < doubleA().length(); ++i)
+            {}
+            if (a == 8)
+            {
+                my_FragColor = vec4(0, 1, 0, 1);
+            }
+            else
+            {
+                my_FragColor = vec4(1, 0, 0, 1);
+            }
+        })";
+
+    ANGLE_GL_PROGRAM(program, mSimpleVSSource, fragmentShader);
+    drawQuad(program.get(), "inputAttribute", 0.5f);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
+// Test calling array length() with a "this" expression having side effects that interact with side
+// effects of another operand of the same sequence operator. The spec says that sequence operator
+// operands need to run in order from left to right (ESSL 3.00.6 section 5.9).
+TEST_P(GLSLTest_ES3, ArrayLengthOnExpressionWithSideEffectsInSequence)
+{
+    const std::string &fragmentShader =
+        R"(#version 300 es
+        precision highp float;
+        out vec4 my_FragColor;
+        uniform int u_zero;
+        int a;
+        int[3] doubleA()
+        {
+            a *= 2;
+            return int[3](a, a, a);
+        }
+        void main()
+        {
+            a = u_zero;
+            int b = (a++, doubleA().length());
+            if (b == 3 && a == 2)
+            {
+                my_FragColor = vec4(0, 1, 0, 1);
+            }
+            else
+            {
+                my_FragColor = vec4(1, 0, 0, 1);
+            }
+        })";
+
+    ANGLE_GL_PROGRAM(program, mSimpleVSSource, fragmentShader);
+    drawQuad(program.get(), "inputAttribute", 0.5f);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
+// Test calling array length() with a "this" expression that also contains a call of array length().
+// Both "this" expressions also have side effects.
+TEST_P(GLSLTest_ES3, NestedArrayLengthMethodsWithSideEffects)
+{
+    const std::string &fragmentShader =
+        R"(#version 300 es
+        precision highp float;
+        out vec4 my_FragColor;
+        uniform int u_zero;
+        int a;
+        int[3] multiplyA(int multiplier)
+        {
+            a *= multiplier;
+            return int[3](a, a, a);
+        }
+        void main()
+        {
+            a = u_zero + 1;
+            int b = multiplyA(multiplyA(2).length()).length();
+            if (b == 3 && a == 6)
+            {
+                my_FragColor = vec4(0, 1, 0, 1);
+            }
+            else
+            {
+                my_FragColor = vec4(1, 0, 0, 1);
+            }
+        })";
+
+    ANGLE_GL_PROGRAM(program, mSimpleVSSource, fragmentShader);
+    drawQuad(program.get(), "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(),