Unfold short-circuiting operators in loop conditions correctly

src/compiler/translator/IntermNode.cpp

@@ -261,7 +261,7 @@ bool TIntermLoop::replaceChildNode(
     REPLACE_IF_IS(mInit, TIntermNode, original, replacement);
     REPLACE_IF_IS(mCond, TIntermTyped, original, replacement);
     REPLACE_IF_IS(mExpr, TIntermTyped, original, replacement);
-    REPLACE_IF_IS(mBody, TIntermNode, original, replacement);
+    REPLACE_IF_IS(mBody, TIntermAggregate, original, replacement);
     return false;
 }
 

src/compiler/translator/IntermNode.h

@@ -173,14 +173,13 @@ class TIntermLoop : public TIntermNode
 {
   public:
     TIntermLoop(TLoopType type,
-                TIntermNode *init, TIntermTyped *cond, TIntermTyped *expr,
-                TIntermNode *body)
-        : mType(type),
-          mInit(init),
-          mCond(cond),
-          mExpr(expr),
-          mBody(body),
-          mUnrollFlag(false) { }
+                TIntermNode *init,
+                TIntermTyped *cond,
+                TIntermTyped *expr,
+                TIntermAggregate *body)
+        : mType(type), mInit(init), mCond(cond), mExpr(expr), mBody(body), mUnrollFlag(false)
+    {
+    }
 
     TIntermLoop *getAsLoopNode() override { return this; }
     void traverse(TIntermTraverser *it) override;
@@ -190,7 +189,7 @@ class TIntermLoop : public TIntermNode
     TIntermNode *getInit() { return mInit; }
     TIntermTyped *getCondition() { return mCond; }
     TIntermTyped *getExpression() { return mExpr; }
-    TIntermNode *getBody() { return mBody; }
+    TIntermAggregate *getBody() { return mBody; }
 
     void setUnrollFlag(bool flag) { mUnrollFlag = flag; }
     bool getUnrollFlag() const { return mUnrollFlag; }
@@ -200,7 +199,7 @@ class TIntermLoop : public TIntermNode
     TIntermNode *mInit;  // for-loop initialization
     TIntermTyped *mCond; // loop exit condition
     TIntermTyped *mExpr; // for-loop expression
-    TIntermNode *mBody;  // loop body
+    TIntermAggregate *mBody;  // loop body
 
     bool mUnrollFlag; // Whether the loop should be unrolled or not.
 };

src/compiler/translator/UnfoldShortCircuitToIf.cpp

@@ -24,19 +24,39 @@ class UnfoldShortCircuitTraverser : public TIntermTraverser
     bool visitBinary(Visit visit, TIntermBinary *node) override;
     bool visitAggregate(Visit visit, TIntermAggregate *node) override;
     bool visitSelection(Visit visit, TIntermSelection *node) override;
+    bool visitLoop(Visit visit, TIntermLoop *node) override;
 
     void nextIteration();
     bool foundShortCircuit() const { return mFoundShortCircuit; }
 
   protected:
+    // Check if the traversal is inside a loop condition or expression, in which case the unfolded
+    // expression needs to be copied inside the loop. Returns true if the copying is done, in which
+    // case no further unfolding should be done on the same traversal.
+    // The parameters are the node that will be unfolded to multiple statements and so can't remain
+    // inside a loop condition, and its parent.
+    bool copyLoopConditionOrExpression(TIntermNode *parent, TIntermTyped *node);
+
     // Marked to true once an operation that needs to be unfolded has been found.
     // After that, no more unfolding is performed on that traversal.
     bool mFoundShortCircuit;
+
+    // Set to the loop node while a loop condition or expression is being traversed.
+    TIntermLoop *mParentLoop;
+    // Parent of the loop node while a loop condition or expression is being traversed.
+    TIntermNode *mLoopParent;
+
+    bool mInLoopCondition;
+    bool mInLoopExpression;
 };
 
 UnfoldShortCircuitTraverser::UnfoldShortCircuitTraverser()
     : TIntermTraverser(true, false, true),
-      mFoundShortCircuit(false)
+      mFoundShortCircuit(false),
+      mParentLoop(nullptr),
+      mLoopParent(nullptr),
+      mInLoopCondition(false),
+      mInLoopExpression(false)
 {
 }
 
