Move the rest of the validation out of TIntermBinary::promote

src/compiler/translator/IntermNode.cpp

@@ -391,36 +391,9 @@ TIntermSelection::TIntermSelection(const TIntermSelection &node) : TIntermTyped(
     mFalseBlock = falseCopy;
 }
 
-//
-// Say whether or not an operation node changes the value of a variable.
-//
 bool TIntermOperator::isAssignment() const
 {
-    switch (mOp)
-    {
-      case EOpPostIncrement:
-      case EOpPostDecrement:
-      case EOpPreIncrement:
-      case EOpPreDecrement:
-      case EOpAssign:
-      case EOpAddAssign:
-      case EOpSubAssign:
-      case EOpMulAssign:
-      case EOpVectorTimesMatrixAssign:
-      case EOpVectorTimesScalarAssign:
-      case EOpMatrixTimesScalarAssign:
-      case EOpMatrixTimesMatrixAssign:
-      case EOpDivAssign:
-      case EOpIModAssign:
-      case EOpBitShiftLeftAssign:
-      case EOpBitShiftRightAssign:
-      case EOpBitwiseAndAssign:
-      case EOpBitwiseXorAssign:
-      case EOpBitwiseOrAssign:
-        return true;
-      default:
-        return false;
-    }
+    return IsAssignment(mOp);
 }
 
 bool TIntermOperator::isMultiplication() const
@@ -612,6 +585,12 @@ void TIntermUnary::promote(const TType *funcReturnType)
         mType.setQualifier(EvqTemporary);
 }
 
+TIntermBinary::TIntermBinary(TOperator op, TIntermTyped *left, TIntermTyped *right)
+    : TIntermOperator(op), mLeft(left), mRight(right), mAddIndexClamp(false)
+{
+    promote();
+}
+
 //
 // Establishes the type of the resultant operation, as well as
 // makes the operator the correct one for the operands.
@@ -619,17 +598,15 @@ void TIntermUnary::promote(const TType *funcReturnType)
 // For lots of operations it should already be established that the operand
 // combination is valid, but returns false if operator can't work on operands.
 //
-bool TIntermBinary::promote()
+void TIntermBinary::promote()
 {
     ASSERT(mLeft->isArray() == mRight->isArray());
 
     ASSERT(!isMultiplication() ||
            mOp == GetMulOpBasedOnOperands(mLeft->getType(), mRight->getType()));
 
-    //
     // Base assumption:  just make the type the same as the left
     // operand.  Then only deviations from this need be coded.
-    //
     setType(mLeft->getType());
 
     // The result gets promoted to the highest precision.
@@ -681,13 +658,11 @@ bool TIntermBinary::promote()
           default:
             break;
         }
-        return true;
+        return;
     }
 
     // If we reach here, at least one of the operands is vector or matrix.
     // The other operand could be a scalar, vector, or matrix.
-    // Can these two operands be combined?
-    //
     TBasicType basicType = mLeft->getBasicType();
 
     switch (mOp)
@@ -728,7 +703,6 @@ bool TIntermBinary::promote()
             break;
         case EOpAssign:
         case EOpInitialize:
-            // No more additional checks are needed.
             ASSERT((mLeft->getNominalSize() == mRight->getNominalSize()) &&
                    (mLeft->getSecondarySize() == mRight->getSecondarySize()));
             break;
@@ -750,44 +724,15 @@ bool TIntermBinary::promote()
         case EOpBitwiseAndAssign:
         case EOpBitwiseXorAssign:
         case EOpBitwiseOrAssign:
