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>

8 files changed, 87 insertions, 62 deletions

diff --git a/src/sksl/SkSLCFGGenerator.cpp b/src/sksl/SkSLCFGGenerator.cpp
index 71bd37f39a..2fe049d9bd 100644
--- a/src/sksl/SkSLCFGGenerator.cpp
+++ b/src/sksl/SkSLCFGGenerator.cpp
@@ -465,13 +465,17 @@ void CFGGenerator::addStatement(CFG& cfg, std::unique_ptr<Statement>* s) {
         }
         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 f63ef977be..dd20b5c257 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -262,12 +262,10 @@ void Compiler::addDefinitions(const BasicBlock::Node& node,
         }
         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 @@ static DefinitionMap compute_start_state(const CFG& cfg) {
                 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 @@ void Compiler::simplifyStatement(DefinitionMap& definitions,
                                  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 d67f7182f8..c3168e3ed3 100644
--- a/src/sksl/SkSLGLSLCodeGenerator.cpp
+++ b/src/sksl/SkSLGLSLCodeGenerator.cpp
@@ -619,26 +619,33 @@ void GLSLCodeGenerator::writeInterfaceBlock(const InterfaceBlock& intf) {
 
 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 @@ void GLSLCodeGenerator::writeVarDeclarations(const VarDeclarations& decl, bool g
             fFoundImageDecl = true;
         }
     }
-    this->write(";");
+    if (wroteType) {
+        this->write(";");
+    }
 }
 
 void GLSLCodeGenerator::writeStatement(const Statement& s) {
@@ -829,7 +838,8 @@ bool GLSLCodeGenerator::generateCode() {
             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 2e280d8702..f85ea1054d 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -614,19 +614,20 @@ std::unique_ptr<InterfaceBlock> IRGenerator::convertInterfaceBlock(const ASTInte
         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 @@ std::unique_ptr<InterfaceBlock> IRGenerator::convertInterfaceBlock(const ASTInte
                 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 9cc25b90fe..20d292fd67 100644
--- a/src/sksl/SkSLSPIRVCodeGenerator.cpp
+++ b/src/sksl/SkSLSPIRVCodeGenerator.cpp
@@ -2603,7 +2603,7 @@ SpvId SPIRVCodeGenerator::writeInterfaceBlock(const InterfaceBlock& intf) {
 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];
+        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 +2665,9 @@ void SPIRVCodeGenerator::writeGlobalVars(Program::Kind kind, const VarDeclaratio
 }
 
 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) {
+        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 +2680,8 @@ void SPIRVCodeGenerator::writeVarDeclarations(const VarDeclarations& decl, Outpu
         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 9af18eca3e..1bc524451b 100644
--- a/src/sksl/ir/SkSLStatement.h
+++ b/src/sksl/ir/SkSLStatement.h
@@ -30,6 +30,7 @@ struct Statement : public IRNode {
         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 dbf2928d49..c07fee87db 100644
--- a/src/sksl/ir/SkSLVarDeclarations.h
+++ b/src/sksl/ir/SkSLVarDeclarations.h
@@ -20,11 +20,12 @@ namespace SkSL {
  * '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 @@ struct VarDeclaration {
     const Variable* fVar;
     std::vector<std::unique_ptr<Expression>> fSizes;
     std::unique_ptr<Expression> fValue;
+
+    typedef Statement INHERITED;
 };
 
 /**
@@ -55,14 +58,18 @@ struct VarDeclarations : public ProgramElement {
     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 @@ struct VarDeclarations : public ProgramElement {
     }
 
     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 50365decc1..ab6753610f 100644
--- a/src/sksl/ir/SkSLVarDeclarationsStatement.h
+++ b/src/sksl/ir/SkSLVarDeclarationsStatement.h
@@ -21,6 +21,15 @@ struct VarDeclarationsStatement : public Statement {
     : 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();
     }