@@ -56,10 +76,12 @@ bool UnfoldShortCircuitTraverser::visitBinary(Visit visit, TIntermBinary *node)
     {
       case EOpLogicalOr:
         mFoundShortCircuit = true;
-
-        // "x || y" is equivalent to "x ? true : y", which unfolds to "bool s; if(x) s = true; else s = y;",
-        // and then further simplifies down to "bool s = x; if(!s) s = y;".
+        if (!copyLoopConditionOrExpression(getParentNode(), node))
         {
+            // "x || y" is equivalent to "x ? true : y", which unfolds to "bool s; if(x) s = true;
+            // else s = y;",
+            // and then further simplifies down to "bool s = x; if(!s) s = y;".
+
             TIntermSequence insertions;
             TType boolType(EbtBool, EbpUndefined, EvqTemporary);
 
@@ -83,10 +105,11 @@ bool UnfoldShortCircuitTraverser::visitBinary(Visit visit, TIntermBinary *node)
         return false;
       case EOpLogicalAnd:
         mFoundShortCircuit = true;
-
-        // "x && y" is equivalent to "x ? y : false", which unfolds to "bool s; if(x) s = y; else s = false;",
-        // and then further simplifies down to "bool s = x; if(s) s = y;".
+        if (!copyLoopConditionOrExpression(getParentNode(), node))
         {
+            // "x && y" is equivalent to "x ? y : false", which unfolds to "bool s; if(x) s = y;
+            // else s = false;",
+            // and then further simplifies down to "bool s = x; if(s) s = y;".
             TIntermSequence insertions;
             TType boolType(EbtBool, EbpUndefined, EvqTemporary);
 
@@ -120,29 +143,35 @@ bool UnfoldShortCircuitTraverser::visitSelection(Visit visit, TIntermSelection *
     if (visit == PreVisit && node->usesTernaryOperator())
     {
         mFoundShortCircuit = true;
-        TIntermSequence insertions;
+        if (!copyLoopConditionOrExpression(getParentNode(), node))
+        {
+            TIntermSequence insertions;
 
-        TIntermSymbol *tempSymbol = createTempSymbol(node->getType());
-        TIntermAggregate *tempDeclaration = new TIntermAggregate(EOpDeclaration);
-        tempDeclaration->getSequence()->push_back(tempSymbol);
-        insertions.push_back(tempDeclaration);
+            TIntermSymbol *tempSymbol         = createTempSymbol(node->getType());
+            TIntermAggregate *tempDeclaration = new TIntermAggregate(EOpDeclaration);
+            tempDeclaration->getSequence()->push_back(tempSymbol);
+            insertions.push_back(tempDeclaration);
 
-        TIntermAggregate *trueBlock = new TIntermAggregate(EOpSequence);
-        TIntermBinary *trueAssignment = createTempAssignment(node->getTrueBlock()->getAsTyped());
-        trueBlock->getSequence()->push_back(trueAssignment);
+            TIntermAggregate *trueBlock = new TIntermAggregate(EOpSequence);
+            TIntermBinary *trueAssignment =
+                createTempAssignment(node->getTrueBlock()->getAsTyped());
+            trueBlock->getSequence()->push_back(trueAssignment);
 
-        TIntermAggregate *falseBlock = new TIntermAggregate(EOpSequence);
-        TIntermBinary *falseAssignment = createTempAssignment(node->getFalseBlock()->getAsTyped());
-        falseBlock->getSequence()->push_back(falseAssignment);
+            TIntermAggregate *falseBlock = new TIntermAggregate(EOpSequence);
+            TIntermBinary *falseAssignment =
+                createTempAssignment(node->getFalseBlock()->getAsTyped());
+            falseBlock->getSequence()->push_back(falseAssignment);
 
-        TIntermSelection *ifNode = new TIntermSelection(node->getCondition()->getAsTyped(), trueBlock, falseBlock);
-        insertions.push_back(ifNode);
+            TIntermSelection *ifNode =
+                new TIntermSelection(node->getCondition()->getAsTyped(), trueBlock, falseBlock);
+            insertions.push_back(ifNode);
 
-        insertStatementsInParentBlock(insertions);
+            insertStatementsInParentBlock(insertions);
 
-        TIntermSymbol *ternaryResult = createTempSymbol(node->getType());
-        NodeUpdateEntry replaceVariable(getParentNode(), node, ternaryResult, false);
-        mReplacements.push_back(replaceVariable);
+            TIntermSymbol *ternaryResult = createTempSymbol(node->getType());
+            NodeUpdateEntry replaceVariable(getParentNode(), node, ternaryResult, false);
+            mReplacements.push_back(replaceVariable);
+        }
         return false;
     }
 
@@ -170,25 +199,148 @@ bool UnfoldShortCircuitTraverser::visitAggregate(Visit visit, TIntermAggregate *
             mMultiReplacements.clear();
             mInsertions.clear();
 
-            TIntermSequence insertions;
-            TIntermSequence *seq = node->getSequence();
+            if (!copyLoopConditionOrExpression(getParentNode(), node))
+            {
+                TIntermSequence insertions;
+                TIntermSequence *seq = node->getSequence();
+
+                TIntermSequence::size_type i = 0;
+                ASSERT(!seq->empty());
+                while (i < seq->size() - 1)
+                {
+                    TIntermTyped *child = (*seq)[i]->getAsTyped();
+                    insertions.push_back(child);
+                    ++i;
+                }
+
+                insertStatementsInParentBlock(insertions);
+
+                NodeUpdateEntry replaceVariable(getParentNode(), node, (*seq)[i], false);
+                mReplacements.push_back(replaceVariable);
+            }
+        }
+    }
+    return true;
+}
+
+bool UnfoldShortCircuitTraverser::visitLoop(Visit visit, TIntermLoop *node)
+{
+    if (visit == PreVisit)
+    {
+        if (mFoundShortCircuit)
+            return false;  // No need to traverse further
 
-            TIntermSequence::size_type i = 0;
-            ASSERT(!seq->empty());
-            while (i < seq->size() - 1)
+        mLoopParent = getParentNode();
+        mParentLoop = node;
+        incrementDepth(node);
+
+        if (node->getInit())
+        {
+            node->getInit()->traverse(this);
+            if (mFoundShortCircuit)
             {
-                TIntermTyped *child = (*seq)[i]->getAsTyped();
-                insertions.push_back(child);
-                ++i;
+                decrementDepth();
+                return false;
             }
+        }
 
-            insertStatementsInParentBlock(insertions);
+        if (node->getCondition())
+        {
+            mInLoopCondition = true;
+            node->getCondition()->traverse(this);
+            mInLoopCondition = false;
 
-            NodeUpdateEntry replaceVariable(getParentNode(), node, (*seq)[i], false);
-            mReplacements.push_back(replaceVariable);
+            if (mFoundShortCircuit)
+            {
+                decrementDepth();
+                return false;
+            }
+        }
+
+        if (node->getExpression())
+        {
+            mInLoopExpression = true;
+            node->getExpression()->traverse(this);
+            mInLoopExpression = false;
+
+            if (mFoundShortCircuit)
+            {
+                decrementDepth();
+                return false;
+            }
         }
+
+        if (node->getBody())
+            node->getBody()->traverse(this);
+
+        decrementDepth();
     }
-    return true;
+    return false;
+}
+
+bool UnfoldShortCircuitTraverser::copyLoopConditionOrExpression(TIntermNode *parent,
+                                                                TIntermTyped *node)
+{
+    if (mInLoopCondition)
+    {
+        mReplacements.push_back(
+            NodeUpdateEntry(parent, node, createTempSymbol(node->getType()), false));
+        TIntermAggregate *body = mParentLoop->getBody();
+        TIntermSequence empty;
+        if (mParentLoop->getType() == ELoopDoWhile)
+        {
+            // Declare the temporary variable before the loop.
+            TIntermSequence insertionsBeforeLoop;
+            insertionsBeforeLoop.push_back(createTempDeclaration(node->getType()));
+            insertStatementsInParentBlock(insertionsBeforeLoop);
+
+            // Move a part of do-while loop condition to inside the loop.
+            TIntermSequence insertionsInLoop;
+            insertionsInLoop.push_back(createTempAssignment(node));
+            mInsertions.push_back(NodeInsertMultipleEntry(body, body->getSequence()->size() - 1,
+                                                          empty, insertionsInLoop));
+        }
+        else
+        {
+            // The loop initializer expression and one copy of the part of the loop condition are
+            // executed before the loop. They need to be in a new scope.
+            TIntermAggregate *loopScope = new TIntermAggregate(EOpSequence);
+
+            TIntermNode *initializer = mParentLoop->getInit();
+            if (initializer != nullptr)
+            {
+                // Move the initializer to the newly created outer scope, so that condition can
+                // depend on it.
+                mReplacements.push_back(NodeUpdateEntry(mParentLoop, initializer, nullptr, false));
+                loopScope->getSequence()->push_back(initializer);
+            }
+
+            loopScope->getSequence()->push_back(createTempInitDeclaration(node));
+            loopScope->getSequence()->push_back(mParentLoop);
+            mReplacements.push_back(NodeUpdateEntry(mLoopParent, mParentLoop, loopScope, true));
+
+            // The second copy of the part of the loop condition is executed inside the loop.
+            TIntermSequence insertionsInLoop;
+            insertionsInLoop.push_back(createTempAssignment(node->deepCopy()));
+            mInsertions.push_back(NodeInsertMultipleEntry(body, body->getSequence()->size() - 1,
+                                                          empty, insertionsInLoop));
+        }
+        return true;
+    }
+
+    if (mInLoopExpression)
+    {
+        TIntermTyped *movedExpression = mParentLoop->getExpression();
+        mReplacements.push_back(NodeUpdateEntry(mParentLoop, movedExpression, nullptr, false));
+        TIntermAggregate *body = mParentLoop->getBody();
+        TIntermSequence empty;
+        TIntermSequence insertions;
+        insertions.push_back(movedExpression);
+        mInsertions.push_back(
+            NodeInsertMultipleEntry(body, body->getSequence()->size() - 1, empty, insertions));
+        return true;
+    }
+    return false;
 }
 
 void UnfoldShortCircuitTraverser::nextIteration()