sksl optimizer fixes
The main issue was a use-after-free due to removing (and thus destroying)
the binary expression prior to re-adding part of it. Also cleaned up the
way dead assignments are handled and added the test that originally
identified these problems.
Bug: skia:
Change-Id: Icda93d69a66c4e57850ecdc88fc4a4f634e1aac2
Reviewed-on: https://skia-review.googlesource.com/15383
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ben Wagner <benjaminwagner@google.com>
diff --git a/src/sksl/SkSLCompiler.cpp b/src/sksl/SkSLCompiler.cpp
index f417626..c6140d3 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -430,11 +430,26 @@
std::vector<BasicBlock::Node>::iterator* iter,
bool* outUpdated,
bool* outNeedsRescan) {
- ASSERT((*(*iter)->expression())->fKind == Expression::kBinary_Kind);
*outUpdated = true;
- if (!try_replace_expression(b, iter, &((BinaryExpression&) **(*iter)->expression()).fRight)) {
- *outNeedsRescan = true;
+ std::unique_ptr<Expression>* target = (*iter)->expression();
+ ASSERT((*target)->fKind == Expression::kBinary_Kind);
+ BinaryExpression& bin = (BinaryExpression&) **target;
+ bool result;
+ if (bin.fOperator == Token::EQ) {
+ result = !b->tryRemoveLValueBefore(iter, bin.fLeft.get());
+ } else {
+ result = !b->tryRemoveExpressionBefore(iter, bin.fLeft.get());
}
+ if (!result) {
+ *target = std::move(bin.fRight);
+ *outNeedsRescan = true;
+ return;
+ }
+ --(*iter);
+ ASSERT((*iter)->expression() == &bin.fRight);
+ *iter = b->fNodes.erase(*iter);
+ ASSERT((*iter)->expression() == target);
+ *target = std::move(bin.fRight);
}
/**
@@ -445,11 +460,20 @@
std::vector<BasicBlock::Node>::iterator* iter,
bool* outUpdated,
bool* outNeedsRescan) {
- ASSERT((*(*iter)->expression())->fKind == Expression::kBinary_Kind);
*outUpdated = true;
- if (!try_replace_expression(b, iter, &((BinaryExpression&) **(*iter)->expression()).fLeft)) {
+ std::unique_ptr<Expression>* target = (*iter)->expression();
+ ASSERT((*target)->fKind == Expression::kBinary_Kind);
+ BinaryExpression& bin = (BinaryExpression&) **target;
+ if (!b->tryRemoveExpressionBefore(iter, bin.fRight.get())) {
+ *target = std::move(bin.fLeft);
*outNeedsRescan = true;
+ return;
}
+ --(*iter);
+ ASSERT((*iter)->expression() == &bin.fLeft);
+ *iter = b->fNodes.erase(*iter);
+ ASSERT((*iter)->expression() == target);
+ *target = std::move(bin.fLeft);
}
/**
@@ -580,8 +604,12 @@
break;
}
case Expression::kBinary_Kind: {
- // collapse useless expressions like x * 1 or x + 0
BinaryExpression* bin = (BinaryExpression*) expr;
+ if (dead_assignment(*bin)) {
+ delete_left(&b, iter, outUpdated, outNeedsRescan);
+ break;
+ }
+ // collapse useless expressions like x * 1 or x + 0
if (((bin->fLeft->fType.kind() != Type::kScalar_Kind) &&
(bin->fLeft->fType.kind() != Type::kVector_Kind)) ||
((bin->fRight->fType.kind() != Type::kScalar_Kind) &&
@@ -795,28 +823,6 @@
case Statement::kExpression_Kind: {
ExpressionStatement& e = (ExpressionStatement&) *stmt;
ASSERT((*iter)->statement()->get() == &e);
- if (e.fExpression->fKind == Expression::kBinary_Kind) {
- BinaryExpression& bin = (BinaryExpression&) *e.fExpression;
- if (dead_assignment(bin)) {
- if (!b.tryRemoveExpressionBefore(iter, &bin)) {
- *outNeedsRescan = true;
- }
- if (bin.fRight->hasSideEffects()) {
- // still have to evaluate the right due to side effects,
- // replace the binary expression with just the right side
- e.fExpression = std::move(bin.fRight);
- if (!b.tryInsertExpression(iter, &e.fExpression)) {
- *outNeedsRescan = true;
- }
- } else {
- // no side effects, kill the whole statement
- ASSERT((*iter)->statement()->get() == stmt);
- (*iter)->setStatement(std::unique_ptr<Statement>(new Nop()));
- }
- *outUpdated = true;
- break;
- }
- }
if (!e.fExpression->hasSideEffects()) {
// Expression statement with no side effects, kill it
if (!b.tryRemoveExpressionBefore(iter, e.fExpression.get())) {