AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Converted SkSLSwitchCase to use ints rather than expressions

Browse Source 
There was no reason to carry around the heavier expressions, as we only
support int literals or constants, and this not only fixes a bug but
simplifies some of the code.

Bug: skia:12811
Change-Id: I3f55d6974d77ce1907f90868775c0e4894243783
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/494037
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This commit is contained in:
Ethan Nicholas 2022-01-12 15:14:28 -05:00 committed by SkCQ
parent 00f71133a8
commit 8af1eebedb
12 changed files with 97 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
src/sksl/SkSLAnalysis.cpp
View File

@ -731,9 +731,6 @@ template <typename T> bool TProgramVisitor<T>::visitStatement(typename T::Statem
case Statement::Kind::kSwitchCase: {
auto& sc = s.template as<SwitchCase>();
if (sc.value() && this->visitExpressionPtr(sc.value())) {
return true;
}
return this->visitStatementPtr(sc.statement());
}
case Statement::Kind::kDo: {

7
src/sksl/SkSLDehydrator.cpp
View File

@ -510,7 +510,12 @@ void Dehydrator::write(const Statement* s) {
this->writeU8(ss.cases().size());
for (const std::unique_ptr<Statement>& stmt : ss.cases()) {
const SwitchCase& sc = stmt->as<SwitchCase>();
this->write(sc.value().get());
if (sc.isDefault()) {
this->writeU8(1);
} else {
this->writeU8(0);
this->writeS32(sc.value());
}
this->write(sc.statement().get());
}
break;

7
src/sksl/SkSLInliner.cpp
View File

@ -566,8 +566,11 @@ std::unique_ptr<Statement> Inliner::inlineStatement(int line,
cases.reserve_back(ss.cases().size());
for (const std::unique_ptr<Statement>& switchCaseStmt : ss.cases()) {
const SwitchCase& sc = switchCaseStmt->as<SwitchCase>();
cases.push_back(std::make_unique<SwitchCase>(line, expr(sc.value()),
stmt(sc.statement())));
if (sc.isDefault()) {
cases.push_back(SwitchCase::MakeDefault(line, stmt(sc.statement())));
} else {
cases.push_back(SwitchCase::Make(line, sc.value(), stmt(sc.statement())));
}
}
return SwitchStatement::Make(*fContext, line, ss.isStatic(), expr(ss.value()),
std::move(cases), SymbolTable::WrapIfBuiltin(ss.symbols()));

14
src/sksl/SkSLRehydrator.cpp
View File

@ -364,10 +364,16 @@ std::unique_ptr<Statement> Rehydrator::statement() {
StatementArray cases;
cases.reserve_back(caseCount);
for (int i = 0; i < caseCount; ++i) {
std::unique_ptr<Expression> value = this->expression();
std::unique_ptr<Statement> statement = this->statement();
cases.push_back(std::make_unique<SwitchCase>(/*line=*/-1, std::move(value),
std::move(statement)));
bool isDefault = this->readU8();
if (isDefault) {
std::unique_ptr<Statement> statement = this->statement();
cases.push_back(SwitchCase::MakeDefault(/*line=*/-1, std::move(statement)));
} else {
SKSL_INT value = this->readS32();
std::unique_ptr<Statement> statement = this->statement();
cases.push_back(SwitchCase::Make(/*line=*/-1, std::move(value),
std::move(statement)));
}
}
return SwitchStatement::Make(fContext, /*line=*/-1, isStatic, std::move(expr),
std::move(cases), fSymbolTable);

2
src/sksl/analysis/SkSLCanExitWithoutReturningValue.cpp
View File

@ -97,7 +97,7 @@ public:
// The default case is indicated by a null value. A switch without a default
// case cannot definitively return, as its value might not be in the cases list.
const SwitchCase& sc = switchStmt->as<SwitchCase>();
if (!sc.value()) {
if (sc.isDefault()) {
foundDefault = true;
}
// Scan this switch-case for any exit (break, continue or return).

16
src/sksl/codegen/SkSLGLSLCodeGenerator.cpp
View File

@ -1408,7 +1408,7 @@ void GLSLCodeGenerator::writeSwitchStatement(const SwitchStatement& s) {
bool firstCase = true;
for (const std::unique_ptr<Statement>& stmt : s.cases()) {
const SwitchCase& c = stmt->as<SwitchCase>();
if (c.value()) {
if (!c.isDefault()) {
this->write("if ((");
if (firstCase) {
firstCase = false;
@ -1418,7 +1418,7 @@ void GLSLCodeGenerator::writeSwitchStatement(const SwitchStatement& s) {
}
this->write(valueVar);
this->write(" == ");
this->writeExpression(*c.value(), Precedence::kEquality);
this->write(to_string(c.value()));
this->writeLine(")) {");
fIndentation++;
@ -1455,17 +1455,17 @@ void GLSLCodeGenerator::writeSwitchStatement(const SwitchStatement& s) {
// If a switch contains only a `default` case and nothing else, this confuses some drivers and
// can lead to a crash. Adding a real case before the default seems to work around the bug,
// and doesn't change the meaning of the switch. (skia:12465)
if (s.cases().size() == 1 && !s.cases().front()->as<SwitchCase>().value()) {
if (s.cases().size() == 1 && s.cases().front()->as<SwitchCase>().isDefault()) {
this->writeLine("case 0:");
}
for (const std::unique_ptr<Statement>& stmt : s.cases()) {
const SwitchCase& c = stmt->as<SwitchCase>();
if (c.value()) {
this->write("case ");
this->writeExpression(*c.value(), Precedence::kTopLevel);
this->writeLine(":");
} else {
if (c.isDefault()) {
this->writeLine("default:");
} else {
this->write("case ");
this->write(to_string(c.value()));
this->writeLine(":");
}
if (!c.statement()->isEmpty()) {
fIndentation++;

10
src/sksl/codegen/SkSLMetalCodeGenerator.cpp
View File

@ -2201,12 +2201,12 @@ void MetalCodeGenerator::writeSwitchStatement(const SwitchStatement& s) {
fIndentation++;
for (const std::unique_ptr<Statement>& stmt : s.cases()) {
const SwitchCase& c = stmt->as<SwitchCase>();
if (c.value()) {
this->write("case ");
this->writeExpression(*c.value(), Precedence::kTopLevel);
this->writeLine(":");
} else {
if (c.isDefault()) {
this->writeLine("default:");
} else {
this->write("case ");
this->write(to_string(c.value()));
this->writeLine(":");
}
if (!c.statement()->isEmpty()) {
fIndentation++;

10
src/sksl/codegen/SkSLPipelineStageCodeGenerator.cpp
View File

@ -311,12 +311,12 @@ void PipelineStageCodeGenerator::writeSwitchStatement(const SwitchStatement& s)
this->writeLine(") {");
for (const std::unique_ptr<Statement>& stmt : s.cases()) {
const SwitchCase& c = stmt->as<SwitchCase>();
if (c.value()) {
this->write("case ");
this->writeExpression(*c.value(), Precedence::kTopLevel);
this->writeLine(":");
} else {
if (c.isDefault()) {
this->writeLine("default:");
} else {
this->write("case ");
this->write(to_string(c.value()));
this->writeLine(":");
}
if (!c.statement()->isEmpty()) {
this->writeStatement(*c.statement());

6
src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp
View File

@ -3317,7 +3317,7 @@ void SPIRVCodeGenerator::writeSwitchStatement(const SwitchStatement& s, OutputSt
const SwitchCase& c = stmt->as<SwitchCase>();
SpvId label = this->nextId(nullptr);
labels.push_back(label);
if (c.value()) {
if (!c.isDefault()) {
size += 2;
} else {
defaultLabel = label;
@ -3330,10 +3330,10 @@ void SPIRVCodeGenerator::writeSwitchStatement(const SwitchStatement& s, OutputSt
this->writeWord(defaultLabel, out);
for (size_t i = 0; i < cases.size(); ++i) {
const SwitchCase& c = cases[i]->as<SwitchCase>();
if (!c.value()) {
if (c.isDefault()) {
continue;
}
this->writeWord(c.value()->as<Literal>().intValue(), out);
this->writeWord(c.value(), out);
this->writeWord(labels[i], out);
}
for (size_t i = 0; i < cases.size(); ++i) {

4
src/sksl/codegen/SkSLVMCodeGenerator.cpp
View File

@ -1928,8 +1928,8 @@ void SkVMGenerator::writeSwitchStatement(const SwitchStatement& s) {
for (const std::unique_ptr<Statement>& stmt : s.cases()) {
const SwitchCase& c = stmt->as<SwitchCase>();
if (c.value()) {
Value caseValue = this->writeExpression(*c.value());
if (!c.isDefault()) {
Value caseValue = fBuilder->splat((int) c.value());
// We want to execute this switch case if we're falling through from a previous case, or
// if the case value matches.

50
src/sksl/ir/SkSLSwitchCase.h
View File

@ -11,6 +11,8 @@
#include "include/private/SkSLStatement.h"
#include "src/sksl/ir/SkSLExpression.h"
#include <inttypes.h>
namespace SkSL {
/**
@ -20,17 +22,23 @@ class SwitchCase final : public Statement {
public:
inline static constexpr Kind kStatementKind = Kind::kSwitchCase;
// null value implies "default" case
SwitchCase(int line, std::unique_ptr<Expression> value, std::unique_ptr<Statement> statement)
: INHERITED(line, kStatementKind)
, fValue(std::move(value))
, fStatement(std::move(statement)) {}
std::unique_ptr<Expression>& value() {
return fValue;
static std::unique_ptr<SwitchCase> Make(int line, SKSL_INT value,
std::unique_ptr<Statement> statement) {
return std::unique_ptr<SwitchCase>(new SwitchCase(line, /*isDefault=*/false, value,
std::move(statement)));
}
const std::unique_ptr<Expression>& value() const {
static std::unique_ptr<SwitchCase> MakeDefault(int line, std::unique_ptr<Statement> statement) {
return std::unique_ptr<SwitchCase>(new SwitchCase(line, /*isDefault=*/true, -1,
std::move(statement)));
}
bool isDefault() const {
return fDefault;
}
SKSL_INT value() const {
SkASSERT(!this->isDefault());
return fValue;
}
@ -43,23 +51,29 @@ public:
}
std::unique_ptr<Statement> clone() const override {
return std::make_unique<SwitchCase>(fLine,
this->value() ? this->value()->clone() : nullptr,
this->statement()->clone());
return fDefault ? SwitchCase::MakeDefault(fLine, this->statement()->clone())
: SwitchCase::Make(fLine, this->value(), this->statement()->clone());
}
String description() const override {
if (this->value()) {
return String::printf("case %s:\n%s",
this->value()->description().c_str(),
fStatement->description().c_str());
} else {
if (this->isDefault()) {
return String::printf("default:\n%s", fStatement->description().c_str());
} else {
return String::printf("case %" PRId64 ":\n%s",
(int64_t) this->value(),
fStatement->description().c_str());
}
}
private:
std::unique_ptr<Expression> fValue;
SwitchCase(int line, bool isDefault, SKSL_INT value, std::unique_ptr<Statement> statement)
: INHERITED(line, kStatementKind)
, fDefault(isDefault)
, fValue(std::move(value))
, fStatement(std::move(statement)) {}
bool fDefault;
SKSL_INT fValue;
std::unique_ptr<Statement> fStatement;
using INHERITED = Statement;

60
src/sksl/ir/SkSLSwitchStatement.cpp
View File

@ -55,27 +55,20 @@ static std::forward_list<const SwitchCase*> find_duplicate_case_values(
for (const std::unique_ptr<Statement>& stmt : cases) {
const SwitchCase* sc = &stmt->as<SwitchCase>();
const std::unique_ptr<Expression>& valueExpr = sc->value();
// A null case-value indicates the `default` switch-case.
if (!valueExpr) {
if (sc->isDefault()) {
if (foundDefault) {
duplicateCases.push_front(sc);
continue;
}
foundDefault = true;
continue;
} else {
SKSL_INT value = sc->value();
if (intValues.contains(value)) {
duplicateCases.push_front(sc);
continue;
}
intValues.add(value);
}
// GetConstantInt already succeeded when the SwitchCase was first assembled, so it should
// succeed this time too.
SKSL_INT intValue = 0;
SkAssertResult(ConstantFolder::GetConstantInt(*valueExpr, &intValue));
if (intValues.contains(intValue)) {
duplicateCases.push_front(sc);
continue;
}
intValues.add(intValue);
}
return duplicateCases;
@ -180,28 +173,23 @@ std::unique_ptr<Statement> SwitchStatement::Convert(const Context& context,
StatementArray cases;
for (int i = 0; i < caseValues.count(); ++i) {
int caseLine;
std::unique_ptr<Expression> caseValue;
if (caseValues[i]) {
caseLine = caseValues[i]->fLine;
// Case values must be the same type as the switch value--`int` or a particular enum.
caseValue = value->type().coerceExpression(std::move(caseValues[i]), context);
int caseLine = caseValues[i]->fLine;
// Case values must be constant integers of the same type as the switch value
std::unique_ptr<Expression> caseValue = value->type().coerceExpression(
std::move(caseValues[i]), context);
if (!caseValue) {
return nullptr;
}
// Case values must be a literal integer or a `const int` variable reference.
SKSL_INT intValue;
if (!ConstantFolder::GetConstantInt(*caseValue, &intValue)) {
context.fErrors->error(caseValue->fLine, "case value must be a constant integer");
return nullptr;
}
cases.push_back(SwitchCase::Make(caseLine, intValue, std::move(caseStatements[i])));
} else {
// The null case-expression corresponds to `default:`.
caseLine = line;
cases.push_back(SwitchCase::MakeDefault(line, std::move(caseStatements[i])));
}
cases.push_back(std::make_unique<SwitchCase>(caseLine, std::move(caseValue),
std::move(caseStatements[i])));
}
// Detect duplicate `case` labels and report an error.
@ -210,11 +198,11 @@ std::unique_ptr<Statement> SwitchStatement::Convert(const Context& context,
if (!duplicateCases.empty()) {
duplicateCases.reverse();
for (const SwitchCase* sc : duplicateCases) {
if (sc->value() != nullptr) {
context.fErrors->error(sc->fLine,
"duplicate case value '" + sc->value()->description() + "'");
} else {
if (sc->isDefault()) {
context.fErrors->error(sc->fLine, "duplicate default case");
} else {
context.fErrors->error(sc->fLine,
"duplicate case value '" + to_string(sc->value()) + "'");
}
}
return nullptr;
@ -235,12 +223,6 @@ std::unique_ptr<Statement> SwitchStatement::Make(const Context& context,
return stmt->is<SwitchCase>();
}));
// Confirm that every switch-case has been coerced to the proper type.
SkASSERT(std::all_of(cases.begin(), cases.end(), [&](const std::unique_ptr<Statement>& stmt) {
return !stmt->as<SwitchCase>().value() || // `default` case has a null value
value->type().matches(stmt->as<SwitchCase>().value()->type());
}));
// Confirm that every switch-case value is unique.
SkASSERT(find_duplicate_case_values(cases).empty());
@ -252,14 +234,12 @@ std::unique_ptr<Statement> SwitchStatement::Make(const Context& context,
SwitchCase* matchingCase = nullptr;
for (const std::unique_ptr<Statement>& stmt : cases) {
SwitchCase& sc = stmt->as<SwitchCase>();
if (!sc.value()) {
if (sc.isDefault()) {
defaultCase = &sc;
continue;
}
SKSL_INT caseValue;
SkAssertResult(ConstantFolder::GetConstantInt(*sc.value(), &caseValue));
if (caseValue == switchValue) {
if (sc.value() == switchValue) {
matchingCase = &sc;
break;
}