skslc comma operator and optimizer fixes

Bug: skia:
Change-Id: I732d4fba843c06af570d4a56cadfaa1cc565808c
Reviewed-on: https://skia-review.googlesource.com/17125
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ben Wagner <benjaminwagner@google.com>

9 files changed, 96 insertions, 22 deletions

diff --git a/src/sksl/SkSLCFGGenerator.cpp b/src/sksl/SkSLCFGGenerator.cpp
index b756f2d762..02e4e702a7 100644
--- a/src/sksl/SkSLCFGGenerator.cpp
+++ b/src/sksl/SkSLCFGGenerator.cpp
@@ -93,8 +93,7 @@ bool BasicBlock::tryRemoveExpressionBefore(std::vector<BasicBlock::Node>::iterat
         Expression* old = (*iter)->expression()->get();
         do {
             if ((*iter) == fNodes.begin()) {
-                ABORT("couldn't find %s before %s\n", e->description().c_str(),
-                                                      old->description().c_str());
+                return false;
             }
             --(*iter);
         } while ((*iter)->fKind != BasicBlock::Node::kExpression_Kind ||
@@ -109,8 +108,7 @@ bool BasicBlock::tryRemoveExpressionBefore(std::vector<BasicBlock::Node>::iterat
         Statement* old = (*iter)->statement()->get();
         do {
             if ((*iter) == fNodes.begin()) {
-                ABORT("couldn't find %s before %s\n", e->description().c_str(),
-                                                      old->description().c_str());
+                return false;
             }
             --(*iter);
         } while ((*iter)->fKind != BasicBlock::Node::kExpression_Kind ||
@@ -300,6 +298,12 @@ void CFGGenerator::addExpression(CFG& cfg, std::unique_ptr<Expression>* e, bool
                     this->addExpression(cfg, &b->fRight, constantPropagate);
                     cfg.newBlock();
                     cfg.addExit(start, cfg.fCurrent);
+                    cfg.fBlocks[cfg.fCurrent].fNodes.push_back({
+                        BasicBlock::Node::kExpression_Kind,
+                        constantPropagate,
+                        e,
+                        nullptr
+                    });
                     break;
                 }
                 case Token::EQ: {
diff --git a/src/sksl/SkSLCompiler.cpp b/src/sksl/SkSLCompiler.cpp
index a283e3051d..f63ef977be 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -436,20 +436,27 @@ void delete_left(BasicBlock* b,
     BinaryExpression& bin = (BinaryExpression&) **target;
     bool result;
     if (bin.fOperator == Token::EQ) {
-        result = !b->tryRemoveLValueBefore(iter, bin.fLeft.get());
+        result = b->tryRemoveLValueBefore(iter, bin.fLeft.get());
     } else {
-        result = !b->tryRemoveExpressionBefore(iter, bin.fLeft.get());
+        result = b->tryRemoveExpressionBefore(iter, bin.fLeft.get());
     }
+    *target = std::move(bin.fRight);
     if (!result) {
-        *target = std::move(bin.fRight);
+        *outNeedsRescan = true;
+        return;
+    }
+    if (*iter == b->fNodes.begin()) {
         *outNeedsRescan = true;
         return;
     }
     --(*iter);
-    ASSERT((*iter)->expression() == &bin.fRight);
+    if ((*iter)->fKind != BasicBlock::Node::kExpression_Kind ||
+        (*iter)->expression() != &bin.fRight) {
+        *outNeedsRescan = true;
+        return;
+    }
     *iter = b->fNodes.erase(*iter);
     ASSERT((*iter)->expression() == target);
-    *target = std::move(bin.fRight);
 }
 
 /**
@@ -469,11 +476,19 @@ void delete_right(BasicBlock* b,
         *outNeedsRescan = true;
         return;
     }
+    *target = std::move(bin.fLeft);
+    if (*iter == b->fNodes.begin()) {
+        *outNeedsRescan = true;
+        return;
+    }
     --(*iter);
-    ASSERT((*iter)->expression() == &bin.fLeft);
+    if (((*iter)->fKind != BasicBlock::Node::kExpression_Kind ||
+        (*iter)->expression() != &bin.fLeft)) {
+        *outNeedsRescan = true;
+        return;
+    }
     *iter = b->fNodes.erase(*iter);
     ASSERT((*iter)->expression() == target);
-    *target = std::move(bin.fLeft);
 }
 
 /**
@@ -569,12 +584,13 @@ void Compiler::simplifyExpression(DefinitionMap& definitions,
     if ((*iter)->fConstantPropagation) {
         std::unique_ptr<Expression> optimized = expr->constantPropagate(*fIRGenerator, definitions);
         if (optimized) {
+            *outUpdated = true;
             if (!try_replace_expression(&b, iter, &optimized)) {
                 *outNeedsRescan = true;
+                return;
             }
             ASSERT((*iter)->fKind == BasicBlock::Node::kExpression_Kind);
             expr = (*iter)->expression()->get();
-            *outUpdated = true;
         }
     }
     switch (expr->fKind) {
@@ -1011,6 +1027,9 @@ void Compiler::scanCFG(FunctionDefinition& f) {
                     this->simplifyStatement(definitions, b, &iter, &undefinedVariables, &updated,
                                              &needsRescan);
                 }
+                if (needsRescan) {
+                    break;
+                }
                 this->addDefinitions(*iter, &definitions);
             }
         }
diff --git a/src/sksl/SkSLGLSLCodeGenerator.cpp b/src/sksl/SkSLGLSLCodeGenerator.cpp
index 8099d45475..d67f7182f8 100644
--- a/src/sksl/SkSLGLSLCodeGenerator.cpp
+++ b/src/sksl/SkSLGLSLCodeGenerator.cpp
@@ -406,6 +406,7 @@ static GLSLCodeGenerator::Precedence get_binary_precedence(Token::Kind op) {
         case Token::BITWISEANDEQ: // fall through
         case Token::BITWISEXOREQ: // fall through
         case Token::BITWISEOREQ:  return GLSLCodeGenerator::kAssignment_Precedence;
+        case Token::COMMA:        return GLSLCodeGenerator::kSequence_Precedence;
         default: ABORT("unsupported binary operator");
     }
 }
diff --git a/src/sksl/SkSLGLSLCodeGenerator.h b/src/sksl/SkSLGLSLCodeGenerator.h
index 65be7dc09e..032b70eea1 100644
--- a/src/sksl/SkSLGLSLCodeGenerator.h
+++ b/src/sksl/SkSLGLSLCodeGenerator.h
@@ -68,7 +68,7 @@ public:
         kTernary_Precedence        = 15,
         kAssignment_Precedence     = 16,
         kSequence_Precedence       = 17,
-        kTopLevel_Precedence       = 18
+        kTopLevel_Precedence       = kSequence_Precedence
     };
 
     GLSLCodeGenerator(const Context* context, const Program* program, ErrorReporter* errors,
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index e160a71474..2e280d8702 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -917,10 +917,15 @@ static bool determine_binary_type(const Context& context,
         case Token::MINUS:   // fall through
         case Token::MINUSEQ: // fall through
         case Token::SLASH:   // fall through
-        case Token::SLASHEQ:
+        case Token::SLASHEQ: // fall through
             isLogical = false;
             validMatrixOrVectorOp = true;
             break;
+        case Token::COMMA:
+            *outLeftType = &left;
+            *outRightType = &right;
+            *outResultType = &right;
+            return true;
         default:
             isLogical = false;
             validMatrixOrVectorOp = false;
diff --git a/src/sksl/SkSLParser.cpp b/src/sksl/SkSLParser.cpp
index 04e25175cb..5e8ec6398b 100644
--- a/src/sksl/SkSLParser.cpp
+++ b/src/sksl/SkSLParser.cpp
@@ -439,8 +439,8 @@ std::unique_ptr<ASTVarDeclarations> Parser::structVarDeclaration(Modifiers modif
     return nullptr;
 }
 
-/* (LBRACKET expression? RBRACKET)* (EQ expression)? (COMMA IDENTIFER
-   (LBRACKET expression? RBRACKET)* (EQ expression)?)* SEMICOLON */
+/* (LBRACKET expression? RBRACKET)* (EQ assignmentExpression)? (COMMA IDENTIFER
+   (LBRACKET expression? RBRACKET)* (EQ assignmentExpression)?)* SEMICOLON */
 std::unique_ptr<ASTVarDeclarations> Parser::varDeclarationEnd(Modifiers mods,
                                                               std::unique_ptr<ASTType> type,
                                                               String name) {
@@ -462,7 +462,7 @@ std::unique_ptr<ASTVarDeclarations> Parser::varDeclarationEnd(Modifiers mods,
     }
     std::unique_ptr<ASTExpression> value;
     if (this->checkNext(Token::EQ)) {
-        value = this->expression();
+        value = this->assignmentExpression();
         if (!value) {
             return nullptr;
         }
@@ -490,7 +490,7 @@ std::unique_ptr<ASTVarDeclarations> Parser::varDeclarationEnd(Modifiers mods,
             }
         }
         if (this->checkNext(Token::EQ)) {
-            value = this->expression();
+            value = this->assignmentExpression();
             if (!value) {
                 return nullptr;
             }
@@ -1222,7 +1222,24 @@ std::unique_ptr<ASTExpression> Parser::expression() {
     if (!depth.checkValid()) {
         return nullptr;
     }
-    return this->assignmentExpression();
+    return this->commaExpression();
+}
+
+/* assignmentExpression (COMMA assignmentExpression)* */
+std::unique_ptr<ASTExpression> Parser::commaExpression() {
+    std::unique_ptr<ASTExpression> result = this->assignmentExpression();
+    if (!result) {
+        return nullptr;
+    }
+    Token t;
+    while (this->checkNext(Token::COMMA, &t)) {
+        std::unique_ptr<ASTExpression> right = this->commaExpression();
+        if (!right) {
+            return nullptr;
+        }
+        result.reset(new ASTBinaryExpression(std::move(result), t, std::move(right)));
+    }
+    return result;
 }
 
 /* ternaryExpression ((EQEQ | STAREQ | SLASHEQ | PERCENTEQ | PLUSEQ | MINUSEQ | SHLEQ | SHREQ |
@@ -1587,15 +1604,14 @@ std::unique_ptr<ASTSuffix> Parser::suffix() {
             std::vector<std::unique_ptr<ASTExpression>> parameters;
             if (this->peek().fKind != Token::RPAREN) {
                 for (;;) {
-                    std::unique_ptr<ASTExpression> expr = this->expression();
+                    std::unique_ptr<ASTExpression> expr = this->assignmentExpression();
                     if (!expr) {
                         return nullptr;
                     }
                     parameters.push_back(std::move(expr));
-                    if (this->peek().fKind != Token::COMMA) {
+                    if (!this->checkNext(Token::COMMA)) {
                         break;
                     }
-                    this->nextToken();
                 }
             }
             this->expect(Token::RPAREN, "')' to complete function parameters");
diff --git a/src/sksl/SkSLParser.h b/src/sksl/SkSLParser.h
index 6d4a0da315..2f55b34986 100644
--- a/src/sksl/SkSLParser.h
+++ b/src/sksl/SkSLParser.h
@@ -169,6 +169,8 @@ private:
 
     std::unique_ptr<ASTExpression> expression();
 
+    std::unique_ptr<ASTExpression> commaExpression();
+
     std::unique_ptr<ASTExpression> assignmentExpression();
 
     std::unique_ptr<ASTExpression> ternaryExpression();
diff --git a/src/sksl/SkSLToken.h b/src/sksl/SkSLToken.h
index 07eb8567a1..92193b9674 100644
--- a/src/sksl/SkSLToken.h
+++ b/src/sksl/SkSLToken.h
@@ -174,6 +174,7 @@ struct Token {
             case Token::BITWISEXOREQ: return String("^=");
             case Token::PLUSPLUS:     return String("++");
             case Token::MINUSMINUS:   return String("--");
+            case Token::COMMA:        return String(",");
             default:
                 ABORT("unsupported operator: %d\n", kind);
         }
diff --git a/tests/SkSLGLSLTest.cpp b/tests/SkSLGLSLTest.cpp
index ff1d5f5972..ba6bbbd13f 100644
--- a/tests/SkSLGLSLTest.cpp
+++ b/tests/SkSLGLSLTest.cpp
@@ -141,6 +141,8 @@ DEF_TEST(SkSLOperators, r) {
          "z >>= 2;"
          "z <<= 4;"
          "z %= 5;"
+         "x = (vec2(sqrt(1)) , 6);"
+         "z = (vec2(sqrt(1)) , 6);"
          "}",
          *SkSL::ShaderCapsFactory::Default(),
          "#version 400\n"
@@ -164,6 +166,8 @@ DEF_TEST(SkSLOperators, r) {
          "    z >>= 2;\n"
          "    z <<= 4;\n"
          "    z %= 5;\n"
+         "    x = float((vec2(sqrt(1.0)) , 6));\n"
+         "    z = (vec2(sqrt(1.0)) , 6);\n"
          "}\n");
 }
 
@@ -1334,4 +1338,26 @@ DEF_TEST(SkSLMultipleAssignments, r) {
          "}\n");
 }
 
+DEF_TEST(SkSLComplexDelete, r) {
+    test(r,
+        "uniform mat4 colorXform;"
+        "uniform sampler2D sampler;"
+        "void main() {"
+        "vec4 tmpColor;"
+        "sk_FragColor = vec4(1.0) * (tmpColor = texture(sampler, vec2(1)) , "
+        "colorXform != mat4(1.0) ? vec4(clamp((mat4(colorXform) * vec4(tmpColor.xyz, 1.0)).xyz, "
+        "0.0, tmpColor.w), tmpColor.w) : tmpColor);"
+        "}",
+        *SkSL::ShaderCapsFactory::Default(),
+        "#version 400\n"
+        "out vec4 sk_FragColor;\n"
+        "uniform mat4 colorXform;\n"
+        "uniform sampler2D sampler;\n"
+        "void main() {\n"
+        "    vec4 tmpColor;\n"
+        "    sk_FragColor = (tmpColor = texture(sampler, vec2(1.0)) , colorXform != mat4(1.0) ? "
+        "vec4(clamp((colorXform * vec4(tmpColor.xyz, 1.0)).xyz, 0.0, tmpColor.w), tmpColor.w) : "
+        "tmpColor);\n"
+        "}\n");
+}
 #endif