Track where l-values are required in AST traversal

src/compiler/translator/EmulatePrecision.cpp

@@ -292,16 +292,12 @@ bool parentUsesResult(TIntermNode* parent, TIntermNode* node)
 }  // namespace anonymous
 
 EmulatePrecision::EmulatePrecision()
-    : TIntermTraverser(true, true, true),
-      mDeclaringVariables(false),
-      mInLValue(false),
-      mInFunctionCallOutParameter(false)
+    : TIntermTraverser(true, true, true), mDeclaringVariables(false)
 {}
 
 void EmulatePrecision::visitSymbol(TIntermSymbol *node)
 {
-    if (canRoundFloat(node->getType()) &&
-        !mDeclaringVariables && !mInLValue && !mInFunctionCallOutParameter)
+    if (canRoundFloat(node->getType()) && !mDeclaringVariables && !isLValueRequiredHere())
     {
         TIntermNode *parent = getParentNode();
         TIntermNode *replacement = createRoundingFunctionCallNode(node);
@@ -314,14 +310,6 @@ bool EmulatePrecision::visitBinary(Visit visit, TIntermBinary *node)
 {
     bool visitChildren = true;
 
-    if (node->isAssignment())
-    {
-        if (visit == PreVisit)
-            mInLValue = true;
-        else if (visit == InVisit)
-            mInLValue = false;
-    }
-
     TOperator op = node->getOp();
 
     // RHS of initialize is not being declared.
@@ -415,22 +403,9 @@ bool EmulatePrecision::visitAggregate(Visit visit, TIntermAggregate *node)
     {
       case EOpSequence:
       case EOpConstructStruct:
-        // No special handling
-        break;
       case EOpFunction:
-        if (visit == PreVisit)
-        {
-            const TIntermSequence &sequence = *(node->getSequence());
-            TIntermSequence::const_iterator seqIter = sequence.begin();
-            TIntermAggregate *params = (*seqIter)->getAsAggregate();
-            ASSERT(params != NULL);
-            ASSERT(params->getOp() == EOpParameters);
-            mFunctionMap[node->getName()] = params->getSequence();
-        }
         break;
       case EOpPrototype:
-        if (visit == PreVisit)
-            mFunctionMap[node->getName()] = node->getSequence();
         visitChildren = false;
         break;
       case EOpParameters:
@@ -457,50 +432,17 @@ bool EmulatePrecision::visitAggregate(Visit visit, TIntermAggregate *node)
       case EOpFunctionCall:
       {
         // Function call.
-        bool inFunctionMap = (mFunctionMap.find(node->getName()) != mFunctionMap.end());
         if (visit == PreVisit)
         {
             // User-defined function return values are not rounded, this relies on that
             // calculations producing the value were rounded.
             TIntermNode *parent = getParentNode();
-            if (canRoundFloat(node->getType()) && !inFunctionMap && parentUsesResult(parent, node))
+            if (canRoundFloat(node->getType()) && !isInFunctionMap(node) &&
+                parentUsesResult(parent, node))
             {
                 TIntermNode *replacement = createRoundingFunctionCallNode(node);
                 mReplacements.push_back(NodeUpdateEntry(parent, node, replacement, true));
             }
-
-            if (inFunctionMap)
-            {
-                mSeqIterStack.push_back(mFunctionMap[node->getName()]->begin());
-                if (mSeqIterStack.back() != mFunctionMap[node->getName()]->end())
-                {
-                    TQualifier qualifier = (*mSeqIterStack.back())->getAsTyped()->getQualifier();
-                    mInFunctionCallOutParameter = (qualifier == EvqOut || qualifier == EvqInOut);
-                }
-            }
-            else
-            {
-                // The function is not user-defined - it is likely built-in texture function.
-                // Assume that those do not have out parameters.
-                mInFunctionCallOutParameter = false;
-            }
-        }
-        else if (visit == InVisit)
-        {
-            if (inFunctionMap)
-            {
-                ++mSeqIterStack.back();
-                TQualifier qualifier = (*mSeqIterStack.back())->getAsTyped()->getQualifier();
-                mInFunctionCallOutParameter = (qualifier == EvqOut || qualifier == EvqInOut);
-            }
-        }
-        else
-        {
-            if (inFunctionMap)
-            {
-                mSeqIterStack.pop_back();
-                mInFunctionCallOutParameter = false;
-            }
         }
         break;
       }
@@ -523,15 +465,10 @@ bool EmulatePrecision::visitUnary(Visit visit, TIntermUnary *node)
       case EOpNegative:
       case EOpVectorLogicalNot:
       case EOpLogicalNot:
-        break;
       case EOpPostIncrement:
       case EOpPostDecrement:
       case EOpPreIncrement:
       case EOpPreDecrement:
-        if (visit == PreVisit)
-            mInLValue = true;
-        else if (visit == PostVisit)
-            mInLValue = false;
         break;
       default:
         if (canRoundFloat(node->getType()) && visit == PreVisit)

src/compiler/translator/EmulatePrecision.h

@@ -56,20 +56,7 @@ class EmulatePrecision : public TIntermTraverser
     EmulationSet mEmulateCompoundMul;
     EmulationSet mEmulateCompoundDiv;
 
-    // Stack of function call parameter iterators
-    std::vector<TIntermSequence::const_iterator> mSeqIterStack;
-
     bool mDeclaringVariables;
-    bool mInLValue;
-    bool mInFunctionCallOutParameter;
-
-    struct TStringComparator
-    {
-        bool operator() (const TString& a, const TString& b) const { return a.compare(b) < 0; }
-    };
-
-    // Map from function names to their parameter sequences
-    std::map<TString, TIntermSequence*, TStringComparator> mFunctionMap;
 };
 
 #endif  // COMPILER_TRANSLATOR_EMULATE_PRECISION_H_

src/compiler/translator/IntermNode.h

@@ -609,7 +609,9 @@ class TIntermTraverser : angle::NonCopyable
           postVisit(postVisit),
           mDepth(0),
           mMaxDepth(0),
-          mTemporaryIndex(nullptr)
+          mTemporaryIndex(nullptr),
+          mOperatorRequiresLValue(false),
+          mInFunctionCallOutParameter(false)
     {
     }
     virtual ~TIntermTraverser() {}
@@ -671,6 +673,35 @@ class TIntermTraverser : angle::NonCopyable
     // Start creating temporary symbols from the given temporary symbol index + 1.
     void useTemporaryIndex(unsigned int *temporaryIndex);
 
+    // Track whether an l-value is required in the node that is currently being traversed.
+    // These functions are intended to be called only from traversal functions, not from subclasses
+    // of TIntermTraverser.
+    // Use isLValueRequiredHere instead to check all conditions which require an l-value.
+    void setOperatorRequiresLValue(bool lValueRequired)
+    {
+        mOperatorRequiresLValue = lValueRequired;
+    }
+    bool operatorRequiresLValue() const { return mOperatorRequiresLValue; }
+
+    // Add a function encountered during traversal to the function map. Intended to be called only
+    // from traversal functions, not from subclasses of TIntermTraverser.
+    void addToFunctionMap(const TString &name, TIntermSequence *paramSequence);
+
+    // Return true if the prototype or definition of the function being called has been encountered
+    // during traversal.
+    bool isInFunctionMap(const TIntermAggregate *callNode) const;
+    // Return the parameters sequence from the function definition or prototype.
+    TIntermSequence *getFunctionParameters(const TIntermAggregate *callNode);
+
+    // Track whether an l-value is required inside a function call.
+    // This function is intended to be called only from traversal functions, not from traverers.
+    void setInFunctionCallOutParameter(bool inOutParameter);
+
+    bool isLValueRequiredHere() const
+    {
+        return mOperatorRequiresLValue || mInFunctionCallOutParameter;
+    }
+
   protected:
     int mDepth;
     int mMaxDepth;
@@ -771,6 +802,17 @@ class TIntermTraverser : angle::NonCopyable
     std::vector<ParentBlock> mParentBlockStack;
 
     unsigned int *mTemporaryIndex;
+
+    bool mOperatorRequiresLValue;
+    bool mInFunctionCallOutParameter;
+
+    struct TStringComparator
+    {
+        bool operator()(const TString &a, const TString &b) const { return a.compare(b) < 0; }
+    };
+
+    // Map from mangled function names to their parameter sequences
+    TMap<TString, TIntermSequence *, TStringComparator> mFunctionMap;
 };
 
 //

src/compiler/translator/IntermTraverse.cpp

@@ -96,6 +96,28 @@ void TIntermTraverser::nextTemporaryIndex()
     ++(*mTemporaryIndex);
 }
 
+void TIntermTraverser::addToFunctionMap(const TString &name, TIntermSequence *paramSequence)
+{
+    mFunctionMap[name] = paramSequence;
+}
+
+bool TIntermTraverser::isInFunctionMap(const TIntermAggregate *callNode) const
+{
+    ASSERT(callNode->getOp() == EOpFunctionCall || callNode->getOp() == EOpInternalFunctionCall);
+    return (mFunctionMap.find(callNode->getName()) != mFunctionMap.end());
+}
+
+TIntermSequence *TIntermTraverser::getFunctionParameters(const TIntermAggregate *callNode)
+{
+    ASSERT(isInFunctionMap(callNode));
+    return mFunctionMap[callNode->getName()];
+}
+
+void TIntermTraverser::setInFunctionCallOutParameter(bool inOutParameter)
+{
+    mInFunctionCallOutParameter = inOutParameter;
+}
+
 //
 // Traverse the intermediate representation tree, and
 // call a node type specific function for each node.
@@ -140,12 +162,23 @@ void TIntermBinary::traverse(TIntermTraverser *it)
     {
         it->incrementDepth(this);
 
+        if (isAssignment())
+        {
+            // Some binary operations like indexing can be inside an l-value.
+            // TODO(oetuaho@nvidia.com): Now the code doesn't unset operatorRequiresLValue for the
+            // index, fix this.
+            it->setOperatorRequiresLValue(true);
+        }
+
         if (mLeft)
             mLeft->traverse(it);
 
         if (it->inVisit)
             visit = it->visitBinary(InVisit, this);
 
+        if (isAssignment())
+            it->setOperatorRequiresLValue(false);
+
         if (visit && mRight)
             mRight->traverse(it);
 
@@ -170,9 +203,26 @@ void TIntermUnary::traverse(TIntermTraverser *it)
     if (it->preVisit)
         visit = it->visitUnary(PreVisit, this);
 
-    if (visit) {
+    if (visit)
+    {
         it->incrementDepth(this);
+
+        switch (getOp())
+        {
+            case EOpPostIncrement:
+            case EOpPostDecrement:
+            case EOpPreIncrement:
+            case EOpPreDecrement:
+                it->setOperatorRequiresLValue(true);
+                break;
+            default:
+                break;
+        }
+
         mOperand->traverse(it);
+
+        it->setOperatorRequiresLValue(false);
+
         it->decrementDepth();
     }
 
@@ -187,36 +237,87 @@ void TIntermAggregate::traverse(TIntermTraverser *it)
 {
     bool visit = true;
 
+    switch (mOp)
+    {
+        case EOpFunction:
+        {
+            TIntermAggregate *params = mSequence.front()->getAsAggregate();
+            ASSERT(params != nullptr);
+            ASSERT(params->getOp() == EOpParameters);
+            it->addToFunctionMap(mName, params->getSequence());
+            break;
+        }
+        case EOpPrototype:
+            it->addToFunctionMap(mName, &mSequence);
+            break;
+        default:
+            break;
+    }
+
     if (it->preVisit)
         visit = it->visitAggregate(PreVisit, this);
 
     if (visit)
     {
-        if (mOp == EOpSequence)
-            it->pushParentBlock(this);
+        bool inFunctionMap = false;
+        if (mOp == EOpFunctionCall)
+        {
+            inFunctionMap = it->isInFunctionMap(this);
+            if (!inFunctionMap)
+            {
+                // The function is not user-defined - it is likely built-in texture function.
+                // Assume that those do not have out parameters.
+                it->setInFunctionCallOutParameter(false);
+            }
+        }
 
         it->incrementDepth(this);
 
-        for (TIntermSequence::iterator sit = mSequence.begin();
-                sit != mSequence.end(); sit++)
+        if (inFunctionMap)
         {
-            (*sit)->traverse(it);
-
-            if (visit && it->inVisit)
+            TIntermSequence *params             = it->getFunctionParameters(this);
+            TIntermSequence::iterator paramIter = params->begin();
+            for (auto *child : mSequence)
             {
-                if (*sit != mSequence.back())
-                    visit = it->visitAggregate(InVisit, this);
+                ASSERT(paramIter != params->end());
+                TQualifier qualifier = (*paramIter)->getAsTyped()->getQualifier();
+                it->setInFunctionCallOutParameter(qualifier == EvqOut || qualifier == EvqInOut);
+
+                child->traverse(it);
+                if (visit && it->inVisit)
+                {
+                    if (child != mSequence.back())
+                        visit = it->visitAggregate(InVisit, this);
+                }
+
+                ++paramIter;
             }
+
+            it->setInFunctionCallOutParameter(false);
+        }
+        else
+        {
             if (mOp == EOpSequence)
+                it->pushParentBlock(this);
+
+            for (auto *child : mSequence)
             {
-                it->incrementParentBlockPos();
+                child->traverse(it);
+                if (visit && it->inVisit)
+                {
+                    if (child != mSequence.back())
+                        visit = it->visitAggregate(InVisit, this);
+                }
+
+                if (mOp == EOpSequence)
+                    it->incrementParentBlockPos();
             }
+
+            if (mOp == EOpSequence)
+                it->popParentBlock();
         }
 
         it->decrementDepth();
-
-        if (mOp == EOpSequence)
-            it->popParentBlock();
     }
 
     if (visit && it->postVisit)