added frexp support to SkSL

This includes an optimizer fix for the situation:

int i;
float f = frexp(foo, i);

If we don't read the variable i, it is considered dead and eliminated -
which then causes an error when we try to write the expression
frexmp(foo, i).

Bug: skia:
Change-Id: Iac385e38e215455346fab62e1f4ec46fa65b3c21
Reviewed-on: https://skia-review.googlesource.com/116521
Reviewed-by: Chris Dalton <csmartdalton@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>

6 files changed, 161 insertions, 118 deletions

diff --git a/src/sksl/SkSLCompiler.cpp b/src/sksl/SkSLCompiler.cpp
index 06ab71f922..bbaaf407d2 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -660,8 +660,11 @@ void Compiler::simplifyExpression(DefinitionMap& definitions,
     }
     switch (expr->fKind) {
         case Expression::kVariableReference_Kind: {
-            const Variable& var = ((VariableReference*) expr)->fVariable;
-            if (var.fStorage == Variable::kLocal_Storage && !definitions[&var] &&
+            const VariableReference& ref = (VariableReference&) *expr;
+            const Variable& var = ref.fVariable;
+            if (ref.refKind() != VariableReference::kWrite_RefKind &&
+                ref.refKind() != VariableReference::kPointer_RefKind &&
+                var.fStorage == Variable::kLocal_Storage && !definitions[&var] &&
                 (*undefinedVariables).find(&var) == (*undefinedVariables).end()) {
                 (*undefinedVariables).insert(&var);
                 this->error(expr->fOffset,
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index c6d09de7d5..68d9f9c0ca 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -1403,7 +1403,9 @@ std::unique_ptr<Expression> IRGenerator::convertBinaryExpression(
         return nullptr;
     }
     if (Compiler::IsAssignment(expression.fOperator)) {
-        this->markWrittenTo(*left, expression.fOperator != Token::EQ);
+        this->setRefKind(*left, expression.fOperator != Token::EQ ?
+                                                             VariableReference::kReadWrite_RefKind :
+                                                             VariableReference::kWrite_RefKind);
     }
     left = this->coerce(std::move(left), *leftType);
     right = this->coerce(std::move(right), *rightType);
@@ -1534,8 +1536,10 @@ std::unique_ptr<Expression> IRGenerator::call(int offset,
             return nullptr;
         }
         if (arguments[i] && (function.fParameters[i]->fModifiers.fFlags & Modifiers::kOut_Flag)) {
-            this->markWrittenTo(*arguments[i],
-                                function.fParameters[i]->fModifiers.fFlags & Modifiers::kIn_Flag);
+            this->setRefKind(*arguments[i],
+                             function.fParameters[i]->fModifiers.fFlags & Modifiers::kIn_Flag ?
+                             VariableReference::kReadWrite_RefKind :
+                             VariableReference::kPointer_RefKind);
         }
     }
     if (function.fBuiltin && function.fName == "texture" &&
@@ -1786,7 +1790,7 @@ std::unique_ptr<Expression> IRGenerator::convertPrefixExpression(
                               "' cannot operate on '" + base->fType.description() + "'");
                 return nullptr;
             }
-            this->markWrittenTo(*base, true);
+            this->setRefKind(*base, VariableReference::kReadWrite_RefKind);
             break;
         case Token::MINUSMINUS:
             if (!base->fType.isNumber()) {
@@ -1795,7 +1799,7 @@ std::unique_ptr<Expression> IRGenerator::convertPrefixExpression(
                               "' cannot operate on '" + base->fType.description() + "'");
                 return nullptr;
             }
-            this->markWrittenTo(*base, true);
+            this->setRefKind(*base, VariableReference::kReadWrite_RefKind);
             break;
         case Token::LOGICALNOT:
             if (base->fType != *fContext.fBool_Type) {
@@ -2032,7 +2036,7 @@ std::unique_ptr<Expression> IRGenerator::convertSuffixExpression(
                               "'++' cannot operate on '" + base->fType.description() + "'");
                 return nullptr;
             }
-            this->markWrittenTo(*base, true);
+            this->setRefKind(*base, VariableReference::kReadWrite_RefKind);
             return std::unique_ptr<Expression>(new PostfixExpression(std::move(base),
                                                                      Token::PLUSPLUS));
         case ASTSuffix::kPostDecrement_Kind:
@@ -2041,7 +2045,7 @@ std::unique_ptr<Expression> IRGenerator::convertSuffixExpression(
                               "'--' cannot operate on '" + base->fType.description() + "'");
                 return nullptr;
             }
-            this->markWrittenTo(*base, true);
+            this->setRefKind(*base, VariableReference::kReadWrite_RefKind);
             return std::unique_ptr<Expression>(new PostfixExpression(std::move(base),
                                                                      Token::MINUSMINUS));
         default:
@@ -2077,7 +2081,7 @@ static bool has_duplicates(const Swizzle& swizzle) {
     return false;
 }
 
-void IRGenerator::markWrittenTo(const Expression& expr, bool readWrite) {
+void IRGenerator::setRefKind(const Expression& expr, VariableReference::RefKind kind) {
     switch (expr.fKind) {
         case Expression::kVariableReference_Kind: {
             const Variable& var = ((VariableReference&) expr).fVariable;
@@ -2085,27 +2089,26 @@ void IRGenerator::markWrittenTo(const Expression& expr, bool readWrite) {
                 fErrors.error(expr.fOffset,
                               "cannot modify immutable variable '" + var.fName + "'");
             }
-            ((VariableReference&) expr).setRefKind(readWrite ? VariableReference::kReadWrite_RefKind
-                                                             : VariableReference::kWrite_RefKind);
+            ((VariableReference&) expr).setRefKind(kind);
             break;
         }
         case Expression::kFieldAccess_Kind:
-            this->markWrittenTo(*((FieldAccess&) expr).fBase, readWrite);
+            this->setRefKind(*((FieldAccess&) expr).fBase, kind);
             break;
         case Expression::kSwizzle_Kind:
             if (has_duplicates((Swizzle&) expr)) {
                 fErrors.error(expr.fOffset,
                               "cannot write to the same swizzle field more than once");
             }
-            this->markWrittenTo(*((Swizzle&) expr).fBase, readWrite);
+            this->setRefKind(*((Swizzle&) expr).fBase, kind);
             break;
         case Expression::kIndex_Kind:
-            this->markWrittenTo(*((IndexExpression&) expr).fBase, readWrite);
+            this->setRefKind(*((IndexExpression&) expr).fBase, kind);
             break;
         case Expression::kTernary_Kind: {
             TernaryExpression& t = (TernaryExpression&) expr;
-            this->markWrittenTo(*t.fIfTrue, readWrite);
-            this->markWrittenTo(*t.fIfFalse, readWrite);
+            this->setRefKind(*t.fIfTrue, kind);
+            this->setRefKind(*t.fIfFalse, kind);
             break;
         }
         default:
diff --git a/src/sksl/SkSLIRGenerator.h b/src/sksl/SkSLIRGenerator.h
index 327fe6fc7a..e8ff1d21c8 100644
--- a/src/sksl/SkSLIRGenerator.h
+++ b/src/sksl/SkSLIRGenerator.h
@@ -50,6 +50,7 @@
 #include "ir/SkSLType.h"
 #include "ir/SkSLTypeReference.h"
 #include "ir/SkSLVarDeclarations.h"
+#include "ir/SkSLVariableReference.h"
 
 namespace SkSL {
 
@@ -162,7 +163,7 @@ private:
 
     void fixRectSampling(std::vector<std::unique_ptr<Expression>>& arguments);
     void checkValid(const Expression& expr);
-    void markWrittenTo(const Expression& expr, bool readWrite);
+    void setRefKind(const Expression& expr, VariableReference::RefKind kind);
     void getConstantInt(const Expression& value, int64_t* out);
 
     Program::Kind fKind;
diff --git a/src/sksl/SkSLSPIRVCodeGenerator.cpp b/src/sksl/SkSLSPIRVCodeGenerator.cpp
index 7d3726a8c1..dadab91753 100644
--- a/src/sksl/SkSLSPIRVCodeGenerator.cpp
+++ b/src/sksl/SkSLSPIRVCodeGenerator.cpp
@@ -684,7 +684,11 @@ SpvId SPIRVCodeGenerator::writeIntrinsicCall(const FunctionCall& c, OutputStream
             SpvId result = this->nextId();
             std::vector<SpvId> arguments;
             for (size_t i = 0; i < c.fArguments.size(); i++) {
-                arguments.push_back(this->writeExpression(*c.fArguments[i], out));
+                if (c.fFunction.fParameters[i]->fModifiers.fFlags & Modifiers::kOut_Flag) {
+                    arguments.push_back(this->getLValue(*c.fArguments[i], out)->getPointer());
+                } else {
+                    arguments.push_back(this->writeExpression(*c.fArguments[i], out));
+                }
             }
             this->writeOpCode(SpvOpExtInst, 5 + (int32_t) arguments.size(), out);
             this->writeWord(this->getType(c.fType), out);
@@ -700,7 +704,11 @@ SpvId SPIRVCodeGenerator::writeIntrinsicCall(const FunctionCall& c, OutputStream
             SpvId result = this->nextId();
             std::vector<SpvId> arguments;
             for (size_t i = 0; i < c.fArguments.size(); i++) {
-                arguments.push_back(this->writeExpression(*c.fArguments[i], out));
+                if (c.fFunction.fParameters[i]->fModifiers.fFlags & Modifiers::kOut_Flag) {
+                    arguments.push_back(this->getLValue(*c.fArguments[i], out)->getPointer());
+                } else {
+                    arguments.push_back(this->writeExpression(*c.fArguments[i], out));
+                }
             }
             if (c.fType != *fContext.fVoid_Type) {
                 this->writeOpCode((SpvOp_) intrinsicId, 3 + (int32_t) arguments.size(), out);
@@ -1574,9 +1582,9 @@ std::unique_ptr<SPIRVCodeGenerator::LValue> SPIRVCodeGenerator::getLValue(const
             auto entry = fVariableMap.find(&var);
             ASSERT(entry != fVariableMap.end());
             return std::unique_ptr<SPIRVCodeGenerator::LValue>(new PointerLValue(
-                                                                       *this,
-                                                                       entry->second,
-                                                                       this->getType(expr.fType)));
+                                                                        *this,
+                                                                        entry->second,
+                                                                        this->getType(expr.fType)));
         }
         case Expression::kIndex_Kind: // fall through
         case Expression::kFieldAccess_Kind: {
@@ -1589,9 +1597,9 @@ std::unique_ptr<SPIRVCodeGenerator::LValue> SPIRVCodeGenerator::getLValue(const
                 this->writeWord(idx, out);
             }
             return std::unique_ptr<SPIRVCodeGenerator::LValue>(new PointerLValue(
-                                                                       *this,
-                                                                       member,
-                                                                       this->getType(expr.fType)));
+                                                                        *this,
+                                                                        member,
+                                                                        this->getType(expr.fType)));
         }
         case Expression::kSwizzle_Kind: {
             Swizzle& swizzle = (Swizzle&) expr;
diff --git a/src/sksl/ir/SkSLVariableReference.cpp b/src/sksl/ir/SkSLVariableReference.cpp
new file mode 100644
index 0000000000..37e0ca2e7d
--- /dev/null
+++ b/src/sksl/ir/SkSLVariableReference.cpp
@@ -0,0 +1,108 @@
+/*
+ * Copyright 2018 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkSLVariableReference.h"
+
+#include "SkSLConstructor.h"
+#include "SkSLFloatLiteral.h"
+#include "SkSLIRGenerator.h"
+#include "SkSLSetting.h"
+
+namespace SkSL {
+
+VariableReference::VariableReference(int offset, const Variable& variable, RefKind refKind)
+: INHERITED(offset, kVariableReference_Kind, variable.fType)
+, fVariable(variable)
+, fRefKind(refKind) {
+    if (refKind != kRead_RefKind) {
+        fVariable.fWriteCount++;
+    }
+    if (refKind != kWrite_RefKind) {
+        fVariable.fReadCount++;
+    }
+}
+
+VariableReference::~VariableReference() {
+    if (fRefKind != kRead_RefKind) {
+        fVariable.fWriteCount--;
+    }
+    if (fRefKind != kWrite_RefKind) {
+        fVariable.fReadCount--;
+    }
+}
+
+void VariableReference::setRefKind(RefKind refKind) {
+    if (fRefKind != kRead_RefKind) {
+        fVariable.fWriteCount--;
+    }
+    if (fRefKind != kWrite_RefKind) {
+        fVariable.fReadCount--;
+    }
+    if (refKind != kRead_RefKind) {
+        fVariable.fWriteCount++;
+    }
+    if (refKind != kWrite_RefKind) {
+        fVariable.fReadCount++;
+    }
+    fRefKind = refKind;
+}
+
+std::unique_ptr<Expression> VariableReference::copy_constant(const IRGenerator& irGenerator,
+                                                             const Expression* expr) {
+    ASSERT(expr->isConstant());
+    switch (expr->fKind) {
+        case Expression::kIntLiteral_Kind:
+            return std::unique_ptr<Expression>(new IntLiteral(irGenerator.fContext,
+                                                              -1,
+                                                              ((IntLiteral*) expr)->fValue));
+        case Expression::kFloatLiteral_Kind:
+            return std::unique_ptr<Expression>(new FloatLiteral(
+                                                               irGenerator.fContext,
+                                                               -1,
+                                                               ((FloatLiteral*) expr)->fValue));
+        case Expression::kBoolLiteral_Kind:
+            return std::unique_ptr<Expression>(new BoolLiteral(irGenerator.fContext,
+                                                               -1,
+                                                               ((BoolLiteral*) expr)->fValue));
+        case Expression::kConstructor_Kind: {
+            const Constructor* c = (const Constructor*) expr;
+            std::vector<std::unique_ptr<Expression>> args;
+            for (const auto& arg : c->fArguments) {
+                args.push_back(copy_constant(irGenerator, arg.get()));
+            }
+            return std::unique_ptr<Expression>(new Constructor(-1, c->fType,
+                                                               std::move(args)));
+        }
+        case Expression::kSetting_Kind: {
+            const Setting* s = (const Setting*) expr;
+            return std::unique_ptr<Expression>(new Setting(-1, s->fName,
+                                                           copy_constant(irGenerator,
+                                                                         s->fValue.get())));
+        }
+        default:
+            ABORT("unsupported constant\n");
+    }
+}
+
+std::unique_ptr<Expression> VariableReference::constantPropagate(const IRGenerator& irGenerator,
+                                                                 const DefinitionMap& definitions) {
+    if (fRefKind != kRead_RefKind) {
+        return nullptr;
+    }
+    if ((fVariable.fModifiers.fFlags & Modifiers::kConst_Flag) && fVariable.fInitialValue &&
+        fVariable.fInitialValue->isConstant()) {
+        return copy_constant(irGenerator, fVariable.fInitialValue);
+    }
+    auto exprIter = definitions.find(&fVariable);
+    if (exprIter != definitions.end() && exprIter->second &&
+        (*exprIter->second)->isConstant()) {
+        return copy_constant(irGenerator, exprIter->second->get());
+    }
+    return nullptr;
+}
+
+} // namespace
diff --git a/src/sksl/ir/SkSLVariableReference.h b/src/sksl/ir/SkSLVariableReference.h
index e1f19ac742..14ddf796ff 100644
--- a/src/sksl/ir/SkSLVariableReference.h
+++ b/src/sksl/ir/SkSLVariableReference.h
@@ -8,16 +8,12 @@
 #ifndef SKSL_VARIABLEREFERENCE
 #define SKSL_VARIABLEREFERENCE
 
-#include "SkSLBoolLiteral.h"
-#include "SkSLConstructor.h"
 #include "SkSLExpression.h"
-#include "SkSLFloatLiteral.h"
-#include "SkSLIRGenerator.h"
-#include "SkSLIntLiteral.h"
-#include "SkSLSetting.h"
 
 namespace SkSL {
 
+class IRGenerator;
+
 /**
  * A reference to a variable, through which it can be read or written. In the statement:
  *
@@ -29,49 +25,21 @@ struct VariableReference : public Expression {
     enum RefKind {
         kRead_RefKind,
         kWrite_RefKind,
-        kReadWrite_RefKind
+        kReadWrite_RefKind,
+        // taking the address of a variable - we consider this a read & write but don't complain if
+        // the variable was not previously assigned
+        kPointer_RefKind
     };
 
-    VariableReference(int offset, const Variable& variable, RefKind refKind = kRead_RefKind)
-    : INHERITED(offset, kVariableReference_Kind, variable.fType)
-    , fVariable(variable)
-    , fRefKind(refKind) {
-        if (refKind != kRead_RefKind) {
-            fVariable.fWriteCount++;
-        }
-        if (refKind != kWrite_RefKind) {
-            fVariable.fReadCount++;
-        }
-    }
+    VariableReference(int offset, const Variable& variable, RefKind refKind = kRead_RefKind);
 
-    ~VariableReference() override {
-        if (fRefKind != kRead_RefKind) {
-            fVariable.fWriteCount--;
-        }
-        if (fRefKind != kWrite_RefKind) {
-            fVariable.fReadCount--;
-        }
-    }
+    ~VariableReference() override;
 
-    RefKind refKind() {
+    RefKind refKind() const {
         return fRefKind;
     }
 
-    void setRefKind(RefKind refKind) {
-        if (fRefKind != kRead_RefKind) {
-            fVariable.fWriteCount--;
-        }
-        if (fRefKind != kWrite_RefKind) {
-            fVariable.fReadCount--;
-        }
-        if (refKind != kRead_RefKind) {
-            fVariable.fWriteCount++;
-        }
-        if (refKind != kWrite_RefKind) {
-            fVariable.fReadCount++;
-        }
-        fRefKind = refKind;
-    }
+    void setRefKind(RefKind refKind);
 
     bool hasSideEffects() const override {
         return false;
@@ -86,58 +54,10 @@ struct VariableReference : public Expression {
     }
 
     static std::unique_ptr<Expression> copy_constant(const IRGenerator& irGenerator,
-                                                     const Expression* expr) {
-        ASSERT(expr->isConstant());
-        switch (expr->fKind) {
-            case Expression::kIntLiteral_Kind:
-                return std::unique_ptr<Expression>(new IntLiteral(irGenerator.fContext,
-                                                                  -1,
-                                                                  ((IntLiteral*) expr)->fValue));
-            case Expression::kFloatLiteral_Kind:
-                return std::unique_ptr<Expression>(new FloatLiteral(
-                                                                   irGenerator.fContext,
-                                                                   -1,
-                                                                   ((FloatLiteral*) expr)->fValue));
-            case Expression::kBoolLiteral_Kind:
-                return std::unique_ptr<Expression>(new BoolLiteral(irGenerator.fContext,
-                                                                   -1,
-                                                                   ((BoolLiteral*) expr)->fValue));
-            case Expression::kConstructor_Kind: {
-                const Constructor* c = (const Constructor*) expr;
-                std::vector<std::unique_ptr<Expression>> args;
-                for (const auto& arg : c->fArguments) {
-                    args.push_back(copy_constant(irGenerator, arg.get()));
-                }
-                return std::unique_ptr<Expression>(new Constructor(-1, c->fType,
-                                                                   std::move(args)));
-            }
-            case Expression::kSetting_Kind: {
-                const Setting* s = (const Setting*) expr;
-                return std::unique_ptr<Expression>(new Setting(-1, s->fName,
-                                                               copy_constant(irGenerator,
-                                                                             s->fValue.get())));
-            }
-            default:
-                ABORT("unsupported constant\n");
-        }
-    }
+                                                     const Expression* expr);
 
     std::unique_ptr<Expression> constantPropagate(const IRGenerator& irGenerator,
-                                                  const DefinitionMap& definitions) override {
-        if (fRefKind != kRead_RefKind) {
-            return nullptr;
-        }
-        if ((fVariable.fModifiers.fFlags & Modifiers::kConst_Flag) && fVariable.fInitialValue &&
-            fVariable.fInitialValue->isConstant()) {
-            return copy_constant(irGenerator, fVariable.fInitialValue);
-        }
-        auto exprIter = definitions.find(&fVariable);
-        if (exprIter != definitions.end() && exprIter->second &&
-            (*exprIter->second)->isConstant()) {
-            return copy_constant(irGenerator, exprIter->second->get());
-        }
-        return nullptr;
-    }
+                                                  const DefinitionMap& definitions) override;
 
     const Variable& fVariable;
     RefKind fRefKind;