Re-land "Fixed an issue with sksl variable declarations"
This reverts commit affa6a3da87e9ea85f1d4fe3137b5bccbbc56f92.
Bug: skia:
Change-Id: I5864830e31acbf786f3ea7ae91f42f10aae6d855
Reviewed-on: https://skia-review.googlesource.com/18459
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
diff --git a/src/sksl/SkSLCFGGenerator.cpp b/src/sksl/SkSLCFGGenerator.cpp
index 71bd37f..2fe049d 100644
--- a/src/sksl/SkSLCFGGenerator.cpp
+++ b/src/sksl/SkSLCFGGenerator.cpp
@@ -465,13 +465,17 @@
}
case Statement::kVarDeclarations_Kind: {
VarDeclarationsStatement& decls = ((VarDeclarationsStatement&) **s);
- for (auto& vd : decls.fDeclaration->fVars) {
- if (vd->fValue) {
- this->addExpression(cfg, &vd->fValue, true);
+ for (auto& stmt : decls.fDeclaration->fVars) {
+ if (stmt->fKind == Statement::kNop_Kind) {
+ continue;
}
+ 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 f63ef97..dd20b5c 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -262,12 +262,10 @@
}
case BasicBlock::Node::kStatement_Kind: {
const Statement* stmt = (Statement*) node.statement()->get();
- 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;
- }
+ if (stmt->fKind == Statement::kVarDeclaration_Kind) {
+ VarDeclaration& vd = (VarDeclaration&) *stmt;
+ if (vd.fValue) {
+ (*definitions)[vd.fVar] = &vd.fValue;
}
}
break;
@@ -324,7 +322,7 @@
if (s->fKind == Statement::kVarDeclarations_Kind) {
const VarDeclarationsStatement* vd = (const VarDeclarationsStatement*) s;
for (const auto& decl : vd->fDeclaration->fVars) {
- result[decl->fVar] = nullptr;
+ result[((VarDeclaration&) *decl).fVar] = nullptr;
}
}
}
@@ -846,27 +844,19 @@
bool* outNeedsRescan) {
Statement* stmt = (*iter)->statement()->get();
switch (stmt->fKind) {
- 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;
- }
+ 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;
}
- 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 d67f718..c3168e3 100644
--- a/src/sksl/SkSLGLSLCodeGenerator.cpp
+++ b/src/sksl/SkSLGLSLCodeGenerator.cpp
@@ -619,26 +619,33 @@
void GLSLCodeGenerator::writeVarDeclarations(const VarDeclarations& decl, bool global) {
ASSERT(decl.fVars.size() > 0);
- 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) {
+ 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->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());
@@ -647,7 +654,9 @@
fFoundImageDecl = true;
}
}
- this->write(";");
+ if (wroteType) {
+ this->write(";");
+ }
}
void GLSLCodeGenerator::writeStatement(const Statement& s) {
@@ -829,7 +838,8 @@
case ProgramElement::kVar_Kind: {
VarDeclarations& decl = (VarDeclarations&) *e;
if (decl.fVars.size() > 0) {
- int builtin = decl.fVars[0]->fVar->fModifiers.fLayout.fBuiltin;
+ int builtin =
+ ((VarDeclaration&) *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 2e280d8..f85ea10 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -614,19 +614,20 @@
if (!decl) {
return nullptr;
}
- for (const auto& var : decl->fVars) {
+ for (const auto& stmt : decl->fVars) {
+ VarDeclaration& vd = (VarDeclaration&) *stmt;
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(var->fVar->fModifiers, var->fVar->fName,
- &var->fVar->fType));
- if (var->fValue) {
+ fields.push_back(Type::Field(vd.fVar->fModifiers, vd.fVar->fName,
+ &vd.fVar->fType));
+ if (vd.fValue) {
fErrors.error(decl->fPosition,
"initializers are not permitted on interface block fields");
}
- if (var->fVar->fModifiers.fFlags & (Modifiers::kIn_Flag |
+ if (vd.fVar->fModifiers.fFlags & (Modifiers::kIn_Flag |
Modifiers::kOut_Flag |
Modifiers::kUniform_Flag |
Modifiers::kBuffer_Flag |
@@ -634,8 +635,8 @@
fErrors.error(decl->fPosition,
"interface block fields may not have storage qualifiers");
}
- if (var->fVar->fType.kind() == Type::kArray_Kind &&
- var->fVar->fType.columns() == -1) {
+ if (vd.fVar->fType.kind() == Type::kArray_Kind &&
+ vd.fVar->fType.columns() == -1) {
haveRuntimeArray = true;
}
}
diff --git a/src/sksl/SkSLSPIRVCodeGenerator.cpp b/src/sksl/SkSLSPIRVCodeGenerator.cpp
index 9cc25b9..e3786a7 100644
--- a/src/sksl/SkSLSPIRVCodeGenerator.cpp
+++ b/src/sksl/SkSLSPIRVCodeGenerator.cpp
@@ -2603,7 +2603,10 @@
void SPIRVCodeGenerator::writeGlobalVars(Program::Kind kind, const VarDeclarations& decl,
OutputStream& out) {
for (size_t i = 0; i < decl.fVars.size(); i++) {
- const VarDeclaration& varDecl = *decl.fVars[i];
+ if (decl.fVars[i]->fKind == Statement::kNop_Kind) {
+ continue;
+ }
+ const VarDeclaration& varDecl = (VarDeclaration&) *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,8 +2668,12 @@
}
void SPIRVCodeGenerator::writeVarDeclarations(const VarDeclarations& decl, OutputStream& out) {
- for (const auto& varDecl : decl.fVars) {
- const Variable* var = varDecl->fVar;
+ for (const auto& stmt : decl.fVars) {
+ if (stmt->fKind == Statement::kNop_Kind) {
+ continue;
+ }
+ VarDeclaration& varDecl = (VarDeclaration&) *stmt;
+ 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 |
@@ -2679,8 +2686,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 9af18ec..1bc5244 100644
--- a/src/sksl/ir/SkSLStatement.h
+++ b/src/sksl/ir/SkSLStatement.h
@@ -30,6 +30,7 @@
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 dbf2928..c07fee8 100644
--- a/src/sksl/ir/SkSLVarDeclarations.h
+++ b/src/sksl/ir/SkSLVarDeclarations.h
@@ -20,11 +20,12 @@
* 'int x = 2, y[3];' is a VarDeclarations statement containing two individual VarDeclaration
* instances.
*/
-struct VarDeclaration {
+struct VarDeclaration : public Statement {
VarDeclaration(const Variable* var,
std::vector<std::unique_ptr<Expression>> sizes,
std::unique_ptr<Expression> value)
- : fVar(var)
+ : INHERITED(var->fPosition, Statement::kVarDeclaration_Kind)
+ , fVar(var)
, fSizes(std::move(sizes))
, fValue(std::move(value)) {}
@@ -46,6 +47,8 @@
const Variable* fVar;
std::vector<std::unique_ptr<Expression>> fSizes;
std::unique_ptr<Expression> fValue;
+
+ typedef Statement INHERITED;
};
/**
@@ -55,14 +58,18 @@
VarDeclarations(Position position, const Type* baseType,
std::vector<std::unique_ptr<VarDeclaration>> vars)
: INHERITED(position, kVar_Kind)
- , fBaseType(*baseType)
- , fVars(std::move(vars)) {}
+ , fBaseType(*baseType) {
+ for (auto& var : vars) {
+ fVars.push_back(std::unique_ptr<Statement>(var.release()));
+ }
+ }
String description() const override {
if (!fVars.size()) {
return String();
}
- String result = fVars[0]->fVar->fModifiers.description() + fBaseType.description() + " ";
+ String result = ((VarDeclaration&) *fVars[0]).fVar->fModifiers.description() +
+ fBaseType.description() + " ";
String separator;
for (const auto& var : fVars) {
result += separator;
@@ -73,7 +80,9 @@
}
const Type& fBaseType;
- std::vector<std::unique_ptr<VarDeclaration>> fVars;
+ // 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;
typedef ProgramElement INHERITED;
};
diff --git a/src/sksl/ir/SkSLVarDeclarationsStatement.h b/src/sksl/ir/SkSLVarDeclarationsStatement.h
index 50365de..ab67536 100644
--- a/src/sksl/ir/SkSLVarDeclarationsStatement.h
+++ b/src/sksl/ir/SkSLVarDeclarationsStatement.h
@@ -21,6 +21,15 @@
: 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 ba6bbbd..678ff13 100644
--- a/tests/SkSLGLSLTest.cpp
+++ b/tests/SkSLGLSLTest.cpp
@@ -1340,24 +1340,39 @@
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