-            if ((mLeft->isMatrix() && mRight->isVector()) ||
-                (mLeft->isVector() && mRight->isMatrix()))
-            {
-                return false;
-            }
-
-            // Are the sizes compatible?
-            if (mLeft->getNominalSize() != mRight->getNominalSize() ||
-                mLeft->getSecondarySize() != mRight->getSecondarySize())
-            {
-                // If the nominal sizes of operands do not match:
-                // One of them must be a scalar.
-                if (!mLeft->isScalar() && !mRight->isScalar())
-                    return false;
-
-                // In the case of compound assignment other than multiply-assign,
-                // the right side needs to be a scalar. Otherwise a vector/matrix
-                // would be assigned to a scalar. A scalar can't be shifted by a
-                // vector either.
-                if (!mRight->isScalar() &&
-                    (isAssignment() || mOp == EOpBitShiftLeft || mOp == EOpBitShiftRight))
-                    return false;
-            }
-
-            {
-                const int secondarySize =
-                    std::max(mLeft->getSecondarySize(), mRight->getSecondarySize());
-                setType(TType(basicType, higherPrecision, resultQualifier,
-                              static_cast<unsigned char>(nominalSize),
-                              static_cast<unsigned char>(secondarySize)));
-                if (mLeft->isArray())
-                {
-                    ASSERT(mLeft->getArraySize() == mRight->getArraySize());
-                    mType.setArraySize(mLeft->getArraySize());
-                }
-            }
+        {
+            const int secondarySize =
+                std::max(mLeft->getSecondarySize(), mRight->getSecondarySize());
+            setType(TType(basicType, higherPrecision, resultQualifier,
+                          static_cast<unsigned char>(nominalSize),
+                          static_cast<unsigned char>(secondarySize)));
+            ASSERT(!mLeft->isArray() && !mRight->isArray());
             break;
-
+        }
         case EOpEqual:
         case EOpNotEqual:
         case EOpLessThan:
@@ -796,13 +741,21 @@ bool TIntermBinary::promote()
         case EOpGreaterThanEqual:
             ASSERT((mLeft->getNominalSize() == mRight->getNominalSize()) &&
                    (mLeft->getSecondarySize() == mRight->getSecondarySize()));
-            setType(TType(EbtBool, EbpUndefined));
+            setType(TType(EbtBool, EbpUndefined, resultQualifier));
             break;
 
+        case EOpIndexDirect:
+        case EOpIndexIndirect:
+        case EOpIndexDirectInterfaceBlock:
+        case EOpIndexDirectStruct:
+            // TODO (oetuaho): These ops could be handled here as well (should be done closer to the
+            // top of the function).
+            UNREACHABLE();
+            break;
         default:
-            return false;
+            UNREACHABLE();
+            break;
     }
-    return true;
 }
 
 TIntermTyped *TIntermBinary::fold(TDiagnostics *diagnostics)

src/compiler/translator/IntermNode.h

@@ -410,10 +410,9 @@ class TIntermBinary : public TIntermOperator
         : TIntermOperator(op),
           mAddIndexClamp(false) {}
 
-    TIntermBinary(TOperator op, TIntermTyped *left, TIntermTyped *right)
-        : TIntermOperator(op), mLeft(left), mRight(right), mAddIndexClamp(false)
-    {
-    }
+    // This constructor determines the type of the binary node based on the operands and op.
+    // This is only supported for math/logical ops, not indexing.
+    TIntermBinary(TOperator op, TIntermTyped *left, TIntermTyped *right);
 
     TIntermTyped *deepCopy() const override { return new TIntermBinary(*this); }
 
@@ -433,7 +432,6 @@ class TIntermBinary : public TIntermOperator
     void setRight(TIntermTyped *node) { mRight = node; }
     TIntermTyped *getLeft() const { return mLeft; }
     TIntermTyped *getRight() const { return mRight; }
-    bool promote();
     TIntermTyped *fold(TDiagnostics *diagnostics);
 
     void setAddIndexClamp() { mAddIndexClamp = true; }
@@ -447,6 +445,8 @@ class TIntermBinary : public TIntermOperator
     bool mAddIndexClamp;
 
   private:
+    void promote();
+
     TIntermBinary(const TIntermBinary &node);  // Note: not deleted, just private!
 };
 

src/compiler/translator/Operator.cpp

@@ -199,3 +199,31 @@ const char *GetOperatorString(TOperator op)
     return "";
 }
 
+bool IsAssignment(TOperator op)
+{
+    switch (op)
+    {
+        case EOpPostIncrement:
+        case EOpPostDecrement:
+        case EOpPreIncrement:
+        case EOpPreDecrement:
+        case EOpAssign:
+        case EOpAddAssign:
+        case EOpSubAssign:
+        case EOpMulAssign:
+        case EOpVectorTimesMatrixAssign:
+        case EOpVectorTimesScalarAssign:
+        case EOpMatrixTimesScalarAssign:
+        case EOpMatrixTimesMatrixAssign:
+        case EOpDivAssign:
+        case EOpIModAssign:
+        case EOpBitShiftLeftAssign:
+        case EOpBitShiftRightAssign:
+        case EOpBitwiseAndAssign:
+        case EOpBitwiseXorAssign:
+        case EOpBitwiseOrAssign:
+            return true;
+        default:
+            return false;
+    }
+}
\ No newline at end of file

