Reland "Revert "moved BinaryExpression's data into IRNode""

This reverts commit 1d3e0e0.

Reason for revert: possibly causing https://task-scheduler.skia.org/task/F1DoniJAddPuEkH9ETTE

Original change's description:
> Revert "Revert "moved BinaryExpression's data into IRNode""
> 
> This reverts commit b61c3a9.
> 
> Change-Id: I4689e1f4977fab3233ff492cee06fbc301b5c689
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317386
> Reviewed-by: John Stiles <[email protected]>
> Commit-Queue: Ethan Nicholas <[email protected]>

[email protected],[email protected],[email protected]

# Not skipping CQ checks because this is a reland.

Change-Id: Id0f3f211f09fbf31b626c648ed141fc6154a450c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317395
Reviewed-by: Ethan Nicholas <[email protected]>
Commit-Queue: Ethan Nicholas <[email protected]>

Author: Ethan Nicholas

gn/sksl.gni

@@ -68,7 +68,6 @@ skia_sksl_sources = [
   "$_src/sksl/ir/SkSLFunctionCall.h",
   "$_src/sksl/ir/SkSLFunctionDefinition.h",
   "$_src/sksl/ir/SkSLFunctionReference.h",
-  "$_src/sksl/ir/SkSLIRNode.cpp",
   "$_src/sksl/ir/SkSLIRNode.h",
   "$_src/sksl/ir/SkSLIfStatement.h",
   "$_src/sksl/ir/SkSLIndexExpression.h",

src/sksl/SkSLASTNode.h

@@ -12,11 +12,15 @@
 #include "src/sksl/SkSLString.h"
 #include "src/sksl/ir/SkSLModifiers.h"
 
-#include <algorithm>
 #include <vector>
 
 namespace SkSL {
 
+// std::max isn't constexpr in some compilers
+static constexpr size_t Max(size_t a, size_t b) {
+    return a > b ? a : b;
+}
+
 /**
  * Represents a node in the abstract syntax tree (AST). The AST is based directly on the parse tree;
  * it is a parsed-but-not-yet-analyzed version of the program.
@@ -263,18 +267,18 @@ struct ASTNode {
     };
 
     struct NodeData {
-        char fBytes[std::max(sizeof(Token),
-                    std::max(sizeof(StringFragment),
-                    std::max(sizeof(bool),
-                    std::max(sizeof(SKSL_INT),
-                    std::max(sizeof(SKSL_FLOAT),
-                    std::max(sizeof(Modifiers),
-                    std::max(sizeof(TypeData),
-                    std::max(sizeof(FunctionData),
-                    std::max(sizeof(ParameterData),
-                    std::max(sizeof(VarData),
-                    std::max(sizeof(InterfaceBlockData),
-                             sizeof(SectionData))))))))))))];
+        char fBytes[Max(sizeof(Token),
+                    Max(sizeof(StringFragment),
+                    Max(sizeof(bool),
+                    Max(sizeof(SKSL_INT),
+                    Max(sizeof(SKSL_FLOAT),
+                    Max(sizeof(Modifiers),
+                    Max(sizeof(TypeData),
+                    Max(sizeof(FunctionData),
+                    Max(sizeof(ParameterData),
+                    Max(sizeof(VarData),
+                    Max(sizeof(InterfaceBlockData),
+                        sizeof(SectionData))))))))))))];
 
         enum class Kind {
             kToken,

src/sksl/SkSLAnalysis.cpp

@@ -266,7 +266,7 @@ bool ProgramVisitor::visitExpression(const Expression& e) {
             return false;
         case Expression::Kind::kBinary: {
             const BinaryExpression& b = e.as<BinaryExpression>();
-            return this->visitExpression(b.left()) || this->visitExpression(b.right()); }
+            return this->visitExpression(*b.fLeft) || this->visitExpression(*b.fRight); }
         case Expression::Kind::kConstructor: {
             const Constructor& c = e.as<Constructor>();
             for (const auto& arg : c.fArguments) {

src/sksl/SkSLByteCodeGenerator.cpp

@@ -681,29 +681,28 @@ void ByteCodeGenerator::writeTypedInstruction(const Type& type,
 }
 
 bool ByteCodeGenerator::writeBinaryExpression(const BinaryExpression& b, bool discard) {
-    const Expression& left = b.left();
-    const Expression& right = b.right();
-    Token::Kind op = b.getOperator();
-    if (op == Token::Kind::TK_EQ) {
-        std::unique_ptr<LValue> lvalue = this->getLValue(left);
-        this->writeExpression(right);
+    if (b.fOperator == Token::Kind::TK_EQ) {
+        std::unique_ptr<LValue> lvalue = this->getLValue(*b.fLeft);
+        this->writeExpression(*b.fRight);
         lvalue->store(discard);
         discard = false;
         return discard;
     }
-    const Type& lType = left.type();
-    const Type& rType = right.type();
+    const Type& lType = b.fLeft->type();
+    const Type& rType = b.fRight->type();
     bool lVecOrMtx = (lType.typeKind() == Type::TypeKind::kVector ||
                       lType.typeKind() == Type::TypeKind::kMatrix);
     bool rVecOrMtx = (rType.typeKind() == Type::TypeKind::kVector ||
                       rType.typeKind() == Type::TypeKind::kMatrix);
+    Token::Kind op;
     std::unique_ptr<LValue> lvalue;
-    if (Compiler::IsAssignment(op)) {
-        lvalue = this->getLValue(left);
+    if (Compiler::IsAssignment(b.fOperator)) {
+        lvalue = this->getLValue(*b.fLeft);
         lvalue->load();
-        op = Compiler::RemoveAssignment(op);
+        op = Compiler::RemoveAssignment(b.fOperator);
     } else {
-        this->writeExpression(left);
+        this->writeExpression(*b.fLeft);
+        op = b.fOperator;
         if (!lVecOrMtx && rVecOrMtx) {
             for (int i = SlotCount(rType); i > 1; --i) {
                 this->write(ByteCodeInstruction::kDup, 1);
@@ -719,7 +718,7 @@ bool ByteCodeGenerator::writeBinaryExpression(const BinaryExpression& b, bool di
             this->write(ByteCodeInstruction::kMaskPush);
             this->write(ByteCodeInstruction::kBranchIfAllFalse);
             DeferredLocation falseLocation(this);
-            this->writeExpression(right);
+            this->writeExpression(*b.fRight);
             this->write(ByteCodeInstruction::kAndB, 1);
             falseLocation.set();
             this->write(ByteCodeInstruction::kMaskPop);
@@ -732,7 +731,7 @@ bool ByteCodeGenerator::writeBinaryExpression(const BinaryExpression& b, bool di
             this->write(ByteCodeInstruction::kMaskPush);
             this->write(ByteCodeInstruction::kBranchIfAllFalse);
             DeferredLocation falseLocation(this);
-            this->writeExpression(right);
+            this->writeExpression(*b.fRight);
             this->write(ByteCodeInstruction::kOrB, 1);
             falseLocation.set();
             this->write(ByteCodeInstruction::kMaskPop);
@@ -742,13 +741,13 @@ bool ByteCodeGenerator::writeBinaryExpression(const BinaryExpression& b, bool di
         case Token::Kind::TK_SHR: {
             SkASSERT(count == 1 && (tc == SkSL::TypeCategory::kSigned ||
                                     tc == SkSL::TypeCategory::kUnsigned));
-            if (!right.isCompileTimeConstant()) {
-                fErrors.error(right.fOffset, "Shift amounts must be constant");
+            if (!b.fRight->isCompileTimeConstant()) {
+                fErrors.error(b.fRight->fOffset, "Shift amounts must be constant");
                 return false;
             }
-            int64_t shift = right.getConstantInt();
+            int64_t shift = b.fRight->getConstantInt();
             if (shift < 0 || shift > 31) {
-                fErrors.error(right.fOffset, "Shift amount out of range");
+                fErrors.error(b.fRight->fOffset, "Shift amount out of range");
                 return false;
             }
 
@@ -766,7 +765,7 @@ bool ByteCodeGenerator::writeBinaryExpression(const BinaryExpression& b, bool di
         default:
             break;
     }
-    this->writeExpression(right);
+    this->writeExpression(*b.fRight);
     if (lVecOrMtx && !rVecOrMtx) {
         for (int i = SlotCount(lType); i > 1; --i) {
             this->write(ByteCodeInstruction::kDup, 1);

src/sksl/SkSLCFGGenerator.cpp

@@ -166,14 +166,14 @@ bool BasicBlock::tryRemoveExpression(std::vector<BasicBlock::Node>::iterator* it
     switch (expr->kind()) {
         case Expression::Kind::kBinary: {
             BinaryExpression& b = expr->as<BinaryExpression>();
-            if (b.getOperator() == Token::Kind::TK_EQ) {
-                if (!this->tryRemoveLValueBefore(iter, &b.left())) {
+            if (b.fOperator == Token::Kind::TK_EQ) {
+                if (!this->tryRemoveLValueBefore(iter, b.fLeft.get())) {
                     return false;
                 }
-            } else if (!this->tryRemoveExpressionBefore(iter, &b.left())) {
+            } else if (!this->tryRemoveExpressionBefore(iter, b.fLeft.get())) {
                 return false;
             }
-            if (!this->tryRemoveExpressionBefore(iter, &b.right())) {
+            if (!this->tryRemoveExpressionBefore(iter, b.fRight.get())) {
                 return false;
             }
             SkASSERT((*iter)->expression()->get() == expr);
@@ -267,12 +267,11 @@ bool BasicBlock::tryInsertExpression(std::vector<BasicBlock::Node>::iterator* it
     switch ((*expr)->kind()) {
         case Expression::Kind::kBinary: {
             BinaryExpression& b = expr->get()->as<BinaryExpression>();
-            if (!this->tryInsertExpression(iter, &b.rightPointer())) {
+            if (!this->tryInsertExpression(iter, &b.fRight)) {
                 return false;
             }
-
             ++(*iter);
-            if (!this->tryInsertExpression(iter, &b.leftPointer())) {
+            if (!this->tryInsertExpression(iter, &b.fLeft)) {
                 return false;
             }
             ++(*iter);
@@ -320,17 +319,16 @@ void CFGGenerator::addExpression(CFG& cfg, std::unique_ptr<Expression>* e, bool
     switch ((*e)->kind()) {
         case Expression::Kind::kBinary: {
             BinaryExpression& b = e->get()->as<BinaryExpression>();
-            Token::Kind op = b.getOperator();
-            switch (op) {
+            switch (b.fOperator) {
                 case Token::Kind::TK_LOGICALAND: // fall through
                 case Token::Kind::TK_LOGICALOR: {
                     // this isn't as precise as it could be -- we don't bother to track that if we
                     // early exit from a logical and/or, we know which branch of an 'if' we're going
                     // to hit -- but it won't make much difference in practice.
-                    this->addExpression(cfg, &b.leftPointer(), constantPropagate);
+                    this->addExpression(cfg, &b.fLeft, constantPropagate);
                     BlockId start = cfg.fCurrent;
                     cfg.newBlock();
-                    this->addExpression(cfg, &b.rightPointer(), constantPropagate);
+                    this->addExpression(cfg, &b.fRight, constantPropagate);
                     cfg.newBlock();
                     cfg.addExit(start, cfg.fCurrent);
                     cfg.fBlocks[cfg.fCurrent].fNodes.push_back({
@@ -342,8 +340,8 @@ void CFGGenerator::addExpression(CFG& cfg, std::unique_ptr<Expression>* e, bool
                     break;
                 }
                 case Token::Kind::TK_EQ: {
-                    this->addExpression(cfg, &b.rightPointer(), constantPropagate);
-                    this->addLValue(cfg, &b.leftPointer());
+                    this->addExpression(cfg, &b.fRight, constantPropagate);
+                    this->addLValue(cfg, &b.fLeft);
                     cfg.fBlocks[cfg.fCurrent].fNodes.push_back({
                         BasicBlock::Node::kExpression_Kind,
                         constantPropagate,
@@ -353,8 +351,8 @@ void CFGGenerator::addExpression(CFG& cfg, std::unique_ptr<Expression>* e, bool
                     break;
                 }
                 default:
-                    this->addExpression(cfg, &b.leftPointer(), !Compiler::IsAssignment(op));
-                    this->addExpression(cfg, &b.rightPointer(), constantPropagate);
+                    this->addExpression(cfg, &b.fLeft, !Compiler::IsAssignment(b.fOperator));
+                    this->addExpression(cfg, &b.fRight, constantPropagate);
                     cfg.fBlocks[cfg.fCurrent].fNodes.push_back({
                         BasicBlock::Node::kExpression_Kind,
                         constantPropagate,

src/sksl/SkSLCPPCodeGenerator.cpp

@@ -70,45 +70,42 @@ String CPPCodeGenerator::getTypeName(const Type& type) {
 
 void CPPCodeGenerator::writeBinaryExpression(const BinaryExpression& b,
                                              Precedence parentPrecedence) {
-    const Expression& left = b.left();
-    const Expression& right = b.right();
-    Token::Kind op = b.getOperator();
-    if (op == Token::Kind::TK_PERCENT) {
+    if (b.fOperator == Token::Kind::TK_PERCENT) {
         // need to use "%%" instead of "%" b/c the code will be inside of a printf
-        Precedence precedence = GetBinaryPrecedence(op);
+        Precedence precedence = GetBinaryPrecedence(b.fOperator);
         if (precedence >= parentPrecedence) {
             this->write("(");
         }
-        this->writeExpression(left, precedence);
+        this->writeExpression(*b.fLeft, precedence);
         this->write(" %% ");
-        this->writeExpression(right, precedence);
+        this->writeExpression(*b.fRight, precedence);
         if (precedence >= parentPrecedence) {
             this->write(")");
         }
-    } else if (left.kind() == Expression::Kind::kNullLiteral ||
-               right.kind() == Expression::Kind::kNullLiteral) {
+    } else if (b.fLeft->kind() == Expression::Kind::kNullLiteral ||
+               b.fRight->kind() == Expression::Kind::kNullLiteral) {
         const Variable* var;
-        if (left.kind() != Expression::Kind::kNullLiteral) {
-            var = &left.as<VariableReference>().fVariable;
+        if (b.fLeft->kind() != Expression::Kind::kNullLiteral) {
+            var = &b.fLeft->as<VariableReference>().fVariable;
         } else {
-            var = &right.as<VariableReference>().fVariable;
+            var = &b.fRight->as<VariableReference>().fVariable;
         }
         SkASSERT(var->type().typeKind() == Type::TypeKind::kNullable &&
                  var->type().componentType() == *fContext.fFragmentProcessor_Type);
         this->write("%s");
-        const char* prefix = "";
-        switch (op) {
+        const char* op = "";
+        switch (b.fOperator) {
             case Token::Kind::TK_EQEQ:
-                prefix = "!";
+                op = "!";
                 break;
             case Token::Kind::TK_NEQ:
-                prefix = "";
+                op = "";
                 break;
             default:
                 SkASSERT(false);
         }
         int childIndex = this->getChildFPIndex(*var);
-        fFormatArgs.push_back(String(prefix) + "_outer.childProcessor(" + to_string(childIndex) +
+        fFormatArgs.push_back(String(op) + "_outer.childProcessor(" + to_string(childIndex) +
                               ") ? \"true\" : \"false\"");
     } else {
         INHERITED::writeBinaryExpression(b, parentPrecedence);