From e1d1b08203f87ffc708c9d2a6e03bd7e21716c91 Mon Sep 17 00:00:00 2001 From: John Stiles Date: Tue, 23 Feb 2021 13:44:36 -0500 Subject: [PATCH] Migrate convertSwitch to SwitchStatement::Make. This splits switch() construction into two stages. - One version of Make takes an array of case-values and case-statement lists, and is responsible for reporting errors if the case-values are not unique or are improperly typed. This is what the IR generator or DSL will start with on its first encounter with the switch statement. - The other version of Make takes an array of already-processed SwitchCases and can assume the invariant that they're all correctly- typed with unique values. This is what we will have when a statement is inlined or otherwise cloned. (We still assert this invariant, for correctness' sake, but in release mode we assume it.) This CL doesn't perform any optimizations at Make time yet; it does work equivalent to how `switch` works in the IR generator today. It does improve duplicate case-label checking slightly; duplicate case labels are now reported, and duplicate `default:` labels are detected. (Multiple `default` labels won't pass the parser, but they can be constructed in DSL.) Change-Id: I537ce2c8236152d58641fb1793619d66a62c01a8 Bug: skia:11342 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372616 Reviewed-by: Brian Osman Commit-Queue: John Stiles Auto-Submit: John Stiles --- gn/sksl.gni | 1 + src/sksl/SkSLIRGenerator.cpp | 54 +------ src/sksl/SkSLIRGenerator.h | 6 - src/sksl/SkSLInliner.cpp | 5 +- src/sksl/dsl/priv/DSLWriter.cpp | 7 +- src/sksl/ir/SkSLSwitchStatement.cpp | 165 +++++++++++++++++++++ src/sksl/ir/SkSLSwitchStatement.h | 50 ++++--- tests/SkSLDSLTest.cpp | 19 ++- tests/sksl/errors/SwitchDuplicateCase.glsl | 2 +- 9 files changed, 216 insertions(+), 93 deletions(-) create mode 100644 src/sksl/ir/SkSLSwitchStatement.cpp diff --git a/gn/sksl.gni b/gn/sksl.gni index 63ab0df1b7..d0c23df618 100644 --- a/gn/sksl.gni +++ b/gn/sksl.gni @@ -128,6 +128,7 @@ skia_sksl_sources = [ "$_src/sksl/ir/SkSLStatement.h", "$_src/sksl/ir/SkSLStructDefinition.h", "$_src/sksl/ir/SkSLSwitchCase.h", + "$_src/sksl/ir/SkSLSwitchStatement.cpp", "$_src/sksl/ir/SkSLSwitchStatement.h", "$_src/sksl/ir/SkSLSwizzle.cpp", "$_src/sksl/ir/SkSLSwizzle.h", diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp index ee6e1c2f7d..9e88f93b68 100644 --- a/src/sksl/SkSLIRGenerator.cpp +++ b/src/sksl/SkSLIRGenerator.cpp @@ -771,56 +771,6 @@ std::unique_ptr IRGenerator::convertDo(const ASTNode& d) { return this->convertDo(std::move(statement), std::move(test)); } -std::unique_ptr IRGenerator::convertSwitch( - int offset, - bool isStatic, - std::unique_ptr value, - ExpressionArray caseValues, - SkTArray caseStatements, - std::shared_ptr symbolTable) { - SkASSERT(caseValues.size() == caseStatements.size()); - if (this->strictES2Mode()) { - this->errorReporter().error(offset, "switch statements are not supported"); - return nullptr; - } - - if (!value->type().isEnum()) { - value = this->coerce(std::move(value), *fContext.fTypes.fInt); - if (!value) { - return nullptr; - } - } - SkTHashSet intValues; - std::vector> cases; - for (size_t i = 0; i < caseValues.size(); ++i) { - int caseOffset; - std::unique_ptr caseValue; - if (caseValues[i]) { - caseOffset = caseValues[i]->fOffset; - caseValue = this->coerce(std::move(caseValues[i]), value->type()); - if (!caseValue) { - return nullptr; - } - SKSL_INT v = 0; - if (!ConstantFolder::GetConstantInt(*caseValue, &v)) { - this->errorReporter().error(caseValue->fOffset, - "case value must be a constant integer"); - return nullptr; - } - if (intValues.contains(v)) { - this->errorReporter().error(caseValue->fOffset, "duplicate case value"); - } - intValues.add(v); - } else { - caseOffset = offset; - } - cases.push_back(std::make_unique(caseOffset, std::move(caseValue), - std::move(caseStatements[i]))); - } - return std::make_unique(offset, isStatic, std::move(value), - std::move(cases), symbolTable); -} - std::unique_ptr IRGenerator::convertSwitch(const ASTNode& s) { SkASSERT(s.fKind == ASTNode::Kind::kSwitch); @@ -854,8 +804,8 @@ std::unique_ptr IRGenerator::convertSwitch(const ASTNode& s) { } caseStatements.push_back(std::move(statements)); } - return this->convertSwitch(s.fOffset, s.getBool(), std::move(value), std::move(caseValues), - std::move(caseStatements), fSymbolTable); + return SwitchStatement::Make(fContext, s.fOffset, s.getBool(), std::move(value), + std::move(caseValues), std::move(caseStatements), fSymbolTable); } std::unique_ptr IRGenerator::convertExpressionStatement(const ASTNode& s) { diff --git a/src/sksl/SkSLIRGenerator.h b/src/sksl/SkSLIRGenerator.h index bf5f2fcf5e..983bd147e3 100644 --- a/src/sksl/SkSLIRGenerator.h +++ b/src/sksl/SkSLIRGenerator.h @@ -202,12 +202,6 @@ private: std::unique_ptr convertDo(std::unique_ptr stmt, std::unique_ptr test); std::unique_ptr convertDo(const ASTNode& d); - std::unique_ptr convertSwitch(int offset, - bool isStatic, - std::unique_ptr value, - ExpressionArray caseValues, - SkTArray caseStatements, - std::shared_ptr symbolTable); std::unique_ptr convertSwitch(const ASTNode& s); std::unique_ptr convertBinaryExpression(const ASTNode& expression); std::unique_ptr convertExtension(int offset, StringFragment name); diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp index 464855d9e3..c9d55cb7ac 100644 --- a/src/sksl/SkSLInliner.cpp +++ b/src/sksl/SkSLInliner.cpp @@ -530,9 +530,8 @@ std::unique_ptr Inliner::inlineStatement(int offset, cases.push_back(std::make_unique(offset, expr(sc->value()), stmts(sc->statements()))); } - return std::make_unique(offset, ss.isStatic(), expr(ss.value()), - std::move(cases), - SymbolTable::WrapIfBuiltin(ss.symbols())); + return SwitchStatement::Make(*fContext, offset, ss.isStatic(), expr(ss.value()), + std::move(cases), SymbolTable::WrapIfBuiltin(ss.symbols())); } case Statement::Kind::kVarDeclaration: { const VarDeclaration& decl = statement.as(); diff --git a/src/sksl/dsl/priv/DSLWriter.cpp b/src/sksl/dsl/priv/DSLWriter.cpp index 9a6edc3c5d..2495e19501 100644 --- a/src/sksl/dsl/priv/DSLWriter.cpp +++ b/src/sksl/dsl/priv/DSLWriter.cpp @@ -15,6 +15,7 @@ #include "src/sksl/SkSLIRGenerator.h" #include "src/sksl/dsl/DSLCore.h" #include "src/sksl/ir/SkSLConstructor.h" +#include "src/sksl/ir/SkSLSwitchStatement.h" #if !SKSL_USE_THREAD_LOCAL #include @@ -135,9 +136,9 @@ DSLExpression DSLWriter::ConvertPrefix(Operator op, std::unique_ptr DSLStatement DSLWriter::ConvertSwitch(std::unique_ptr value, ExpressionArray caseValues, SkTArray caseStatements) { - return IRGenerator().convertSwitch(/*offset=*/-1, /*isStatic=*/false, std::move(value), - std::move(caseValues), std::move(caseStatements), - IRGenerator().fSymbolTable); + return SwitchStatement::Make(Context(), /*offset=*/-1, /*isStatic=*/false, std::move(value), + std::move(caseValues), std::move(caseStatements), + IRGenerator().fSymbolTable); } diff --git a/src/sksl/ir/SkSLSwitchStatement.cpp b/src/sksl/ir/SkSLSwitchStatement.cpp new file mode 100644 index 0000000000..20499283df --- /dev/null +++ b/src/sksl/ir/SkSLSwitchStatement.cpp @@ -0,0 +1,165 @@ +/* + * Copyright 2021 Google LLC + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#include "src/sksl/ir/SkSLSwitchStatement.h" + +#include + +#include "include/private/SkTHash.h" +#include "src/sksl/SkSLConstantFolder.h" +#include "src/sksl/SkSLContext.h" +#include "src/sksl/SkSLProgramSettings.h" +#include "src/sksl/ir/SkSLSymbolTable.h" +#include "src/sksl/ir/SkSLType.h" + +namespace SkSL { + +std::unique_ptr SwitchStatement::clone() const { + std::vector> cloned; + for (const std::unique_ptr& sc : this->cases()) { + cloned.emplace_back(&sc->clone().release()->as()); + } + return std::make_unique(fOffset, + this->isStatic(), + this->value()->clone(), + std::move(cloned), + SymbolTable::WrapIfBuiltin(this->symbols())); +} + +String SwitchStatement::description() const { + String result; + if (this->isStatic()) { + result += "@"; + } + result += String::printf("switch (%s) {\n", this->value()->description().c_str()); + for (const auto& c : this->cases()) { + result += c->description(); + } + result += "}"; + return result; +} + +static std::forward_list find_duplicate_case_values( + const std::vector>& cases) { + std::forward_list duplicateCases; + SkTHashSet intValues; + bool foundDefault = false; + + for (const std::unique_ptr& sc : cases) { + const std::unique_ptr& valueExpr = sc->value(); + + // A null case-value indicates the `default` switch-case. + if (!valueExpr) { + if (foundDefault) { + duplicateCases.push_front(sc.get()); + continue; + } + foundDefault = true; + continue; + } + + // 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.get()); + continue; + } + intValues.add(intValue); + } + + return duplicateCases; +} + +std::unique_ptr SwitchStatement::Make(const Context& context, + int offset, + bool isStatic, + std::unique_ptr value, + ExpressionArray caseValues, + SkTArray caseStatements, + std::shared_ptr symbolTable) { + SkASSERT(caseValues.size() == caseStatements.size()); + if (context.fConfig->strictES2Mode()) { + context.fErrors.error(offset, "switch statements are not supported"); + return nullptr; + } + + if (!value->type().isEnum()) { + value = context.fTypes.fInt->coerceExpression(std::move(value), context); + if (!value) { + return nullptr; + } + } + + std::vector> cases; + for (int i = 0; i < caseValues.count(); ++i) { + int caseOffset; + std::unique_ptr caseValue; + if (caseValues[i]) { + caseOffset = caseValues[i]->fOffset; + + // 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); + 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->fOffset, "case value must be a constant integer"); + return nullptr; + } + } else { + // The null case-expression corresponds to `default:`. + caseOffset = offset; + } + cases.push_back(std::make_unique(caseOffset, std::move(caseValue), + std::move(caseStatements[i]))); + } + + // Detect duplicate `case` labels and report an error. + // (Using forward_list here to optimize for the common case of no results.) + std::forward_list duplicateCases = find_duplicate_case_values(cases); + if (!duplicateCases.empty()) { + duplicateCases.reverse(); + for (const SwitchCase* sc : duplicateCases) { + if (sc->value() != nullptr) { + context.fErrors.error(sc->fOffset, + "duplicate case value '" + sc->value()->description() + "'"); + } else { + context.fErrors.error(sc->fOffset, "duplicate default case"); + } + } + return nullptr; + } + + return SwitchStatement::Make(context, offset, isStatic, std::move(value), std::move(cases), + std::move(symbolTable)); +} + +std::unique_ptr SwitchStatement::Make(const Context& context, + int offset, + bool isStatic, + std::unique_ptr value, + std::vector> cases, + std::shared_ptr symbolTable) { + // Confirm that every switch-case has been coerced to the proper type. + SkASSERT(std::all_of(cases.begin(), cases.end(), [&](const std::unique_ptr& sc) { + return !sc->value() || // `default` case has a null value + value->type() == sc->value()->type(); + })); + + // Confirm that every switch-case value is unique. + SkASSERT(find_duplicate_case_values(cases).empty()); + + // TODO(skia:11340): Optimize static switches. + return std::make_unique(offset, isStatic, std::move(value), std::move(cases), + std::move(symbolTable)); +} + +} // namespace SkSL diff --git a/src/sksl/ir/SkSLSwitchStatement.h b/src/sksl/ir/SkSLSwitchStatement.h index 507c5c1160..e4714d75fc 100644 --- a/src/sksl/ir/SkSLSwitchStatement.h +++ b/src/sksl/ir/SkSLSwitchStatement.h @@ -11,8 +11,12 @@ #include "src/sksl/ir/SkSLStatement.h" #include "src/sksl/ir/SkSLSwitchCase.h" +#include +#include + namespace SkSL { +class Expression; class SymbolTable; /** @@ -22,6 +26,7 @@ class SwitchStatement final : public Statement { public: static constexpr Kind kStatementKind = Kind::kSwitch; + // Use SwitchStatement::Make to check invariants when creating switch statements. SwitchStatement(int offset, bool isStatic, std::unique_ptr value, std::vector> cases, const std::shared_ptr symbols) @@ -31,6 +36,25 @@ public: , fCases(std::move(cases)) , fSymbols(std::move(symbols)) {} + // Create a `switch` statement with an array of case-values and case-statements. + // Coerces case values to the proper type and reports an error if cases are duplicated. + static std::unique_ptr Make(const Context& context, + int offset, + bool isStatic, + std::unique_ptr value, + ExpressionArray caseValues, + SkTArray caseStatements, + std::shared_ptr symbolTable); + + // Create a `switch` statement with an array of SwitchCases. + // The array of SwitchCases must already contain non-overlapping, correctly-typed case values. + static std::unique_ptr Make(const Context& context, + int offset, + bool isStatic, + std::unique_ptr value, + std::vector> cases, + std::shared_ptr symbolTable); + std::unique_ptr& value() { return fValue; } @@ -55,31 +79,9 @@ public: return fSymbols; } - std::unique_ptr clone() const override { - std::vector> cloned; - for (const std::unique_ptr& sc : this->cases()) { - cloned.emplace_back(&sc->clone().release()->as()); - } - return std::unique_ptr(new SwitchStatement( - fOffset, - this->isStatic(), - this->value()->clone(), - std::move(cloned), - SymbolTable::WrapIfBuiltin(this->symbols()))); - } + std::unique_ptr clone() const override; - String description() const override { - String result; - if (this->isStatic()) { - result += "@"; - } - result += String::printf("switch (%s) {\n", this->value()->description().c_str()); - for (const auto& c : this->cases()) { - result += c->description(); - } - result += "}"; - return result; - } + String description() const override; private: bool fIsStatic; diff --git a/tests/SkSLDSLTest.cpp b/tests/SkSLDSLTest.cpp index 80a1fca6e2..1b871d06bc 100644 --- a/tests/SkSLDSLTest.cpp +++ b/tests/SkSLDSLTest.cpp @@ -1119,11 +1119,17 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLSwitch, r, ctxInfo) { Statement x = Switch(5, Case(0, a = 0, Break()), Case(1, a = 1, Continue()), + Case(2, a = 2 /*Fallthrough*/), Default(Discard()) ); - EXPECT_EQUAL(x, - "switch (5) { case 0: (a = 0.0); break; case 1: (a = 1.0); continue; default: discard; }"); - + EXPECT_EQUAL(x, R"( + switch (5) { + case 0: (a = 0.0); break; + case 1: (a = 1.0); continue; + case 2: (a = 2.0); + default: discard; + } + )"); Statement y = Switch(b); EXPECT_EQUAL(y, "switch (b) {}"); @@ -1131,10 +1137,15 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLSwitch, r, ctxInfo) { EXPECT_EQUAL(z, "switch (b) { default: case 0: case 1: }"); { - ExpectError error(r, "error: duplicate case value\n"); + ExpectError error(r, "error: duplicate case value '0'\n"); Switch(0, Case(0), Case(0)).release(); } + { + ExpectError error(r, "error: duplicate default case\n"); + Switch(0, Default(a = 0), Default(a = 1)).release(); + } + { ExpectError error(r, "error: case value must be a constant integer\n"); Var b(kInt); diff --git a/tests/sksl/errors/SwitchDuplicateCase.glsl b/tests/sksl/errors/SwitchDuplicateCase.glsl index 876cf0caad..eb5a35b062 100644 --- a/tests/sksl/errors/SwitchDuplicateCase.glsl +++ b/tests/sksl/errors/SwitchDuplicateCase.glsl @@ -1,4 +1,4 @@ ### Compilation failed: -error: 5: duplicate case value +error: 5: duplicate case value '0' 1 error