Remove variable-declared flags from DSLVarBase.

The DSLVarBase `fDeclared` field was used to track whether a variable
was created with `Var`, but never declared with a matching `Declare`, in
a hand-authored DSL program. This was expected to be a common source of
slip-ups in hand-authored DSL, so we tracked it and printed a handy
error message if `Declare` was ever missing. However, this complicated
testing, because in tiny test snippets we _do_ want the ability to
create variables without necessarily declaring them. So, we had a
separate program setting to disable enforcement for test code.

For programs created in DSLParser, tracking this state is unnecessary;
the parser won't ever forget to declare variables.

Since hand-authored DSL programs are no longer expected to be
commonplace, we can remove the state tracking and the associated
ProgramSettings field entirely.

Change-Id: I32b28f8a2ca6591d3da80cd974fff8101f5a4be8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/541638
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This commit is contained in:
John Stiles 2022-05-17 23:22:10 -04:00 committed by SkCQ
parent d35e9151bc
commit e76f507164
10 changed files with 23 additions and 122 deletions

View File

@ -35,7 +35,7 @@ public:
/**
* Creates an empty, unpopulated var. Can be replaced with a real var later via `swap`.
*/
DSLVarBase() : fType(kVoid_Type), fDeclared(true) {}
DSLVarBase() : fType(kVoid_Type) {}
/**
* Constructs a new variable with the specified type and name. The name is used (in mangled
@ -44,7 +44,7 @@ public:
* parameter is optional.
*/
DSLVarBase(DSLType type, std::string_view name, DSLExpression initialValue, Position pos,
Position namePos);
Position namePos);
DSLVarBase(DSLType type, DSLExpression initialValue, Position pos, Position namePos);
@ -139,9 +139,7 @@ protected:
Position fNamePosition;
std::string_view fName;
DSLExpression fInitialValue;
// true if we have attempted to create the SkSL var
bool fInitialized = false;
bool fDeclared = false;
Position fPosition;
friend class DSLCore;

View File

