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>

9 files changed, 81 insertions, 121 deletions

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