From 4b330dfd3334bf24bf93043acfcd31590a3cdbbf Mon Sep 17 00:00:00 2001 From: Ethan Nicholas Date: Wed, 17 May 2017 10:52:55 -0400 Subject: [PATCH] skslc comma operator and optimizer fixes Bug: skia: Change-Id: I732d4fba843c06af570d4a56cadfaa1cc565808c Reviewed-on: https://skia-review.googlesource.com/17125 Commit-Queue: Ethan Nicholas Reviewed-by: Ben Wagner --- src/sksl/SkSLCFGGenerator.cpp | 12 ++++++++---- src/sksl/SkSLCompiler.cpp | 35 +++++++++++++++++++++++++++-------- src/sksl/SkSLGLSLCodeGenerator.cpp | 1 + src/sksl/SkSLGLSLCodeGenerator.h | 2 +- src/sksl/SkSLIRGenerator.cpp | 7 ++++++- src/sksl/SkSLParser.cpp | 32 ++++++++++++++++++++++++-------- src/sksl/SkSLParser.h | 2 ++ src/sksl/SkSLToken.h | 1 + tests/SkSLGLSLTest.cpp | 26 ++++++++++++++++++++++++++ 9 files changed, 96 insertions(+), 22 deletions(-) diff --git a/src/sksl/SkSLCFGGenerator.cpp b/src/sksl/SkSLCFGGenerator.cpp index b756f2d..02e4e70 100644 --- a/src/sksl/SkSLCFGGenerator.cpp +++ b/src/sksl/SkSLCFGGenerator.cpp @@ -93,8 +93,7 @@ bool BasicBlock::tryRemoveExpressionBefore(std::vector::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::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* 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 a283e30..f63ef97 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 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 8099d45..d67f718 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 65be7dc..032b70e 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 e160a71..2e280d8 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 04e2517..5e8ec63 100644 --- a/src/sksl/SkSLParser.cpp +++ b/src/sksl/SkSLParser.cpp @@ -439,8 +439,8 @@ std::unique_ptr 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 Parser::varDeclarationEnd(Modifiers mods, std::unique_ptr type, String name) { @@ -462,7 +462,7 @@ std::unique_ptr Parser::varDeclarationEnd(Modifiers mods, } std::unique_ptr value; if (this->checkNext(Token::EQ)) { - value = this->expression(); + value = this->assignmentExpression(); if (!value) { return nullptr; } @@ -490,7 +490,7 @@ std::unique_ptr 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 Parser::expression() { if (!depth.checkValid()) { return nullptr; } - return this->assignmentExpression(); + return this->commaExpression(); +} + +/* assignmentExpression (COMMA assignmentExpression)* */ +std::unique_ptr Parser::commaExpression() { + std::unique_ptr result = this->assignmentExpression(); + if (!result) { + return nullptr; + } + Token t; + while (this->checkNext(Token::COMMA, &t)) { + std::unique_ptr 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 Parser::suffix() { std::vector> parameters; if (this->peek().fKind != Token::RPAREN) { for (;;) { - std::unique_ptr expr = this->expression(); + std::unique_ptr 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 6d4a0da..2f55b34 100644 --- a/src/sksl/SkSLParser.h +++ b/src/sksl/SkSLParser.h @@ -169,6 +169,8 @@ private: std::unique_ptr expression(); + std::unique_ptr commaExpression(); + std::unique_ptr assignmentExpression(); std::unique_ptr ternaryExpression(); diff --git a/src/sksl/SkSLToken.h b/src/sksl/SkSLToken.h index 07eb856..92193b9 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 ff1d5f5..ba6bbbd 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 -- 2.7.4