Revert "Fixed an issue with sksl variable declarations"
This reverts commit 88bd8edcff23dc9cf31b664cba7ba73b235318b0.
Reason for revert: unhappy bots
Original change's description:
> Fixed an issue with sksl variable declarations
>
> There was an issue where multiple variables defined in the same
> declaration were not being sequenced appropriately during analysis, so
> 'int x = 0, y = x + 1' would report that x was undefined.
>
> Bug: skia:
> Change-Id: I882f7e216467306f6a6013a0a34aac30a4c60744
> Reviewed-on: https://skia-review.googlesource.com/18313
> Reviewed-by: Chris Dalton <csmartdalton@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
>
TBR=csmartdalton@google.com,ethannicholas@google.com
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:
Change-Id: Ibc68674289dce70b6173a347a0e78bb0f1e6db1b
Reviewed-on: https://skia-review.googlesource.com/18457
Commit-Queue: Greg Daniel <egdaniel@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
diff --git a/src/sksl/SkSLCFGGenerator.cpp b/src/sksl/SkSLCFGGenerator.cpp
index 2fe049d..71bd37f 100644
--- a/src/sksl/SkSLCFGGenerator.cpp
+++ b/src/sksl/SkSLCFGGenerator.cpp
@@ -465,17 +465,13 @@
}
case Statement::kVarDeclarations_Kind: {
VarDeclarationsStatement& decls = ((VarDeclarationsStatement&) **s);
- for (auto& stmt : decls.fDeclaration->fVars) {
- if (stmt->fKind == Statement::kNop_Kind) {
- continue;
+ for (auto& vd : decls.fDeclaration->fVars) {
+ if (vd->fValue) {
+ this->addExpression(cfg, &vd->fValue, true);
}
- VarDeclaration& vd = (VarDeclaration&) *stmt;
- if (vd.fValue) {
- this->addExpression(cfg, &vd.fValue, true);
- }
- cfg.fBlocks[cfg.fCurrent].fNodes.push_back({ BasicBlock::Node::kStatement_Kind,
- false, nullptr, &stmt });
}
+ cfg.fBlocks[cfg.fCurrent].fNodes.push_back({ BasicBlock::Node::kStatement_Kind, false,
+ nullptr, s });
break;
}
case Statement::kDiscard_Kind:
diff --git a/src/sksl/SkSLCompiler.cpp b/src/sksl/SkSLCompiler.cpp
index dd20b5c..f63ef97 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -262,10 +262,12 @@
}
case BasicBlock::Node::kStatement_Kind: {
const Statement* stmt = (Statement*) node.statement()->get();
- if (stmt->fKind == Statement::kVarDeclaration_Kind) {
- VarDeclaration& vd = (VarDeclaration&) *stmt;
- if (vd.fValue) {
- (*definitions)[vd.fVar] = &vd.fValue;
+ if (stmt->fKind == Statement::kVarDeclarations_Kind) {
+ VarDeclarationsStatement* vd = (VarDeclarationsStatement*) stmt;
+ for (const auto& decl : vd->fDeclaration->fVars) {
+ if (decl->fValue) {
+ (*definitions)[decl->fVar] = &decl->fValue;
+ }
}
}
break;
@@ -322,7 +324,7 @@
if (s->fKind == Statement::kVarDeclarations_Kind) {
const VarDeclarationsStatement* vd = (const VarDeclarationsStatement*) s;
for (const auto& decl : vd->fDeclaration->fVars) {
- result[((VarDeclaration&) *decl).fVar] = nullptr;
+ result[decl->fVar] = nullptr;
}
}
}
@@ -844,19 +846,27 @@
bool* outNeedsRescan) {
Statement* stmt = (*iter)->statement()->get();
switch (stmt->fKind) {
- case Statement::kVarDeclaration_Kind: {
- const auto& varDecl = (VarDeclaration&) *stmt;
- if (varDecl.fVar->dead() &&
- (!varDecl.fValue ||
- !varDecl.fValue->hasSideEffects())) {
- if (varDecl.fValue) {
- ASSERT((*iter)->statement()->get() == stmt);
- if (!b.tryRemoveExpressionBefore(iter, varDecl.fValue.get())) {
- *outNeedsRescan = true;
+ case Statement::kVarDeclarations_Kind: {
+ VarDeclarations& vd = *((VarDeclarationsStatement&) *stmt).fDeclaration;
+ for (auto varIter = vd.fVars.begin(); varIter != vd.fVars.end(); ) {
+ const auto& varDecl = **varIter;
+ if (varDecl.fVar->dead() &&
+ (!varDecl.fValue ||
+ !varDecl.fValue->hasSideEffects())) {
+ if (varDecl.fValue) {
+ ASSERT((*iter)->statement()->get() == stmt);
+ if (!b.tryRemoveExpressionBefore(iter, varDecl.fValue.get())) {
+ *outNeedsRescan = true;
+ }
}
+ varIter = vd.fVars.erase(varIter);
+ *outUpdated = true;
+ } else {
+ ++varIter;
}
+ }
+ if (vd.fVars.size() == 0) {
(*iter)->setStatement(std::unique_ptr<Statement>(new Nop()));
- *outUpdated = true;
}
break;
}
diff --git a/src/sksl/SkSLGLSLCodeGenerator.cpp b/src/sksl/SkSLGLSLCodeGenerator.cpp
index c3168e3..d67f718 100644
--- a/src/sksl/SkSLGLSLCodeGenerator.cpp
+++ b/src/sksl/SkSLGLSLCodeGenerator.cpp
@@ -619,33 +619,26 @@
void GLSLCodeGenerator::writeVarDeclarations(const VarDeclarations& decl, bool global) {
ASSERT(decl.fVars.size() > 0);
- bool wroteType = false;
- for (const auto& stmt : decl.fVars) {
- if (stmt->fKind == Statement::kNop_Kind) {
- continue;
- }
- VarDeclaration& var = (VarDeclaration&) *stmt;
- if (wroteType) {
- this->write(", ");
- } else {
- this->writeModifiers(var.fVar->fModifiers, global);
- this->writeType(decl.fBaseType);
- this->write(" ");
- wroteType = true;
- }
- this->write(var.fVar->fName);
- for (const auto& size : var.fSizes) {
+ this->writeModifiers(decl.fVars[0]->fVar->fModifiers, global);
+ this->writeType(decl.fBaseType);
+ String separator(" ");
+ for (const auto& var : decl.fVars) {
+ ASSERT(var->fVar->fModifiers == decl.fVars[0]->fVar->fModifiers);
+ this->write(separator);
+ separator = String(", ");
+ this->write(var->fVar->fName);
+ for (const auto& size : var->fSizes) {
this->write("[");
if (size) {
this->writeExpression(*size, kTopLevel_Precedence);
}
this->write("]");
}
- if (var.fValue) {
+ if (var->fValue) {
this->write(" = ");
- this->writeExpression(*var.fValue, kTopLevel_Precedence);
+ this->writeExpression(*var->fValue, kTopLevel_Precedence);
}
- if (!fFoundImageDecl && var.fVar->fType == *fContext.fImage2D_Type) {
+ if (!fFoundImageDecl && var->fVar->fType == *fContext.fImage2D_Type) {
if (fProgram.fSettings.fCaps->imageLoadStoreExtensionString()) {
fHeader.writeText("#extension ");
fHeader.writeText(fProgram.fSettings.fCaps->imageLoadStoreExtensionString());
@@ -654,9 +647,7 @@
fFoundImageDecl = true;
}
}
- if (wroteType) {
- this->write(";");
- }
+ this->write(";");
}
void GLSLCodeGenerator::writeStatement(const Statement& s) {
@@ -838,8 +829,7 @@
case ProgramElement::kVar_Kind: {
VarDeclarations& decl = (VarDeclarations&) *e;
if (decl.fVars.size() > 0) {
- int builtin =
- ((VarDeclaration&) *decl.fVars[0]).fVar->fModifiers.fLayout.fBuiltin;
+ int builtin = decl.fVars[0]->fVar->fModifiers.fLayout.fBuiltin;
if (builtin == -1) {
// normal var
this->writeVarDeclarations(decl, true);
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index f85ea10..2e280d8 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -614,20 +614,19 @@
if (!decl) {
return nullptr;
}
- for (const auto& stmt : decl->fVars) {
- VarDeclaration& vd = (VarDeclaration&) *stmt;
+ for (const auto& var : decl->fVars) {
if (haveRuntimeArray) {
fErrors.error(decl->fPosition,
"only the last entry in an interface block may be a runtime-sized "
"array");
}
- fields.push_back(Type::Field(vd.fVar->fModifiers, vd.fVar->fName,
- &vd.fVar->fType));
- if (vd.fValue) {
+ fields.push_back(Type::Field(var->fVar->fModifiers, var->fVar->fName,
+ &var->fVar->fType));
+ if (var->fValue) {
fErrors.error(decl->fPosition,
"initializers are not permitted on interface block fields");
}
- if (vd.fVar->fModifiers.fFlags & (Modifiers::kIn_Flag |
+ if (var->fVar->fModifiers.fFlags & (Modifiers::kIn_Flag |
Modifiers::kOut_Flag |
Modifiers::kUniform_Flag |
Modifiers::kBuffer_Flag |
@@ -635,8 +634,8 @@
fErrors.error(decl->fPosition,
"interface block fields may not have storage qualifiers");
}
- if (vd.fVar->fType.kind() == Type::kArray_Kind &&
- vd.fVar->fType.columns() == -1) {
+ if (var->fVar->fType.kind() == Type::kArray_Kind &&
+ var->fVar->fType.columns() == -1) {
haveRuntimeArray = true;
}
}
diff --git a/src/sksl/SkSLSPIRVCodeGenerator.cpp b/src/sksl/SkSLSPIRVCodeGenerator.cpp
index 20d292f..9cc25b9 100644
--- a/src/sksl/SkSLSPIRVCodeGenerator.cpp
+++ b/src/sksl/SkSLSPIRVCodeGenerator.cpp
@@ -2603,7 +2603,7 @@
void SPIRVCodeGenerator::writeGlobalVars(Program::Kind kind, const VarDeclarations& decl,
OutputStream& out) {
for (size_t i = 0; i < decl.fVars.size(); i++) {
- const VarDeclaration& varDecl = (VarDeclaration&) *decl.fVars[i];
+ const VarDeclaration& varDecl = *decl.fVars[i];
const Variable* var = varDecl.fVar;
// These haven't been implemented in our SPIR-V generator yet and we only currently use them
// in the OpenGL backend.
@@ -2665,9 +2665,8 @@
}
void SPIRVCodeGenerator::writeVarDeclarations(const VarDeclarations& decl, OutputStream& out) {
- for (const auto& stmt : decl.fVars) {
- VarDeclaration& varDecl = (VarDeclaration&) *stmt;
- const Variable* var = varDecl.fVar;
+ for (const auto& varDecl : decl.fVars) {
+ const Variable* var = varDecl->fVar;
// These haven't been implemented in our SPIR-V generator yet and we only currently use them
// in the OpenGL backend.
ASSERT(!(var->fModifiers.fFlags & (Modifiers::kReadOnly_Flag |
@@ -2680,8 +2679,8 @@
SpvId type = this->getPointerType(var->fType, SpvStorageClassFunction);
this->writeInstruction(SpvOpVariable, type, id, SpvStorageClassFunction, fVariableBuffer);
this->writeInstruction(SpvOpName, id, var->fName.c_str(), fNameBuffer);
- if (varDecl.fValue) {
- SpvId value = this->writeExpression(*varDecl.fValue, out);
+ if (varDecl->fValue) {
+ SpvId value = this->writeExpression(*varDecl->fValue, out);
this->writeInstruction(SpvOpStore, id, value, out);
}
}
diff --git a/src/sksl/ir/SkSLStatement.h b/src/sksl/ir/SkSLStatement.h
index 1bc5244..9af18ec 100644
--- a/src/sksl/ir/SkSLStatement.h
+++ b/src/sksl/ir/SkSLStatement.h
@@ -30,7 +30,6 @@
kNop_Kind,
kReturn_Kind,
kSwitch_Kind,
- kVarDeclaration_Kind,
kVarDeclarations_Kind,
kWhile_Kind
};
diff --git a/src/sksl/ir/SkSLVarDeclarations.h b/src/sksl/ir/SkSLVarDeclarations.h
index c07fee8..dbf2928 100644
--- a/src/sksl/ir/SkSLVarDeclarations.h
+++ b/src/sksl/ir/SkSLVarDeclarations.h
@@ -20,12 +20,11 @@
* 'int x = 2, y[3];' is a VarDeclarations statement containing two individual VarDeclaration
* instances.
*/
-struct VarDeclaration : public Statement {
+struct VarDeclaration {
VarDeclaration(const Variable* var,
std::vector<std::unique_ptr<Expression>> sizes,
std::unique_ptr<Expression> value)
- : INHERITED(var->fPosition, Statement::kVarDeclaration_Kind)
- , fVar(var)
+ : fVar(var)
, fSizes(std::move(sizes))
, fValue(std::move(value)) {}
@@ -47,8 +46,6 @@
const Variable* fVar;
std::vector<std::unique_ptr<Expression>> fSizes;
std::unique_ptr<Expression> fValue;
-
- typedef Statement INHERITED;
};
/**
@@ -58,18 +55,14 @@
VarDeclarations(Position position, const Type* baseType,
std::vector<std::unique_ptr<VarDeclaration>> vars)
: INHERITED(position, kVar_Kind)
- , fBaseType(*baseType) {
- for (auto& var : vars) {
- fVars.push_back(std::unique_ptr<Statement>(var.release()));
- }
- }
+ , fBaseType(*baseType)
+ , fVars(std::move(vars)) {}
String description() const override {
if (!fVars.size()) {
return String();
}
- String result = ((VarDeclaration&) *fVars[0]).fVar->fModifiers.description() +
- fBaseType.description() + " ";
+ String result = fVars[0]->fVar->fModifiers.description() + fBaseType.description() + " ";
String separator;
for (const auto& var : fVars) {
result += separator;
@@ -80,9 +73,7 @@
}
const Type& fBaseType;
- // this *should* be a vector of unique_ptr<VarDeclaration>, but it significantly simplifies the
- // CFG to only have to worry about unique_ptr<Statement>
- std::vector<std::unique_ptr<Statement>> fVars;
+ std::vector<std::unique_ptr<VarDeclaration>> fVars;
typedef ProgramElement INHERITED;
};
diff --git a/src/sksl/ir/SkSLVarDeclarationsStatement.h b/src/sksl/ir/SkSLVarDeclarationsStatement.h
index ab67536..50365de 100644
--- a/src/sksl/ir/SkSLVarDeclarationsStatement.h
+++ b/src/sksl/ir/SkSLVarDeclarationsStatement.h
@@ -21,15 +21,6 @@
: INHERITED(decl->fPosition, kVarDeclarations_Kind)
, fDeclaration(std::move(decl)) {}
- bool isEmpty() const override {
- for (const auto& s : fDeclaration->fVars) {
- if (!s->isEmpty()) {
- return false;
- }
- }
- return true;
- }
-
String description() const override {
return fDeclaration->description();
}
diff --git a/tests/SkSLGLSLTest.cpp b/tests/SkSLGLSLTest.cpp
index 678ff13..ba6bbbd 100644
--- a/tests/SkSLGLSLTest.cpp
+++ b/tests/SkSLGLSLTest.cpp
@@ -1340,39 +1340,24 @@
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");
+ "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");
}
-
-DEF_TEST(SkSLDependentInitializers, r) {
- test(r,
- "void main() {"
- "float x = 0.5, y = x * 2;"
- "sk_FragColor = vec4(y);"
- "}",
- *SkSL::ShaderCapsFactory::Default(),
- "#version 400\n"
- "out vec4 sk_FragColor;\n"
- "void main() {\n"
- " sk_FragColor = vec4(1.0);\n"
- "}\n");
-}
-
#endif