@ -75,8 +75,6 @@ struct ProgramSettings {
// every temporary value, even ones that would otherwise be optimized away entirely. The other
// debug opcodes are much less invasive on the generated code.
bool fAllowTraceVarInSkVMDebugTrace = true;
// If true, the DSL should automatically mark variables declared upon creation.
bool fDSLMarkVarsDeclared = false;
// If true, the DSL should install a memory pool when possible.
bool fDSLUseMemoryPool = true;
// If true, DSL objects assert that they were used prior to destruction

View File

@ -210,14 +210,12 @@ generated_cc_atom(
"//include/core:SkTypes_hdr",
"//include/private:SkSLDefines_hdr",
"//include/private:SkSLStatement_hdr",
"//include/private:SkSLString_hdr",
"//include/private:SkSLSymbol_hdr",
"//include/sksl:DSLModifiers_hdr",
"//include/sksl:DSLType_hdr",
"//include/sksl:DSLVar_hdr",
"//include/sksl:SkSLOperator_hdr",
"//src/sksl:SkSLThreadContext_hdr",
"//src/sksl/dsl/priv:DSLWriter_hdr",
"//src/sksl/ir:SkSLBinaryExpression_hdr",
"//src/sksl/ir:SkSLExpression_hdr",
"//src/sksl/ir:SkSLFieldAccess_hdr",

View File

@ -163,10 +163,6 @@ public:
}
static DSLStatement Declare(DSLVar& var, Position pos) {
if (var.fDeclared) {
ThreadContext::ReportError("variable has already been declared", pos);
}
var.fDeclared = true;
return DSLWriter::Declaration(var);
}
@ -179,10 +175,6 @@ public:
}
static void Declare(DSLGlobalVar& var, Position pos) {
if (var.fDeclared) {
ThreadContext::ReportError("variable has already been declared", pos);
}
var.fDeclared = true;
std::unique_ptr<SkSL::Statement> stmt = DSLWriter::Declaration(var);
if (stmt) {
if (!stmt->isEmpty()) {
@ -276,11 +268,6 @@ public:
DSLType varType = arraySize > 0 ? Array(structType, arraySize) : DSLType(structType);
DSLGlobalVar var(modifiers, varType, !varName.empty() ? varName : typeName, DSLExpression(),
pos);
// Interface blocks can't be declared, so we always need to mark the var declared ourselves.
// We do this only when fDSLMarkVarDeclared is false, so we don't double-declare it.
if (!ThreadContext::Settings().fDSLMarkVarsDeclared) {
DSLWriter::MarkDeclared(var);
}
const SkSL::Variable* skslVar = DSLWriter::Var(var);
if (skslVar) {
auto intf = std::make_unique<SkSL::InterfaceBlock>(pos, *skslVar, typeName, varName,

View File

@ -52,12 +52,8 @@ void DSLFunction::init(DSLModifiers modifiers, const DSLType& returnType, std::s
std::vector<std::unique_ptr<Variable>> paramVars;
paramVars.reserve(params.size());
for (DSLParameter* param : params) {
if (param->fDeclared) {
ThreadContext::ReportError("parameter has already been used in another function");
}
SkASSERT(!param->fInitialValue.hasValue());
SkASSERT(!param->fDeclaration);
param->fDeclared = true;
std::unique_ptr<SkSL::Variable> paramVar = DSLWriter::CreateParameterVar(*param);
if (!paramVar) {
return;

View File

@ -10,13 +10,11 @@
#include "include/core/SkTypes.h"
#include "include/private/SkSLDefines.h"
#include "include/private/SkSLStatement.h"
#include "include/private/SkSLString.h"
#include "include/private/SkSLSymbol.h"
#include "include/sksl/DSLModifiers.h"
#include "include/sksl/DSLType.h"
#include "include/sksl/SkSLOperator.h"
#include "src/sksl/SkSLThreadContext.h"
#include "src/sksl/dsl/priv/DSLWriter.h"
#include "src/sksl/ir/SkSLBinaryExpression.h"
#include "src/sksl/ir/SkSLExpression.h"
#include "src/sksl/ir/SkSLFieldAccess.h"
@ -24,8 +22,6 @@
#include "src/sksl/ir/SkSLSymbolTable.h"
#include "src/sksl/ir/SkSLVariable.h"
#include <string>
namespace SkSL {
namespace dsl {
@ -48,17 +44,9 @@ DSLVarBase::DSLVarBase(const DSLModifiers& modifiers, DSLType type, std::string_
, fNamePosition(namePos)
, fName(name)
, fInitialValue(std::move(initialValue))
, fDeclared(DSLWriter::MarkVarsDeclared())
, fPosition(pos) {}
DSLVarBase::~DSLVarBase() {
if (fDeclaration && !fDeclared) {
ThreadContext::ReportError(String::printf("variable '%.*s' was destroyed without being "
"declared",
(int)fName.length(),
fName.data()).c_str());
}
}
DSLVarBase::~DSLVarBase() {}
void DSLVarBase::swap(DSLVarBase& other) {
SkASSERT(this->storage() == other.storage());
@ -70,7 +58,6 @@ void DSLVarBase::swap(DSLVarBase& other) {
std::swap(fNamePosition, other.fNamePosition);
std::swap(fName, other.fName);
std::swap(fInitialValue.fExpression, other.fInitialValue.fExpression);
std::swap(fDeclared, other.fDeclared);
std::swap(fInitialized, other.fInitialized);
std::swap(fPosition, other.fPosition);
}
@ -86,7 +73,6 @@ VariableStorage DSLVar::storage() const {
DSLGlobalVar::DSLGlobalVar(const char* name)
: INHERITED(kVoid_Type, name, DSLExpression(), Position(), Position()) {
fName = name;
DSLWriter::MarkDeclared(*this);
const SkSL::Symbol* result = (*ThreadContext::SymbolTable())[fName];
SkASSERTF(result, "could not find '%.*s' in symbol table", (int)fName.length(), fName.data());
fVar = &result->as<SkSL::Variable>();

View File

@ -35,7 +35,6 @@ generated_cc_atom(
"//include/sksl:DSLVar_hdr",
"//include/sksl:SkSLPosition_hdr",
"//src/sksl:SkSLModifiersPool_hdr",
"//src/sksl:SkSLProgramSettings_hdr",
"//src/sksl:SkSLThreadContext_hdr",
"//src/sksl/ir:SkSLBlock_hdr",
"//src/sksl/ir:SkSLExpression_hdr",

View File

@ -20,7 +20,6 @@
#include "include/sksl/DSLVar.h"
#include "include/sksl/SkSLPosition.h"
#include "src/sksl/SkSLModifiersPool.h"
#include "src/sksl/SkSLProgramSettings.h"
#include "src/sksl/SkSLThreadContext.h"
#include "src/sksl/ir/SkSLBlock.h"
#include "src/sksl/ir/SkSLExpression.h"
@ -88,15 +87,6 @@ std::unique_ptr<SkSL::Statement> DSLWriter::Declaration(DSLVarBase& var) {
return std::move(var.fDeclaration);
}
void DSLWriter::MarkDeclared(DSLVarBase& var) {
SkASSERT(!var.fDeclared);
var.fDeclared = true;
}
bool DSLWriter::MarkVarsDeclared() {
return ThreadContext::Instance().fSettings.fDSLMarkVarsDeclared;
}
void DSLWriter::AddVarDeclaration(DSLStatement& existing, DSLVar& additional) {
if (existing.fStatement->is<Block>()) {
SkSL::Block& block = existing.fStatement->as<Block>();

View File

@ -49,18 +49,6 @@ public:
*/
static std::unique_ptr<SkSL::Statement> Declaration(DSLVarBase& var);
/**
* For use in testing only: marks the variable as having been declared, so that it can be
* destroyed without generating errors.
*/
static void MarkDeclared(DSLVarBase& var);
/**
* Returns whether DSLVars should automatically be marked declared upon creation. This is used
* to simplify testing.
*/
static bool MarkVarsDeclared();
/**
* Adds a new declaration into an existing declaration statement. This either turns the original
* declaration into an unscoped block or, if it already was, appends a new statement to the end

View File

@ -53,22 +53,11 @@
using namespace SkSL::dsl;
SkSL::ProgramSettings default_settings() {
SkSL::ProgramSettings result;
result.fDSLMarkVarsDeclared = true;
return result;
}
SkSL::ProgramSettings no_mark_vars_declared() {
SkSL::ProgramSettings result = default_settings();
result.fDSLMarkVarsDeclared = false;
return result;
return SkSL::ProgramSettings{};
}
/**
* Issues an automatic Start() and End(), and optionally auto-declares variables during its
* lifetime. Variable auto-declaration simplifies testing so we don't have to sprinkle all the tests
* with a bunch of Declare(foo).release() calls just to avoid errors, especially given that some of
* the variables have options that make them an error to actually declare.
* Issues an automatic Start() and End().
*/
class AutoDSLContext {
public:
@ -185,7 +174,7 @@ static void expect_equal(skiatest::Reporter* r, int lineNumber, T&& dsl, const c
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLFlags, r, ctxInfo) {
{
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
EXPECT_EQUAL(All(GreaterThan(Float4(1), Float4(0))), "true");
Var x(kInt_Type, "x");
@ -1301,7 +1290,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLCall, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLBlock, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
EXPECT_EQUAL(Block(), "{ }");
Var a(kInt_Type, "a", 1), b(kInt_Type, "b", 2);
EXPECT_EQUAL(Block(Declare(a), Declare(b), a = b), "{ int a = 1; int b = 2; (a = b); }");
@ -1315,7 +1304,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLBlock, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLBreak, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
Var i(kInt_Type, "i", 0);
DSLFunction(kVoid_Type, "success").define(
For(Declare(i), i < 10, ++i, Block(
@ -1335,7 +1324,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLBreak, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLContinue, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
Var i(kInt_Type, "i", 0);
DSLFunction(kVoid_Type, "success").define(
For(Declare(i), i < 10, ++i, Block(
@ -1355,7 +1344,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLContinue, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLDeclare, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
{
Var a(kHalf4_Type, "a"), b(kHalf4_Type, "b", Half4(1));
EXPECT_EQUAL(Declare(a), "half4 a;");
@ -1400,14 +1389,6 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLDeclare, r, ctxInfo) {
Declare(a).release();
}
{
DSLWriter::Reset();
Var a(kInt_Type, "a");
Declare(a).release();
ExpectError error(r, "variable has already been declared");
Declare(a).release();
}
{
DSLWriter::Reset();
Var a(kUniform_Modifier, kInt_Type, "a");
@ -1417,7 +1398,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLDeclare, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLDeclareGlobal, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
DSLGlobalVar x(kInt_Type, "x", 0);
Declare(x);
DSLGlobalVar y(kUniform_Modifier, kFloat2_Type, "y");
@ -1449,7 +1430,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLDo, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLFor, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
EXPECT_EQUAL(For(Statement(), Expression(), Expression(), Block()),
"for (;;) {}");
@ -1479,7 +1460,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLFor, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLFunction, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
Parameter coords(kFloat2_Type, "coords");
DSLFunction(kVoid_Type, "main", coords).define(
sk_FragColor() = Half4(coords, 0, 1)
@ -1526,8 +1507,6 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLFunction, r, ctxInfo) {
);
Var varArg1(kFloat_Type, "varArg1");
Var varArg2(kFloat_Type, "varArg2");
DSLWriter::MarkDeclared(varArg1);
DSLWriter::MarkDeclared(varArg2);
EXPECT_EQUAL(pair(varArg1, varArg2), "pair(varArg1, varArg2)");
}
@ -1571,16 +1550,6 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLFunction, r, ctxInfo) {
DSLFunction(kFloat_Type, "broken").define(
);
}
{
ExpectError error(r, "parameter has already been used in another function");
DSLWriter::Reset();
DSLParameter p(kFloat_Type);
DSLFunction(kVoid_Type, "ok", p).define(
);
DSLFunction(kVoid_Type, "broken", p).define(
);
}
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLIf, r, ctxInfo) {
@ -1748,7 +1717,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLSwizzle, r, ctxInfo) {
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLVarSwap, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
// We should be able to convert `a` into a proper var by swapping it, even from within a scope.
Var a;
@ -1861,7 +1830,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLBuiltins, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLModifiers, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
Var v1(kConst_Modifier, kInt_Type, "v1", 0);
Statement d1 = Declare(v1);
@ -1872,38 +1841,30 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLModifiers, r, ctxInfo) {
// TODO: better tests when able
Var v2(kIn_Modifier, kInt_Type, "v2");
REPORTER_ASSERT(r, v2.modifiers().flags() == SkSL::Modifiers::kIn_Flag);
DSLWriter::MarkDeclared(v2);
Var v3(kOut_Modifier, kInt_Type, "v3");
REPORTER_ASSERT(r, v3.modifiers().flags() == SkSL::Modifiers::kOut_Flag);
DSLWriter::MarkDeclared(v3);
Var v4(kFlat_Modifier, kInt_Type, "v4");
REPORTER_ASSERT(r, v4.modifiers().flags() == SkSL::Modifiers::kFlat_Flag);
DSLWriter::MarkDeclared(v4);
Var v5(kNoPerspective_Modifier, kInt_Type, "v5");
REPORTER_ASSERT(r, v5.modifiers().flags() == SkSL::Modifiers::kNoPerspective_Flag);
DSLWriter::MarkDeclared(v5);
Var v6(kIn_Modifier | kOut_Modifier, kInt_Type, "v6");
REPORTER_ASSERT(r, v6.modifiers().flags() == (SkSL::Modifiers::kIn_Flag |
SkSL::Modifiers::kOut_Flag));
DSLWriter::MarkDeclared(v6);
Var v7(kInOut_Modifier, kInt_Type, "v7");
REPORTER_ASSERT(r, v7.modifiers().flags() == (SkSL::Modifiers::kIn_Flag |
SkSL::Modifiers::kOut_Flag));
DSLWriter::MarkDeclared(v7);
Var v8(kUniform_Modifier, kInt_Type, "v8");
REPORTER_ASSERT(r, v8.modifiers().flags() == SkSL::Modifiers::kUniform_Flag);
DSLWriter::MarkDeclared(v8);
// Uniforms do not need to be explicitly declared
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLLayout, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
Var v1(DSLModifiers(DSLLayout().location(1).offset(4).index(5).builtin(6)
.inputAttachmentIndex(7),
kConst_Modifier), kInt_Type, "v1", 0);
@ -2001,7 +1962,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLSampleShader, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLStruct, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
DSLType simpleStruct = Struct("SimpleStruct",
Field(kFloat_Type, "x"),
@ -2047,7 +2008,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLWrapper, r, ctxInfo) {
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLRTAdjust, r, ctxInfo) {
{
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared(),
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), default_settings(),
SkSL::ProgramKind::kVertex);
DSLGlobalVar rtAdjust(kUniform_Modifier, kFloat4_Type, "sk_RTAdjust");
Declare(rtAdjust);
@ -2064,7 +2025,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLRTAdjust, r, ctxInfo) {
}
{
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared(),
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), default_settings(),
SkSL::ProgramKind::kVertex);
REPORTER_ASSERT(r, !SkSL::ThreadContext::RTAdjustState().fInterfaceBlock);
@ -2076,7 +2037,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLRTAdjust, r, ctxInfo) {
}
{
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared(),
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), default_settings(),
SkSL::ProgramKind::kVertex);
ExpectError error(r, "sk_RTAdjust must have type 'float4'");
InterfaceBlock(kUniform_Modifier, "uniforms",
@ -2084,7 +2045,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLRTAdjust, r, ctxInfo) {
}
{
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared(),
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), default_settings(),
SkSL::ProgramKind::kVertex);
ExpectError error(r, "symbol 'sk_RTAdjust' was already defined");
InterfaceBlock(kUniform_Modifier, "uniforms1",
@ -2095,7 +2056,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLRTAdjust, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLInlining, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
DSLParameter x(kFloat_Type, "x");
DSLFunction sqr(kFloat_Type, "sqr", x);
sqr.define(
@ -2127,7 +2088,7 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLReleaseUnused, r, ctxInfo) {
}
DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLPrototypes, r, ctxInfo) {
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu(), no_mark_vars_declared());
AutoDSLContext context(ctxInfo.directContext()->priv().getGpu());
{
DSLParameter x(kFloat_Type, "x");
DSLFunction sqr(kFloat_Type, "sqr", x);