Fold user-definedness of function nodes into TOperator

src/compiler/translator/ASTMetadataHLSL.cpp

@@ -102,27 +102,23 @@ class PullGradient : public TIntermTraverser
     {
         if (visit == PreVisit)
         {
-            if (node->getOp() == EOpFunctionCall)
+            if (node->getOp() == EOpCallFunctionInAST)
             {
-                if (node->isUserDefined())
-                {
-                    size_t calleeIndex = mDag.findIndex(node->getFunctionSymbolInfo());
-                    ASSERT(calleeIndex != CallDAG::InvalidIndex && calleeIndex < mIndex);
+                size_t calleeIndex = mDag.findIndex(node->getFunctionSymbolInfo());
+                ASSERT(calleeIndex != CallDAG::InvalidIndex && calleeIndex < mIndex);
 
-                    if ((*mMetadataList)[calleeIndex].mUsesGradient)
-                    {
-                        onGradient();
-                    }
-                }
-                else
+                if ((*mMetadataList)[calleeIndex].mUsesGradient)
                 {
-                    TString name =
-                        TFunction::unmangleName(node->getFunctionSymbolInfo()->getName());
+                    onGradient();
+                }
+            }
+            else if (node->getOp() == EOpCallBuiltInFunction)
+            {
+                TString name = TFunction::unmangleName(node->getFunctionSymbolInfo()->getName());
 
-                    if (name == "texture2D" || name == "texture2DProj" || name == "textureCube")
-                    {
-                        onGradient();
-                    }
+                if (name == "texture2D" || name == "texture2DProj" || name == "textureCube")
+                {
+                    onGradient();
                 }
             }
         }
@@ -273,17 +269,14 @@ class PullComputeDiscontinuousAndGradientLoops : public TIntermTraverser
 
     bool visitAggregate(Visit visit, TIntermAggregate *node) override
     {
-        if (visit == PreVisit && node->getOp() == EOpFunctionCall)
+        if (visit == PreVisit && node->getOp() == EOpCallFunctionInAST)
         {
-            if (node->isUserDefined())
-            {
-                size_t calleeIndex = mDag.findIndex(node->getFunctionSymbolInfo());
-                ASSERT(calleeIndex != CallDAG::InvalidIndex && calleeIndex < mIndex);
+            size_t calleeIndex = mDag.findIndex(node->getFunctionSymbolInfo());
+            ASSERT(calleeIndex != CallDAG::InvalidIndex && calleeIndex < mIndex);
 
-                if ((*mMetadataList)[calleeIndex].mHasGradientLoopInCallGraph)
-                {
-                    onGradientLoop();
-                }
+            if ((*mMetadataList)[calleeIndex].mHasGradientLoopInCallGraph)
+            {
+                onGradientLoop();
             }
         }
 
@@ -354,8 +347,8 @@ class PushDiscontinuousLoops : public TIntermTraverser
     {
         switch (node->getOp())
         {
-            case EOpFunctionCall:
-                if (visit == PreVisit && node->isUserDefined() && mNestedDiscont > 0)
+            case EOpCallFunctionInAST:
+                if (visit == PreVisit && mNestedDiscont > 0)
                 {
                     size_t calleeIndex = mDag.findIndex(node->getFunctionSymbolInfo());
                     ASSERT(calleeIndex != CallDAG::InvalidIndex && calleeIndex < mIndex);

src/compiler/translator/ArrayReturnValueToOutParameter.cpp

@@ -43,9 +43,8 @@ TIntermSymbol *CreateReturnValueOutSymbol(const TType &type)
 TIntermAggregate *CreateReplacementCall(TIntermAggregate *originalCall,
                                         TIntermTyped *returnValueTarget)
 {
-    TIntermAggregate *replacementCall = new TIntermAggregate(EOpFunctionCall);
+    TIntermAggregate *replacementCall = new TIntermAggregate(EOpCallFunctionInAST);
     replacementCall->setType(TType(EbtVoid));
-    replacementCall->setUserDefined();
     *replacementCall->getFunctionSymbolInfo() = *originalCall->getFunctionSymbolInfo();
     replacementCall->setLine(originalCall->getLine());
     TIntermSequence *replacementParameters = replacementCall->getSequence();
@@ -124,7 +123,8 @@ bool ArrayReturnValueToOutParameterTraverser::visitFunctionPrototype(Visit visit
 
 bool ArrayReturnValueToOutParameterTraverser::visitAggregate(Visit visit, TIntermAggregate *node)
 {
-    if (visit == PreVisit && node->isArray() && node->getOp() == EOpFunctionCall)
+    ASSERT(!node->isArray() || node->getOp() != EOpCallInternalRawFunction);
+    if (visit == PreVisit && node->isArray() && node->getOp() == EOpCallFunctionInAST)
     {
         // Handle call sites where the returned array is not assigned.
         // Examples where f() is a function returning an array:
@@ -181,8 +181,8 @@ bool ArrayReturnValueToOutParameterTraverser::visitBinary(Visit visit, TIntermBi
     if (node->getOp() == EOpAssign && node->getLeft()->isArray())
     {
         TIntermAggregate *rightAgg = node->getRight()->getAsAggregate();
-        if (rightAgg != nullptr && rightAgg->getOp() == EOpFunctionCall &&
-            rightAgg->isUserDefined())
+        ASSERT(rightAgg == nullptr || rightAgg->getOp() != EOpCallInternalRawFunction);
+        if (rightAgg != nullptr && rightAgg->getOp() == EOpCallFunctionInAST)
         {
             TIntermAggregate *replacementCall = CreateReplacementCall(rightAgg, node->getLeft());
             queueReplacement(node, replacementCall, OriginalNode::IS_DROPPED);

src/compiler/translator/BuiltInFunctionEmulator.cpp

@@ -38,7 +38,7 @@ class BuiltInFunctionEmulator::BuiltInFunctionEmulationMarker : public TIntermTr
         {
             // Here we handle all the built-in functions mapped to ops, not just the ones that are
             // currently identified as problematic.
-            if (node->isConstructor() || node->getOp() == EOpFunctionCall)
+            if (node->isConstructor() || node->isFunctionCall())
             {
                 return true;
             }

src/compiler/translator/CallDAG.cpp

@@ -136,30 +136,17 @@ class CallDAG::CallDAGCreator : public TIntermTraverser
     // Aggregates the AST node for each function as well as the name of the functions called by it
     bool visitAggregate(Visit visit, TIntermAggregate *node) override
     {
-        switch (node->getOp())
+        if (visit == PreVisit && node->getOp() == EOpCallFunctionInAST)
         {
-            case EOpFunctionCall:
+            // Function call, add the callees
+            auto it = mFunctions.find(node->getFunctionSymbolInfo()->getName());
+            ASSERT(it != mFunctions.end());
+
+            // We might be in a top-level function call to set a global variable
+            if (mCurrentFunction)
             {
-                // Function call, add the callees
-                if (visit == PreVisit)
-                {
-                    // Do not handle calls to builtin functions
-                    if (node->isUserDefined())
-                    {
-                        auto it = mFunctions.find(node->getFunctionSymbolInfo()->getName());
-                        ASSERT(it != mFunctions.end());
-
-                        // We might be in a top-level function call to set a global variable
-                        if (mCurrentFunction)
-                        {
-                            mCurrentFunction->callees.insert(&it->second);
-                        }
-                    }
-                }
-                break;
+                mCurrentFunction->callees.insert(&it->second);
             }
-            default:
-                break;
         }
         return true;
     }

src/compiler/translator/DeferGlobalInitializers.cpp

@@ -50,9 +50,7 @@ TIntermFunctionDefinition *CreateFunctionDefinitionNode(const char *name,
 
 TIntermAggregate *CreateFunctionCallNode(const char *name, const int functionId)
 {
-    TIntermAggregate *functionNode = new TIntermAggregate(EOpFunctionCall);
-
-    functionNode->setUserDefined();
+    TIntermAggregate *functionNode = new TIntermAggregate(EOpCallFunctionInAST);
     SetInternalFunctionName(functionNode->getFunctionSymbolInfo(), name);
     TType returnType(EbtVoid);
     functionNode->setType(returnType);

src/compiler/translator/EmulatePrecision.cpp

@@ -429,7 +429,7 @@ bool canRoundFloat(const TType &type)
 
 TIntermAggregate *createInternalFunctionCallNode(TString name, TIntermNode *child)
 {
-    TIntermAggregate *callNode = new TIntermAggregate(EOpFunctionCall);
+    TIntermAggregate *callNode = new TIntermAggregate(EOpCallInternalRawFunction);
     TName nameObj(TFunction::mangleName(name));
     nameObj.setInternal(true);
     callNode->getFunctionSymbolInfo()->setNameObj(nameObj);
@@ -638,24 +638,11 @@ bool EmulatePrecision::visitAggregate(Visit visit, TIntermAggregate *node)
     switch (node->getOp())
     {
         case EOpConstructStruct:
+        case EOpCallInternalRawFunction:
+        case EOpCallFunctionInAST:
+            // User-defined function return values are not rounded. The calculations that produced
+            // the value inside the function definition should have been rounded.
             break;
-        case EOpFunctionCall:
-        {
-            // Function call.
-            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()) && !isInFunctionMap(node) &&
-                    parentUsesResult(parent, node))
-                {
-                    TIntermNode *replacement = createRoundingFunctionCallNode(node);
-                    queueReplacement(node, replacement, OriginalNode::BECOMES_CHILD);
-                }
-            }
-            break;
-        }
         default:
             TIntermNode *parent = getParentNode();
             if (canRoundFloat(node->getType()) && visit == PreVisit &&

src/compiler/translator/IntermNode.cpp

@@ -309,7 +309,7 @@ void TIntermAggregate::setPrecisionFromChildren()
 void TIntermAggregate::setPrecisionForBuiltInOp()
 {
     ASSERT(!isConstructor());
-    ASSERT(mOp != EOpFunctionCall);
+    ASSERT(!isFunctionCall());
     if (!setPrecisionForSpecialBuiltInOp())
     {
         setPrecisionFromChildren();
@@ -340,7 +340,7 @@ void TIntermAggregate::setBuiltInFunctionPrecision()
 {
     // All built-ins returning bool should be handled as ops, not functions.
     ASSERT(getBasicType() != EbtBool);
-    ASSERT(getOp() == EOpFunctionCall);
+    ASSERT(mOp == EOpCallBuiltInFunction);
 
     TPrecision precision                = EbpUndefined;
     TIntermSequence::iterator childIter = mSequence.begin();
@@ -549,7 +549,6 @@ void TFunctionSymbolInfo::setFromFunction(const TFunction &function)
 
 TIntermAggregate::TIntermAggregate(const TIntermAggregate &node)
     : TIntermOperator(node),
-      mUserDefined(node.mUserDefined),
       mUseEmulatedFunction(node.mUseEmulatedFunction),
       mGotPrecisionFromChildren(node.mGotPrecisionFromChildren),
       mFunctionInfo(node.mFunctionInfo)
@@ -659,6 +658,19 @@ bool TIntermOperator::isConstructor() const
     }
 }
 
+bool TIntermOperator::isFunctionCall() const
+{
+    switch (mOp)
+    {
+        case EOpCallFunctionInAST:
+        case EOpCallBuiltInFunction:
+        case EOpCallInternalRawFunction:
+            return true;
+        default:
+            return false;
+    }
+}
+
 TOperator TIntermBinary::GetMulOpBasedOnOperands(const TType &left, const TType &right)
 {
     if (left.isMatrix())

src/compiler/translator/IntermNode.h

@@ -411,6 +411,10 @@ class TIntermOperator : public TIntermTyped
     bool isMultiplication() const;
     bool isConstructor() const;
 
+    // Returns true for calls mapped to EOpCall*, false for built-ins that have their own specific
+    // ops.
+    bool isFunctionCall() const;
+
     bool hasSideEffects() const override { return isAssignment(); }
 
   protected:
@@ -547,6 +551,7 @@ class TFunctionSymbolInfo
 
     void setFromFunction(const TFunction &function);
 
+    // The name stored here should always be mangled.
     void setNameObj(const TName &name) { mName = name; }
     const TName &getNameObj() const { return mName; }
 
@@ -590,7 +595,6 @@ class TIntermAggregate : public TIntermOperator, public TIntermAggregateBase
   public:
     TIntermAggregate(TOperator op)
         : TIntermOperator(op),
-          mUserDefined(false),
           mUseEmulatedFunction(false),
           mGotPrecisionFromChildren(false)
     {
@@ -613,9 +617,6 @@ class TIntermAggregate : public TIntermOperator, public TIntermAggregateBase
     TIntermSequence *getSequence() override { return &mSequence; }
     const TIntermSequence *getSequence() const override { return &mSequence; }
 
-    void setUserDefined() { mUserDefined = true; }
-    bool isUserDefined() const { return mUserDefined; }
-
     void setUseEmulatedFunction() { mUseEmulatedFunction = true; }
     bool getUseEmulatedFunction() { return mUseEmulatedFunction; }
 
@@ -625,7 +626,7 @@ class TIntermAggregate : public TIntermOperator, public TIntermAggregateBase
 
     void setPrecisionFromChildren();
 
-    // Used for built-in functions under EOpFunctionCall.
+    // Used for built-in functions under EOpCallBuiltInFunction.
     void setBuiltInFunctionPrecision();
 
     // Returns true if changing parameter precision may affect the return value.
@@ -636,10 +637,9 @@ class TIntermAggregate : public TIntermOperator, public TIntermAggregateBase
 
   protected:
     TIntermSequence mSequence;
-    bool mUserDefined;  // used for user defined function names
 
     // If set to true, replace the built-in function call with an emulated one
-    // to work around driver bugs.
+    // to work around driver bugs. Only for calls mapped to ops other than EOpCall*.
     bool mUseEmulatedFunction;
 
     bool mGotPrecisionFromChildren;
@@ -1188,10 +1188,6 @@ class TLValueTrackingTraverser : public TIntermTraverser
         return mOperatorRequiresLValue || mInFunctionCallOutParameter;
     }
 
-    // Return true if the prototype or definition of the function being called has been encountered
-    // during traversal.
-    bool isInFunctionMap(const TIntermAggregate *callNode) const;
-
   private:
     // Track whether an l-value is required in the node that is currently being traversed by the
     // surrounding operator.
@@ -1205,6 +1201,10 @@ class TLValueTrackingTraverser : public TIntermTraverser
     // Add a function encountered during traversal to the function map.
     void addToFunctionMap(const TName &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);
 

src/compiler/translator/IntermNodePatternMatcher.cpp

@@ -87,8 +87,7 @@ bool IntermNodePatternMatcher::match(TIntermAggregate *node, TIntermNode *parent
                  (parentBinary->getOp() == EOpAssign || parentBinary->getOp() == EOpInitialize));
 
             if (node->getType().isArray() && !parentIsAssignment &&
-                (node->isConstructor() || node->getOp() == EOpFunctionCall) &&
-                !parentNode->getAsBlock())
+                (node->isConstructor() || node->isFunctionCall()) && !parentNode->getAsBlock())
             {
                 return true;
             }

src/compiler/translator/IntermTraverse.cpp

@@ -235,7 +235,7 @@ void TLValueTrackingTraverser::addToFunctionMap(const TName &name, TIntermSequen
 
 bool TLValueTrackingTraverser::isInFunctionMap(const TIntermAggregate *callNode) const
 {
-    ASSERT(callNode->getOp() == EOpFunctionCall);
+    ASSERT(callNode->getOp() == EOpCallFunctionInAST);
     return (mFunctionMap.find(callNode->getFunctionSymbolInfo()->getNameObj()) !=
             mFunctionMap.end());
 }
@@ -641,36 +641,43 @@ void TLValueTrackingTraverser::traverseAggregate(TIntermAggregate *node)
 
     if (visit)
     {
-        bool inFunctionMap = false;
-        if (node->getOp() == EOpFunctionCall)
+        if (node->getOp() == EOpCallFunctionInAST)
         {
-            inFunctionMap = isInFunctionMap(node);
-            if (!inFunctionMap)
+            if (isInFunctionMap(node))
             {
-                // The function is not user-defined - it is likely built-in texture function.
-                // Assume that those do not have out parameters.
-                setInFunctionCallOutParameter(false);
+                TIntermSequence *params             = getFunctionParameters(node);
+                TIntermSequence::iterator paramIter = params->begin();
+                for (auto *child : *sequence)
+                {
+                    ASSERT(paramIter != params->end());
+                    TQualifier qualifier = (*paramIter)->getAsTyped()->getQualifier();
+                    setInFunctionCallOutParameter(qualifier == EvqOut || qualifier == EvqInOut);
+
+                    child->traverse(this);
+                    if (visit && inVisit)
+                    {
+                        if (child != sequence->back())
+                            visit = visitAggregate(InVisit, node);
+                    }
+
+                    ++paramIter;
+                }
             }
-        }
-
-        if (inFunctionMap)
-        {
-            TIntermSequence *params             = getFunctionParameters(node);
-            TIntermSequence::iterator paramIter = params->begin();
-            for (auto *child : *sequence)
+            else
             {
-                ASSERT(paramIter != params->end());
-                TQualifier qualifier = (*paramIter)->getAsTyped()->getQualifier();
-                setInFunctionCallOutParameter(qualifier == EvqOut || qualifier == EvqInOut);
-
-                child->traverse(this);
-                if (visit && inVisit)
+                // The node might not be in the function map in case we're in the middle of
+                // transforming the AST, and have inserted function call nodes without inserting the
+                // function definitions yet.
+                setInFunctionCallOutParameter(false);
+                for (auto *child : *sequence)
                 {
-                    if (child != sequence->back())
-                        visit = visitAggregate(InVisit, node);
+                    child->traverse(this);
+                    if (visit && inVisit)
+                    {
+                        if (child != sequence->back())
+                            visit = visitAggregate(InVisit, node);
+                    }
                 }
-
-                ++paramIter;
             }
 
             setInFunctionCallOutParameter(false);
@@ -680,7 +687,7 @@ void TLValueTrackingTraverser::traverseAggregate(TIntermAggregate *node)
             // Find the built-in function corresponding to this op so that we can determine the
             // in/out qualifiers of its parameters.
             TFunction *builtInFunc = nullptr;
-            if (!node->isConstructor() && node->getOp() != EOpFunctionCall)
+            if (!node->isFunctionCall() && !node->isConstructor())
             {
                 builtInFunc = mSymbolTable.findBuiltInOp(node, mShaderVersion);
             }
@@ -689,6 +696,8 @@ void TLValueTrackingTraverser::traverseAggregate(TIntermAggregate *node)
 
             for (auto *child : *sequence)
             {
+                // This assumes that raw functions called with
+                // EOpCallInternalRawFunction don't have out parameters.
                 TQualifier qualifier = EvqIn;
                 if (builtInFunc != nullptr)
                     qualifier = builtInFunc->getParam(paramIndex).type->getQualifier();

src/compiler/translator/Operator.cpp

@@ -10,7 +10,7 @@ const char *GetOperatorString(TOperator op)
 {
     switch (op)
     {
-        // Note: EOpNull and EOpFunctionCall can't be handled here.
+        // Note: EOpNull and EOpCall* can't be handled here.
 
         case EOpNegative:
             return "-";

src/compiler/translator/Operator.h

@@ -13,7 +13,20 @@
 enum TOperator
 {
     EOpNull,  // if in a node, should only mean a node is still being built
-    EOpFunctionCall,
+
+    // Call a function defined in the AST. This might be a user-defined function or a function
+    // inserted by an AST transformation.
+    EOpCallFunctionInAST,
+
+    // Call an internal helper function with a raw implementation - the implementation can't be
+    // subject to AST transformations. Raw functions have a few constraints to keep them compatible
+    // with AST traversers:
+    // * They should not return arrays.
+    // * They should not have out parameters.
+    EOpCallInternalRawFunction,
+
+    // Call a built-in function like a texture or image function.
+    EOpCallBuiltInFunction,
 
     //
     // Unary operators

src/compiler/translator/OutputGLSLBase.cpp

@@ -877,7 +877,9 @@ bool TOutputGLSLBase::visitAggregate(Visit visit, TIntermAggregate *node)
     TInfoSinkBase &out       = objSink();
     switch (node->getOp())
     {
-        case EOpFunctionCall:
+        case EOpCallFunctionInAST:
+        case EOpCallInternalRawFunction:
+        case EOpCallBuiltInFunction:
             // Function call.
             if (visit == PreVisit)
                 out << hashFunctionNameIfNeeded(node->getFunctionSymbolInfo()->getNameObj()) << "(";

src/compiler/translator/OutputHLSL.cpp

@@ -1000,7 +1000,7 @@ bool OutputHLSL::visitBinary(Visit visit, TIntermBinary *node)
                 }
                 // ArrayReturnValueToOutParameter should have eliminated expressions where a
                 // function call is assigned.
-                ASSERT(rightAgg == nullptr || rightAgg->getOp() != EOpFunctionCall);
+                ASSERT(rightAgg == nullptr);
 
                 const TString &functionName = addArrayAssignmentFunction(node->getType());
                 outputTriplet(out, visit, (functionName + "(").c_str(), ", ", ")");
@@ -1755,12 +1755,14 @@ bool OutputHLSL::visitAggregate(Visit visit, TIntermAggregate *node)
 
     switch (node->getOp())
     {
-        case EOpFunctionCall:
+        case EOpCallBuiltInFunction:
+        case EOpCallFunctionInAST:
+        case EOpCallInternalRawFunction:
         {
             TIntermSequence *arguments = node->getSequence();
 
             bool lod0 = mInsideDiscontinuousLoop || mOutputLod0Function;
-            if (node->isUserDefined())
+            if (node->getOp() == EOpCallFunctionInAST)
             {
                 if (node->isArray())
                 {
@@ -1774,7 +1776,7 @@ bool OutputHLSL::visitAggregate(Visit visit, TIntermAggregate *node)
                 out << DisambiguateFunctionName(node->getSequence());
                 out << (lod0 ? "Lod0(" : "(");
             }
-            else if (node->getFunctionSymbolInfo()->getNameObj().isInternal())
+            else if (node->getOp() == EOpCallInternalRawFunction)
             {
                 // This path is used for internal functions that don't have their definitions in the
                 // AST, such as precision emulation functions.

src/compiler/translator/ParseContext.cpp

@@ -4180,7 +4180,7 @@ TIntermBranch *TParseContext::addBranch(TOperator op,
 
 void TParseContext::checkTextureOffsetConst(TIntermAggregate *functionCall)
 {
-    ASSERT(!functionCall->isUserDefined());
+    ASSERT(functionCall->getOp() == EOpCallBuiltInFunction);
     const TString &name        = functionCall->getFunctionSymbolInfo()->getName();
     TIntermNode *offset        = nullptr;
     TIntermSequence *arguments = functionCall->getSequence();
@@ -4232,7 +4232,7 @@ void TParseContext::checkTextureOffsetConst(TIntermAggregate *functionCall)
 // GLSL ES 3.10 Revision 4, 4.9 Memory Access Qualifiers
 void TParseContext::checkImageMemoryAccessForBuiltinFunctions(TIntermAggregate *functionCall)
 {
-    ASSERT(!functionCall->isUserDefined());
+    ASSERT(functionCall->getOp() == EOpCallBuiltInFunction);
     const TString &name = functionCall->getFunctionSymbolInfo()->getName();
 
     if (name.compare(0, 5, "image") == 0)
@@ -4269,7 +4269,7 @@ void TParseContext::checkImageMemoryAccessForUserDefinedFunctions(
     const TFunction *functionDefinition,
     const TIntermAggregate *functionCall)
 {
-    ASSERT(functionCall->isUserDefined());
+    ASSERT(functionCall->getOp() == EOpCallFunctionInAST);
 
     const TIntermSequence &arguments = *functionCall->getSequence();
 
@@ -4463,29 +4463,24 @@ TIntermTyped *TParseContext::addFunctionCallOrMethod(TFunction *fnCall,
             {
                 // This is a real function call
                 ASSERT(argumentsNode->getOp() == EOpNull);
-                argumentsNode->setOp(EOpFunctionCall);
                 argumentsNode->setType(fnCandidate->getReturnType());
-
-                // this is how we know whether the given function is a builtIn function or a user
-                // defined function
-                // if builtIn == false, it's a userDefined -> could be an overloaded
-                // builtIn function also
-                // if builtIn == true, it's definitely a builtIn function with EOpNull
-                if (!builtIn)
-                    argumentsNode->setUserDefined();
                 argumentsNode->getFunctionSymbolInfo()->setFromFunction(*fnCandidate);
 
-                // This needs to happen after the function info including name is set
+                // If builtIn == false, the function is user defined - could be an overloaded
+                // built-in as well.
+                // if builtIn == true, it's a builtIn function with no op associated with it.
+                // This needs to happen after the function info including name is set.
                 if (builtIn)
                 {
+                    argumentsNode->setOp(EOpCallBuiltInFunction);
                     argumentsNode->setBuiltInFunctionPrecision();
 
                     checkTextureOffsetConst(argumentsNode);
-
                     checkImageMemoryAccessForBuiltinFunctions(argumentsNode);
                 }
                 else
                 {
+                    argumentsNode->setOp(EOpCallFunctionInAST);
                     checkImageMemoryAccessForUserDefinedFunctions(fnCandidate, argumentsNode);
                 }
 

src/compiler/translator/RemoveDynamicIndexing.cpp

@@ -55,6 +55,8 @@ TName GetIndexFunctionName(const TType &type, bool write)
     }
     TString nameString = TFunction::mangleName(nameSink.c_str());
     TName name(nameString);
+    // TODO(oetuaho@nvidia.com): would be better to have the parameter types in the mangled name as
+    // well.
     name.setInternal(true);
     return name;
 }
@@ -349,9 +351,8 @@ TIntermAggregate *CreateIndexFunctionCall(TIntermBinary *node,
                                           TIntermTyped *index)
 {
     ASSERT(node->getOp() == EOpIndexIndirect);
-    TIntermAggregate *indexingCall = new TIntermAggregate(EOpFunctionCall);
+    TIntermAggregate *indexingCall = new TIntermAggregate(EOpCallFunctionInAST);
     indexingCall->setLine(node->getLine());
-    indexingCall->setUserDefined();
     indexingCall->getFunctionSymbolInfo()->setNameObj(
         GetIndexFunctionName(indexedNode->getType(), false));
     indexingCall->getSequence()->push_back(indexedNode);
@@ -520,6 +521,11 @@ void RemoveDynamicIndexing(TIntermNode *root,
         root->traverse(&traverser);
         traverser.updateTree();
     } while (traverser.usedTreeInsertion());
+    // TOOD(oetuaho@nvidia.com): It might be nicer to add the helper definitions also in the middle
+    // of traversal. Now the tree ends up in an inconsistent state in the middle, since there are
+    // function call nodes with no corresponding definition nodes. This needs special handling in
+    // TIntermLValueTrackingTraverser, and creates intricacies that are not easily apparent from a
+    // superficial reading of the code.
     traverser.insertHelperDefinitions(root);
     traverser.updateTree();
 }

src/compiler/translator/RewriteTexelFetchOffset.cpp

@@ -66,7 +66,7 @@ bool Traverser::visitAggregate(Visit visit, TIntermAggregate *node)
     }
 
     // Decide if the node represents the call of texelFetchOffset.
-    if (node->getOp() != EOpFunctionCall || node->isUserDefined())
+    if (node->getOp() != EOpCallBuiltInFunction)
     {
         return true;
     }
@@ -94,7 +94,7 @@ bool Traverser::visitAggregate(Visit visit, TIntermAggregate *node)
 
     // Create new node that represents the call of function texelFetch.
     // Its argument list will be: texelFetch(sampler, Position+offset, lod).
-    TIntermAggregate *texelFetchNode = new TIntermAggregate(EOpFunctionCall);
+    TIntermAggregate *texelFetchNode = new TIntermAggregate(EOpCallBuiltInFunction);
     texelFetchNode->getFunctionSymbolInfo()->setName(newName);
     texelFetchNode->getFunctionSymbolInfo()->setId(uniqueId);
     texelFetchNode->setType(node->getType());

src/compiler/translator/SeparateExpressionsReturningArrays.cpp

@@ -63,10 +63,6 @@ TIntermAggregate *CopyAggregateNode(TIntermAggregate *node)
     copySeq->insert(copySeq->begin(), node->getSequence()->begin(), node->getSequence()->end());
     copyNode->setType(node->getType());
     *copyNode->getFunctionSymbolInfo() = *node->getFunctionSymbolInfo();
-    if (node->isUserDefined())
-    {
-        copyNode->setUserDefined();
-    }
     return copyNode;
 }
 
@@ -104,7 +100,7 @@ bool SeparateExpressionsTraverser::visitAggregate(Visit visit, TIntermAggregate
     if (!mPatternToSeparateMatcher.match(node, getParentNode()))
         return true;
 
-    ASSERT(node->isConstructor() || node->getOp() == EOpFunctionCall);
+    ASSERT(node->isConstructor() || node->getOp() == EOpCallFunctionInAST);
 
     mFoundArrayExpression = true;
 

src/compiler/translator/SymbolTable.cpp

@@ -158,7 +158,7 @@ TSymbol *TSymbolTable::findBuiltIn(const TString &name, int shaderVersion) const
 TFunction *TSymbolTable::findBuiltInOp(TIntermAggregate *callNode, int shaderVersion) const
 {
     ASSERT(!callNode->isConstructor());
-    ASSERT(callNode->getOp() != EOpFunctionCall);
+    ASSERT(!callNode->isFunctionCall());
     TString opString = GetOperatorString(callNode->getOp());
     // The return type doesn't affect the mangled name of the function, which is used to look it up.
     TType dummyReturnType;

src/compiler/translator/ValidateGlobalInitializer.cpp

@@ -72,8 +72,8 @@ bool ValidateGlobalInitializerTraverser::visitAggregate(Visit visit, TIntermAggr
     // Disallow calls to user-defined functions and texture lookup functions in global variable
     // initializers.
     // This is done simply by disabling all function calls - built-in math functions don't use
-    // EOpFunctionCall.
-    if (node->getOp() == EOpFunctionCall)
+    // the function call ops.
+    if (node->isFunctionCall())
     {
         mIsValid = false;
     }

src/compiler/translator/ValidateLimitations.cpp

@@ -108,7 +108,8 @@ bool ValidateLimitations::visitAggregate(Visit, TIntermAggregate *node)
 {
     switch (node->getOp())
     {
-        case EOpFunctionCall:
+        case EOpCallFunctionInAST:
+        case EOpCallBuiltInFunction:
             validateFunctionCall(node);
             break;
         default:
@@ -378,7 +379,7 @@ bool ValidateLimitations::validateForLoopExpr(TIntermLoop *node, int indexSymbol
 
 bool ValidateLimitations::validateFunctionCall(TIntermAggregate *node)
 {
-    ASSERT(node->getOp() == EOpFunctionCall);
+    ASSERT(node->getOp() == EOpCallFunctionInAST || node->getOp() == EOpCallBuiltInFunction);
 
     // If not within loop body, there is nothing to check.
     if (!withinLoopBody())

src/compiler/translator/ValidateMultiviewWebGL.cpp

@@ -354,27 +354,24 @@ bool ValidateMultiviewTraverser::visitAggregate(Visit visit, TIntermAggregate *n
     {
         // Check if the node is an user-defined function call or if an l-value is required, or if
         // there are possible visible side effects, such as image writes.
-        if (node->getOp() == EOpFunctionCall)
+        if (node->getOp() == EOpCallFunctionInAST)
         {
-            if (node->isUserDefined())
-            {
-                mDiagnostics->error(node->getLine(),
-                                    "Disallowed user defined function call inside assignment to "
-                                    "gl_Position.x when using OVR_multiview",
-                                    GetOperatorString(node->getOp()));
-                mValid = false;
-            }
-            else if (TFunction::unmangleName(node->getFunctionSymbolInfo()->getName()) ==
-                     "imageStore")
-            {
-                // TODO(oetuaho@nvidia.com): Record which built-in functions have side effects in
-                // the symbol info instead.
-                mDiagnostics->error(node->getLine(),
-                                    "Disallowed function call with side effects inside assignment "
-                                    "to gl_Position.x when using OVR_multiview",
-                                    GetOperatorString(node->getOp()));
-                mValid = false;
-            }
+            mDiagnostics->error(node->getLine(),
+                                "Disallowed user defined function call inside assignment to "
+                                "gl_Position.x when using OVR_multiview",
+                                GetOperatorString(node->getOp()));
+            mValid = false;
+        }
+        else if (node->getOp() == EOpCallBuiltInFunction &&
+                 TFunction::unmangleName(node->getFunctionSymbolInfo()->getName()) == "imageStore")
+        {
+            // TODO(oetuaho@nvidia.com): Record which built-in functions have side effects in
+            // the symbol info instead.
+            mDiagnostics->error(node->getLine(),
+                                "Disallowed function call with side effects inside assignment "
+                                "to gl_Position.x when using OVR_multiview",
+                                GetOperatorString(node->getOp()));
+            mValid = false;
         }
         else if (!node->isConstructor())
         {

src/compiler/translator/intermOut.cpp

@@ -403,8 +403,15 @@ bool TOutputTraverser::visitAggregate(Visit visit, TIntermAggregate *node)
         // mostly use GLSL names for functions.
         switch (node->getOp())
         {
-            case EOpFunctionCall:
-                OutputFunction(out, "Function Call", node->getFunctionSymbolInfo());
+            case EOpCallFunctionInAST:
+                OutputFunction(out, "Call an user-defined function", node->getFunctionSymbolInfo());
+                break;
+            case EOpCallInternalRawFunction:
+                OutputFunction(out, "Call an internal function with raw implementation",
+                               node->getFunctionSymbolInfo());
+                break;
+            case EOpCallBuiltInFunction:
+                OutputFunction(out, "Call a built-in function", node->getFunctionSymbolInfo());
                 break;
 
             case EOpEqualComponentWise:

src/tests/test_utils/compiler_test.cpp

@@ -27,8 +27,7 @@ class FunctionCallFinder : public TIntermTraverser
 
     bool visitAggregate(Visit visit, TIntermAggregate *node) override
     {
-        if (node->getOp() == EOpFunctionCall &&
-            node->getFunctionSymbolInfo()->getName() == mFunctionName)
+        if (node->isFunctionCall() && node->getFunctionSymbolInfo()->getName() == mFunctionName)
         {
             mNodeFound = node;
             return false;