src/compiler/translator/Operator.h

@@ -228,4 +228,7 @@ enum TOperator
 // Returns the string corresponding to the operator in GLSL
 const char* GetOperatorString(TOperator op);
 
+// Say whether or not a binary or unary operation changes the value of a variable.
+bool IsAssignment(TOperator op);
+
 #endif  // COMPILER_TRANSLATOR_OPERATOR_H_

src/compiler/translator/ParseContext.cpp

@@ -3533,8 +3533,10 @@ bool TParseContext::binaryOpCommonCheck(TOperator op,
         return false;
     }
 
-    // Check that type sizes match exactly on ops that require that.
-    // Also check restrictions for structs that contain arrays or samplers.
+    // Check that:
+    // 1. Type sizes match exactly on ops that require that.
+    // 2. Restrictions for structs that contain arrays or samplers are respected.
+    // 3. Arithmetic op type dimensionality restrictions for ops other than multiply are respected.
     switch (op)
     {
         case EOpAssign:
@@ -3567,6 +3569,48 @@ bool TParseContext::binaryOpCommonCheck(TOperator op,
             {
                 return false;
             }
+            break;
+        case EOpAdd:
+        case EOpSub:
+        case EOpDiv:
+        case EOpIMod:
+        case EOpBitShiftLeft:
+        case EOpBitShiftRight:
+        case EOpBitwiseAnd:
+        case EOpBitwiseXor:
+        case EOpBitwiseOr:
+        case EOpAddAssign:
+        case EOpSubAssign:
+        case EOpDivAssign:
+        case EOpIModAssign:
+        case EOpBitShiftLeftAssign:
+        case EOpBitShiftRightAssign:
+        case EOpBitwiseAndAssign:
+        case EOpBitwiseXorAssign:
+        case EOpBitwiseOrAssign:
+            if ((left->isMatrix() && right->isVector()) || (left->isVector() && right->isMatrix()))
+            {
+                return false;
+            }
+
+            // Are the sizes compatible?
+            if (left->getNominalSize() != right->getNominalSize() ||
+                left->getSecondarySize() != right->getSecondarySize())
+            {
+                // If the nominal sizes of operands do not match:
+                // One of them must be a scalar.
+                if (!left->isScalar() && !right->isScalar())
+                    return false;
+
+                // In the case of compound assignment other than multiply-assign,
+                // the right side needs to be a scalar. Otherwise a vector/matrix
+                // would be assigned to a scalar. A scalar can't be shifted by a
+                // vector either.
+                if (!right->isScalar() &&
+                    (IsAssignment(op) || op == EOpBitShiftLeft || op == EOpBitShiftRight))
+                    return false;
+            }
+            break;
         default:
             break;
     }
@@ -3671,8 +3715,6 @@ TIntermTyped *TParseContext::addBinaryMathInternal(TOperator op,
                 return nullptr;
             }
             break;
-        // Note that for bitwise ops, type checking is done in promote() to
-        // share code between ops and compound assignment
         default:
             break;
     }
@@ -3689,9 +3731,6 @@ TIntermTyped *TParseContext::addBinaryMathInternal(TOperator op,
     TIntermBinary *node = new TIntermBinary(op, left, right);
     node->setLine(loc);
 
-    if (!node->promote())
-        return nullptr;
-
     // See if we can fold constants.
     TIntermTyped *foldedNode = node->fold(&mDiagnostics);
     if (foldedNode)
@@ -3751,9 +3790,6 @@ TIntermTyped *TParseContext::createAssign(TOperator op,
         TIntermBinary *node = new TIntermBinary(op, left, right);
         node->setLine(loc);
 
-        if (!node->promote())
-            return nullptr;
-
         return node;
     }
     return nullptr;

src/compiler/translator/RemovePow.cpp

@@ -63,9 +63,6 @@ bool RemovePowTraverser::visitAggregate(Visit visit, TIntermAggregate *node)
         TOperator op       = TIntermBinary::GetMulOpBasedOnOperands(y->getType(), log->getType());
         TIntermBinary *mul = new TIntermBinary(op, y, log);
         mul->setLine(node->getLine());
-        bool valid = mul->promote();
-        UNUSED_ASSERTION_VARIABLE(valid);
-        ASSERT(valid);
 
         TIntermUnary *exp = new TIntermUnary(EOpExp2);
         exp->setOperand(mul);