Simplify InterfaceBlock by removing multi-dimensional array support.

johnstiles-google · Skia Commit-Bot · commit d39aec0e40ec · 2020-12-03T16:37:49.000Z
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: Id58404c5c8d48786e02585d2a6391b2f3e5393e8 Bug: skia:11026 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/340456 Reviewed-by: Brian Osman <brianosman@google.com> Commit-Queue: John Stiles <johnstiles@google.com> Auto-Submit: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/SkSLAnalysis.cpp b/src/sksl/SkSLAnalysis.cpp
@@ -629,6 +629,7 @@ bool TProgramVisitor<PROG, EXPR, STMT, ELEM>::visitProgramElement(ELEM pe) {
         case ProgramElement::Kind::kEnum:
         case ProgramElement::Kind::kExtension:
         case ProgramElement::Kind::kFunctionPrototype:
+        case ProgramElement::Kind::kInterfaceBlock:
         case ProgramElement::Kind::kModifiers:
         case ProgramElement::Kind::kSection:
         case ProgramElement::Kind::kStructDefinition:
@@ -638,14 +639,6 @@ bool TProgramVisitor<PROG, EXPR, STMT, ELEM>::visitProgramElement(ELEM pe) {
         case ProgramElement::Kind::kFunction:
             return this->visitStatement(*pe.template as<FunctionDefinition>().body());
 
-        case ProgramElement::Kind::kInterfaceBlock:
-            for (auto& e : pe.template as<InterfaceBlock>().sizes()) {
-                if (e && this->visitExpression(*e)) {
-                    return true;
-                }
-            }
-            return false;
-
         case ProgramElement::Kind::kGlobalVar:
             if (this->visitStatement(*pe.template as<GlobalVarDeclaration>().declaration())) {
                 return true;
diff --git a/src/sksl/SkSLDehydrator.cpp b/src/sksl/SkSLDehydrator.cpp
@@ -553,10 +553,7 @@ void Dehydrator::write(const ProgramElement& e) {
             this->write(i.variable());
             this->write(i.typeName());
             this->write(i.instanceName());
-            this->writeU8(i.sizes().count());
-            for (const auto& s : i.sizes()) {
-                this->write(s.get());
-            }
+            this->writeS8(i.arraySize());
             break;
         }
         case ProgramElement::Kind::kModifiers:
diff --git a/src/sksl/SkSLGLSLCodeGenerator.cpp b/src/sksl/SkSLGLSLCodeGenerator.cpp
@@ -1218,12 +1218,12 @@ void GLSLCodeGenerator::writeInterfaceBlock(const InterfaceBlock& intf) {
     if (intf.instanceName().size()) {
         this->write(" ");
         this->write(intf.instanceName());
-        for (const auto& size : intf.sizes()) {
+        if (intf.arraySize() > 0) {
             this->write("[");
-            if (size) {
-                this->writeExpression(*size, kTopLevel_Precedence);
-            }
+            this->write(to_string(intf.arraySize()));
             this->write("]");
+        } else if (intf.arraySize() == Type::kUnsizedArray){
+            this->write("[]");
         }
     }
     this->writeLine(";");
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
@@ -1146,39 +1146,30 @@ std::unique_ptr<InterfaceBlock> IRGenerator::convertInterfaceBlock(const ASTNode
             }
         }
     }
-    const Type* type =
-            old->takeOwnershipOfSymbol(std::make_unique<Type>(intf.fOffset, id.fTypeName, fields));
-    ExpressionArray sizes;
-    sizes.reserve_back(id.fSizeCount);
-    for (size_t i = 0; i < id.fSizeCount; ++i) {
+    const Type* type = old->takeOwnershipOfSymbol(std::make_unique<Type>(intf.fOffset, id.fTypeName,
+                                                                         fields));
+    int arraySize = 0;
+    if (id.fSizeCount > 0) {
+        SkASSERT(id.fSizeCount == 1); // multi-dimensional arrays are not supported
         const ASTNode& size = *(iter++);
         if (size) {
             std::unique_ptr<Expression> converted = this->convertExpression(size);
             if (!converted) {
                 return nullptr;
             }
-            String name = type->name();
-            int64_t count;
-            if (converted->kind() == Expression::Kind::kIntLiteral) {
-                count = converted->as<IntLiteral>().value();
-                if (count <= 0) {
-                    fErrors.error(converted->fOffset, "array size must be positive");
-                    return nullptr;
-                }
-                name += "[" + to_string(count) + "]";
-            } else {
+            if (!converted->is<IntLiteral>()) {
                 fErrors.error(intf.fOffset, "array size must be specified");
                 return nullptr;
             }
-            type = symbols->takeOwnershipOfSymbol(
-                    std::make_unique<Type>(name, Type::TypeKind::kArray, *type, (int)count));
-            sizes.push_back(std::move(converted));
+            arraySize = converted->as<IntLiteral>().value();
+            if (arraySize <= 0) {
+                fErrors.error(converted->fOffset, "array size must be positive");
+                return nullptr;
+            }
         } else {
-            String name = String(type->name()) + "[]";
-            type = symbols->takeOwnershipOfSymbol(std::make_unique<Type>(
-                    name, Type::TypeKind::kArray, *type, Type::kUnsizedArray));
-            sizes.push_back(nullptr);
+            arraySize = Type::kUnsizedArray;
         }
+        type = symbols->addArrayDimensions(type, {arraySize});
     }
     const Variable* var = old->takeOwnershipOfSymbol(
             std::make_unique<Variable>(intf.fOffset,
@@ -1201,7 +1192,7 @@ std::unique_ptr<InterfaceBlock> IRGenerator::convertInterfaceBlock(const ASTNode
                                             var,
                                             id.fTypeName,
                                             id.fInstanceName,
-                                            std::move(sizes),
+                                            arraySize,
                                             symbols);
 }
 
diff --git a/src/sksl/SkSLMetalCodeGenerator.cpp b/src/sksl/SkSLMetalCodeGenerator.cpp
@@ -1260,12 +1260,12 @@ void MetalCodeGenerator::writeInterfaceBlock(const InterfaceBlock& intf) {
     if (intf.instanceName().size()) {
         this->write(" ");
         this->write(intf.instanceName());
-        for (const auto& size : intf.sizes()) {
+        if (intf.arraySize() > 0) {
             this->write("[");
-            if (size) {
-                this->writeExpression(*size, kTopLevel_Precedence);
-            }
+            this->write(to_string(intf.arraySize()));
             this->write("]");
+        } else if (intf.arraySize() == Type::kUnsizedArray){
+            this->write("[]");
         }
         fInterfaceBlockNameMap[&intf] = intf.instanceName();
     } else {
diff --git a/src/sksl/SkSLRehydrator.cpp b/src/sksl/SkSLRehydrator.cpp
@@ -327,14 +327,9 @@ std::unique_ptr<ProgramElement> Rehydrator::element() {
             SkASSERT(var && var->is<Variable>());
             StringFragment typeName = this->readString();
             StringFragment instanceName = this->readString();
-            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();
             return std::make_unique<InterfaceBlock>(/*offset=*/-1, &var->as<Variable>(), typeName,
-                                                    instanceName, std::move(sizes), nullptr);
+                                                    instanceName, arraySize, nullptr);
         }
         case Rehydrator::kVarDeclarations_Command: {
             std::unique_ptr<Statement> decl = this->statement();
diff --git a/src/sksl/SkSLSPIRVCodeGenerator.cpp b/src/sksl/SkSLSPIRVCodeGenerator.cpp
@@ -1907,8 +1907,8 @@ SpvId SPIRVCodeGenerator::writeVariableReference(const VariableReference& ref, O
                                                    &intfStruct,
                                                    /*builtin=*/false,
                                                    Variable::Storage::kGlobal));
-                InterfaceBlock intf(/*offset=*/-1, intfVar, name, /*instanceName=*/"",
-                                    /*sizes=*/ExpressionArray(),
+                InterfaceBlock intf(/*offset=*/-1, intfVar, name,
+                                    /*instanceName=*/"", /*arraySize=*/0,
                                     std::make_shared<SymbolTable>(&fErrors, /*builtin=*/false));
 
                 fRTHeightStructId = this->writeInterfaceBlock(intf, false);
diff --git a/src/sksl/generated/sksl_geom.dehydrated.sksl b/src/sksl/generated/sksl_geom.dehydrated.sksl
@@ -70,8 +70,7 @@ static uint8_t SKSL_INCLUDE_sksl_geom[] = {134,0,
 1,0,
 12,
 27,
-42,4,0,2,0,53,0,1,
-52,
+42,4,0,2,0,53,0,255,
 27,
 42,7,0,2,0,135,0,0,
 13,};
diff --git a/src/sksl/ir/SkSLInterfaceBlock.h b/src/sksl/ir/SkSLInterfaceBlock.h
@@ -31,12 +31,12 @@ class InterfaceBlock final : public ProgramElement {
     static constexpr Kind kProgramElementKind = Kind::kInterfaceBlock;
 
     InterfaceBlock(int offset, const Variable* var, String typeName, String instanceName,
-                   ExpressionArray sizes, std::shared_ptr<SymbolTable> typeOwner)
+                   int arraySize, std::shared_ptr<SymbolTable> typeOwner)
     : INHERITED(offset, kProgramElementKind)
     , fVariable(var)
     , fTypeName(std::move(typeName))
     , fInstanceName(std::move(instanceName))
-    , fSizes(std::move(sizes))
+    , fArraySize(arraySize)
     , fTypeOwner(std::move(typeOwner)) {}
 
     const Variable& variable() const {
@@ -59,22 +59,13 @@ class InterfaceBlock final : public ProgramElement {
         return fTypeOwner;
     }
 
-    ExpressionArray& sizes() {
-        return fSizes;
-    }
-
-    const ExpressionArray& sizes() const {
-        return fSizes;
+    int arraySize() const {
+        return fArraySize;
     }
 
     std::unique_ptr<ProgramElement> clone() const override {
-        ExpressionArray sizesClone;
-        sizesClone.reserve_back(this->sizes().size());
-        for (const auto& size : this->sizes()) {
-            sizesClone.push_back(size ? size->clone() : nullptr);
-        }
         return std::make_unique<InterfaceBlock>(fOffset, &this->variable(), this->typeName(),
-                                                this->instanceName(), std::move(sizesClone),
+                                                this->instanceName(), this->arraySize(),
                                                 SymbolTable::WrapIfBuiltin(this->typeOwner()));
     }
 
@@ -88,14 +79,12 @@ class InterfaceBlock final : public ProgramElement {
             result += f.description() + "\n";
         }
         result += "}";
-        if (this->instanceName().size()) {
+        if (!this->instanceName().empty()) {
             result += " " + this->instanceName();
-            for (const auto& size : this->sizes()) {
-                result += "[";
-                if (size) {
-                    result += size->description();
-                }
-                result += "]";
+            if (this->arraySize() > 0) {
+                result.appendf("[%d]", this->arraySize());
+            } else if (this->arraySize() == Type::kUnsizedArray){
+                result += "[]";
             }
         }
         return result + ";";
@@ -105,7 +94,7 @@ class InterfaceBlock final : public ProgramElement {
     const Variable* fVariable;
     String fTypeName;
     String fInstanceName;
-    ExpressionArray fSizes;
+    int fArraySize;
     std::shared_ptr<SymbolTable> fTypeOwner;
 
     using INHERITED = ProgramElement;
