Simplify VarDeclaration by removing multi-dimensional array support.
Maintaining an array of Expression-based sizes is not necessary as GLSL
only supports a single dimension, and doesn't allow any expression other
than a constant integer or nothing (meaning "unsized").
Change-Id: I01b5f88b94234a27e694aa2fc087f9d5f01b99c5
Bug: skia:11026
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340341
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/SkSLAnalysis.cpp b/src/sksl/SkSLAnalysis.cpp
index c16f5d0..0feb4cb 100644
--- a/src/sksl/SkSLAnalysis.cpp
+++ b/src/sksl/SkSLAnalysis.cpp
@@ -612,11 +612,6 @@
}
case Statement::Kind::kVarDeclaration: {
auto& v = s.template as<VarDeclaration>();
- for (const std::unique_ptr<Expression>& size : v.sizes()) {
- if (size && this->visitExpression(*size)) {
- return true;
- }
- }
return v.value() && this->visitExpression(*v.value());
}
case Statement::Kind::kWhile: {
diff --git a/src/sksl/SkSLDehydrator.cpp b/src/sksl/SkSLDehydrator.cpp
index 66f9cc9..5475c21 100644
--- a/src/sksl/SkSLDehydrator.cpp
+++ b/src/sksl/SkSLDehydrator.cpp
@@ -490,10 +490,7 @@
this->writeCommand(Rehydrator::kVarDeclaration_Command);
this->writeU16(this->symbolId(&v.var()));
this->write(v.baseType());
- this->writeU8(v.sizes().count());
- for (const std::unique_ptr<Expression>& size : v.sizes()) {
- this->write(size.get());
- }
+ this->writeS8(v.arraySize());
this->write(v.value().get());
break;
}
diff --git a/src/sksl/SkSLGLSLCodeGenerator.cpp b/src/sksl/SkSLGLSLCodeGenerator.cpp
index edbf890..a2a619f 100644
--- a/src/sksl/SkSLGLSLCodeGenerator.cpp
+++ b/src/sksl/SkSLGLSLCodeGenerator.cpp
@@ -1273,12 +1273,12 @@
this->writeType(var.baseType());
this->write(" ");
this->write(var.var().name());
- for (const std::unique_ptr<Expression>& size : var.sizes()) {
+ if (var.arraySize() > 0) {
this->write("[");
- if (size) {
- this->writeExpression(*size, kTopLevel_Precedence);
- }
+ this->write(to_string(var.arraySize()));
this->write("]");
+ } else if (var.arraySize() == Type::kUnsizedArray){
+ this->write("[]");
}
if (var.value()) {
this->write(" = ");
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index 1e14256..f750856 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -373,17 +373,16 @@
}
const ASTNode::VarData& varData = varDecl.getVarData();
const Type* type = baseType;
- ExpressionArray sizes;
- sizes.reserve_back(varData.fSizeCount);
+ int arraySize = 0;
auto iter = varDecl.begin();
if (iter != varDecl.end()) {
- if (type->isOpaque()) {
- fErrors.error(type->fOffset,
- "opaque type '" + type->name() + "' may not be used in an array");
- }
- SkSTArray<kMaxArrayDimensionality, int> dimensions;
- for (size_t i = 0; i < varData.fSizeCount; ++i, ++iter) {
- const ASTNode& rawSize = *iter;
+ if (varData.fSizeCount > 0) {
+ SkASSERT(varData.fSizeCount == 1); // only single-dimension arrays are supported
+ if (type->isOpaque()) {
+ fErrors.error(type->fOffset,
+ "opaque type '" + type->name() + "' may not be used in an array");
+ }
+ const ASTNode& rawSize = *iter++;
if (rawSize) {
auto size = this->coerce(this->convertExpression(rawSize), *fContext.fInt_Type);
if (!size) {
@@ -400,17 +399,12 @@
fErrors.error(size->fOffset, "array size must be positive");
return {};
}
- dimensions.push_back(count);
- sizes.push_back(std::move(size));
- } else if (i == 0) {
- dimensions.push_back(Type::kUnsizedArray);
- sizes.push_back(nullptr);
+ arraySize = count;
} else {
- fErrors.error(varDecl.fOffset, "array size must be specified");
- return {};
+ arraySize = Type::kUnsizedArray;
}
+ type = fSymbolTable->addArrayDimensions(type, {arraySize});
}
- type = fSymbolTable->addArrayDimensions(type, dimensions);
}
auto var = std::make_unique<Variable>(varDecl.fOffset, fModifiers->addToPool(modifiers),
varData.fName, type, fIsBuiltinCode, storage);
@@ -421,7 +415,7 @@
}
std::unique_ptr<Expression> value;
if (iter == varDecl.end()) {
- if (varData.fSizeCount > 0 && sizes.front() == nullptr) {
+ if (arraySize == Type::kUnsizedArray) {
fErrors.error(varDecl.fOffset,
"arrays without an explicit size must use an initializer expression");
return {};
@@ -444,8 +438,8 @@
if (symbol && storage == Variable::Storage::kGlobal && var->name() == "sk_FragColor") {
// Already defined, ignore.
} else {
- varDecls.push_back(std::make_unique<VarDeclaration>(
- var.get(), baseType, std::move(sizes), std::move(value)));
+ varDecls.push_back(std::make_unique<VarDeclaration>(var.get(), baseType, arraySize,
+ std::move(value)));
fSymbolTable->add(std::move(var));
}
}
@@ -2979,8 +2973,7 @@
fContext.fInt_Type.get(), false,
Variable::Storage::kGlobal);
auto decl = std::make_unique<VarDeclaration>(var.get(), fContext.fInt_Type.get(),
- /*sizes=*/ExpressionArray{},
- /*value=*/nullptr);
+ /*arraySize=*/0, /*value=*/nullptr);
fSymbolTable->add(std::move(var));
fProgramElements->push_back(
std::make_unique<GlobalVarDeclaration>(/*offset=*/-1, std::move(decl)));
diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp
index 5e3e906..34e3b8b 100644
--- a/src/sksl/SkSLInliner.cpp
+++ b/src/sksl/SkSLInliner.cpp
@@ -539,12 +539,8 @@
}
case Statement::Kind::kVarDeclaration: {
const VarDeclaration& decl = statement.as<VarDeclaration>();
- ExpressionArray sizes;
- sizes.reserve_back(decl.sizes().count());
- for (const std::unique_ptr<Expression>& size : decl.sizes()) {
- sizes.push_back(expr(size));
- }
std::unique_ptr<Expression> initialValue = expr(decl.value());
+ int arraySize = decl.arraySize();
const Variable& old = decl.var();
// We assign unique names to inlined variables--scopes hide most of the problems in this
// regard, but see `InlinerAvoidsVariableNameOverlap` for a counterexample where unique
@@ -563,7 +559,7 @@
old.storage(),
initialValue.get()));
(*varMap)[&old] = std::make_unique<VariableReference>(offset, clone);
- return std::make_unique<VarDeclaration>(clone, baseTypePtr, std::move(sizes),
+ return std::make_unique<VarDeclaration>(clone, baseTypePtr, arraySize,
std::move(initialValue));
}
case Statement::Kind::kWhile: {
@@ -643,11 +639,11 @@
// initial value).
std::unique_ptr<Statement> variable;
if (initialValue && (modifiers.fFlags & Modifiers::kOut_Flag)) {
- variable = std::make_unique<VarDeclaration>(
- variableSymbol, type, /*sizes=*/ExpressionArray{}, (*initialValue)->clone());
+ variable = std::make_unique<VarDeclaration>(variableSymbol, type, /*arraySize=*/0,
+ (*initialValue)->clone());
} else {
- variable = std::make_unique<VarDeclaration>(
- variableSymbol, type, /*sizes=*/ExpressionArray{}, std::move(*initialValue));
+ variable = std::make_unique<VarDeclaration>(variableSymbol, type, /*arraySize=*/0,
+ std::move(*initialValue));
}
// Add the new variable-declaration statement to our block of extra statements.
diff --git a/src/sksl/SkSLMetalCodeGenerator.cpp b/src/sksl/SkSLMetalCodeGenerator.cpp
index 7d2725e..9752174 100644
--- a/src/sksl/SkSLMetalCodeGenerator.cpp
+++ b/src/sksl/SkSLMetalCodeGenerator.cpp
@@ -1345,12 +1345,12 @@
this->disallowArrayTypes(var.baseType()); // `float[2] x` shouldn't be possible (invalid SkSL)
this->write(" ");
this->writeName(var.var().name());
- for (const std::unique_ptr<Expression>& size : var.sizes()) {
+ if (var.arraySize() > 0) {
this->write("[");
- if (size) {
- this->writeExpression(*size, kTopLevel_Precedence);
- }
+ this->write(to_string(var.arraySize()));
this->write("]");
+ } else if (var.arraySize() == Type::kUnsizedArray){
+ this->write("[]");
}
if (var.value()) {
this->write(" = ");
diff --git a/src/sksl/SkSLRehydrator.cpp b/src/sksl/SkSLRehydrator.cpp
index 57fead0..da27ddf 100644
--- a/src/sksl/SkSLRehydrator.cpp
+++ b/src/sksl/SkSLRehydrator.cpp
@@ -441,18 +441,12 @@
case Rehydrator::kVarDeclaration_Command: {
Variable* var = this->symbolRef<Variable>(Symbol::Kind::kVariable);
const Type* baseType = this->type();
- uint8_t sizeCount = this->readU8();
- ExpressionArray sizes;
- sizes.reserve_back(sizeCount);
- for (int i = 0; i < sizeCount; ++i) {
- sizes.push_back(this->expression());
- }
+ int arraySize = this->readS8();
std::unique_ptr<Expression> value = this->expression();
if (value) {
var->setInitialValue(value.get());
}
- return std::make_unique<VarDeclaration>(var, baseType, std::move(sizes),
- std::move(value));
+ return std::make_unique<VarDeclaration>(var, baseType, arraySize, std::move(value));
}
case Rehydrator::kVoid_Command:
return nullptr;
diff --git a/src/sksl/generated/sksl_fp.dehydrated.sksl b/src/sksl/generated/sksl_fp.dehydrated.sksl
index 72bfe00..88379d9 100644
--- a/src/sksl/generated/sksl_fp.dehydrated.sksl
+++ b/src/sksl/generated/sksl_fp.dehydrated.sksl
@@ -352,14 +352,10 @@
50,
49,7,0,
42,6,0,1,
-28,
-42,6,0,1,0,0,0,
52,
50,
49,9,0,
42,4,0,1,
-28,
-42,6,0,1,0,0,0,
52,
50,
49,10,0,
diff --git a/src/sksl/generated/sksl_frag.dehydrated.sksl b/src/sksl/generated/sksl_frag.dehydrated.sksl
index 001913b..c477552 100644
--- a/src/sksl/generated/sksl_frag.dehydrated.sksl
+++ b/src/sksl/generated/sksl_frag.dehydrated.sksl
@@ -68,8 +68,6 @@
50,
49,7,0,
42,6,0,1,
-28,
-42,6,0,1,0,0,0,
52,
50,
49,8,0,
diff --git a/src/sksl/ir/SkSLVarDeclarations.h b/src/sksl/ir/SkSLVarDeclarations.h
index c82dd5f..aa83c06 100644
--- a/src/sksl/ir/SkSLVarDeclarations.h
+++ b/src/sksl/ir/SkSLVarDeclarations.h
@@ -26,12 +26,12 @@
VarDeclaration(const Variable* var,
const Type* baseType,
- ExpressionArray sizes,
+ int arraySize,
std::unique_ptr<Expression> value)
: INHERITED(var->fOffset, kStatementKind)
, fVar(var)
, fBaseType(*baseType)
- , fSizes(std::move(sizes))
+ , fArraySize(arraySize)
, fValue(std::move(value)) {}
const Type& baseType() const {
@@ -46,8 +46,8 @@
fVar = var;
}
- const ExpressionArray& sizes() const {
- return fSizes;
+ int arraySize() const {
+ return fArraySize;
}
std::unique_ptr<Expression>& value() {
@@ -59,30 +59,19 @@
}
std::unique_ptr<Statement> clone() const override {
- ExpressionArray sizesClone;
- sizesClone.reserve_back(this->sizes().count());
- for (const std::unique_ptr<Expression>& size : this->sizes()) {
- if (size) {
- sizesClone.push_back(size->clone());
- } else {
- sizesClone.push_back(nullptr);
- }
- }
return std::make_unique<VarDeclaration>(&this->var(),
&this->baseType(),
- std::move(sizesClone),
+ fArraySize,
this->value() ? this->value()->clone() : nullptr);
}
String description() const override {
String result = this->var().modifiers().description() + this->baseType().description() +
" " + this->var().name();
- for (const std::unique_ptr<Expression>& size : this->sizes()) {
- if (size) {
- result += "[" + size->description() + "]";
- } else {
- result += "[]";
- }
+ if (this->arraySize() > 0) {
+ result.appendf("[%d]", this->arraySize());
+ } else if (this->arraySize() == Type::kUnsizedArray){
+ result += "[]";
}
if (this->value()) {
result += " = " + this->value()->description();
@@ -94,7 +83,7 @@
private:
const Variable* fVar;
const Type& fBaseType;
- ExpressionArray fSizes;
+ int fArraySize; // zero means "not an array", Type::kUnsizedArray means var[]
std::unique_ptr<Expression> fValue;
using INHERITED = Statement;