initial checkin of SkSL compiler

src/sksl/SkSLIRGenerator.cpp

Index: src/sksl/SkSLIRGenerator.cpp
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..4824d64ed813411c0a47db2edbb59d89b6b76c7b
--- /dev/null
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -0,0 +1,1100 @@
+/*
+ * Copyright 2016 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+ 
+#include "SkSLIRGenerator.h"
+
+#include "limits.h"
+
+#include "ast/SkSLASTBoolLiteral.h"
+#include "ast/SkSLASTFieldSuffix.h"
+#include "ast/SkSLASTFloatLiteral.h"
+#include "ast/SkSLASTIndexSuffix.h"
+#include "ast/SkSLASTIntLiteral.h"
+#include "ir/SkSLBinaryExpression.h"
+#include "ir/SkSLBoolLiteral.h"
+#include "ir/SkSLBreakStatement.h"
+#include "ir/SkSLConstructor.h"
+#include "ir/SkSLContinueStatement.h"
+#include "ir/SkSLDiscardStatement.h"
+#include "ir/SkSLDoStatement.h"
+#include "ir/SkSLExpressionStatement.h"
+#include "ir/SkSLField.h"
+#include "ir/SkSLFieldAccess.h"
+#include "ir/SkSLFloatLiteral.h"
+#include "ir/SkSLForStatement.h"
+#include "ir/SkSLFunctionCall.h"
+#include "ir/SkSLFunctionDeclaration.h"
+#include "ir/SkSLFunctionDefinition.h"
+#include "ir/SkSLFunctionReference.h"
+#include "ir/SkSLIfStatement.h"
+#include "ir/SkSLIndexExpression.h"
+#include "ir/SkSLInterfaceBlock.h"
+#include "ir/SkSLIntLiteral.h"
+#include "ir/SkSLLayout.h"
+#include "ir/SkSLPostfixExpression.h"
+#include "ir/SkSLPrefixExpression.h"
+#include "ir/SkSLReturnStatement.h"
+#include "ir/SkSLSwizzle.h"
+#include "ir/SkSLTernaryExpression.h"
+#include "ir/SkSLUnresolvedFunction.h"
+#include "ir/SkSLVariable.h"
+#include "ir/SkSLVarDeclaration.h"
+#include "ir/SkSLVarDeclarationStatement.h"
+#include "ir/SkSLVariableReference.h"
+#include "ir/SkSLWhileStatement.h"
+
+namespace SkSL {
+
+class AutoSymbolTable {
+public:
+    AutoSymbolTable(IRGenerator* ir) 
+    : fIR(ir)
+    , fPrevious(fIR->fSymbolTable) {
+        fIR->pushSymbolTable();
+    }
+
+    ~AutoSymbolTable() {
+        fIR->popSymbolTable();
+        ASSERT(fPrevious == fIR->fSymbolTable);
+    }
+
+    IRGenerator* fIR;
+    std::shared_ptr<SymbolTable> fPrevious;
+};
+
+IRGenerator::IRGenerator(std::shared_ptr<SymbolTable> symbolTable, ErrorReporter& errorReporter)
+: fSymbolTable(symbolTable)

[dogben, 2016/06/23 15:25:10]

nit: std::move

+, fErrors(errorReporter) {
+}
+
+void IRGenerator::pushSymbolTable() {
+    fSymbolTable.reset(new SymbolTable(fSymbolTable, fErrors));

[dogben, 2016/06/23 15:25:08]

nit: std::move

+}
+
+void IRGenerator::popSymbolTable() {
+    fSymbolTable = fSymbolTable->fParent;
+}
+
+std::unique_ptr<Extension> IRGenerator::convertExtension(ASTExtension& extension) {
+    return std::unique_ptr<Extension>(new Extension(extension.fPosition, extension.fName));
+}
+
+std::unique_ptr<Statement> IRGenerator::convertStatement(ASTStatement& statement) {
+    switch (statement.fKind) {
+        case ASTStatement::kBlock_Kind:
+            return this->convertBlock((ASTBlock&) statement);
+        case ASTStatement::kVarDeclaration_Kind:
+            return this->convertVarDeclarationStatement((ASTVarDeclarationStatement&) statement);
+        case ASTStatement::kExpression_Kind:
+            return this->convertExpressionStatement((ASTExpressionStatement&) statement);
+        case ASTStatement:: kIf_Kind:

[dogben, 2016/06/23 15:25:10]

nit: extra space

+            return this->convertIf((ASTIfStatement&) statement);
+        case ASTStatement::kFor_Kind:
+            return this->convertFor((ASTForStatement&) statement);
+        case ASTStatement::kWhile_Kind:
+            return this->convertWhile((ASTWhileStatement&) statement);
+        case ASTStatement::kDo_Kind:
+            return this->convertDo((ASTDoStatement&) statement);
+        case ASTStatement::kReturn_Kind:
+            return this->convertReturn((ASTReturnStatement&) statement);
+        case ASTStatement::kBreak_Kind:
+            return this->convertBreak((ASTBreakStatement&) statement);
+        case ASTStatement::kContinue_Kind:
+            return this->convertContinue((ASTContinueStatement&) statement);
+        case ASTStatement::kDiscard_Kind:
+            return this->convertDiscard((ASTDiscardStatement&) statement);
+        default:

[dogben, 2016/06/23 15:25:08]

nit: Do we compile with the warning enabled when not all values of the enum are
handled?

[ethannicholas, 2016/06/24 21:23:08]

Without the 'default:' it works on most platforms, but the Android compiler
complains about the function not always returning.

+            ABORT("unsupported statement type: %d\n", statement.fKind);
+    }
+}
+
+std::unique_ptr<Block> IRGenerator::convertBlock(ASTBlock& block) {
+    AutoSymbolTable table(this);
+    std::vector<std::unique_ptr<Statement>> statements;
+    for (size_t i = 0; i < block.fStatements.size(); i++) {
+        std::unique_ptr<Statement> statement = this->convertStatement(*block.fStatements[i]);
+        if (statement == nullptr) {
+            return nullptr;
+        }
+        statements.push_back(std::move(statement));
+    }
+    return std::unique_ptr<Block>(new Block(block.fPosition, std::move(statements)));
+}
+
+std::unique_ptr<Statement> IRGenerator::convertVarDeclarationStatement(
+                                                                    ASTVarDeclarationStatement& s) {

[dogben, 2016/06/23 15:25:09]

nit: odd indentation

+    auto decl = this->convertVarDeclaration(*s.fDeclaration, Variable::kLocal_Storage);
+    if (decl == nullptr) {
+        return nullptr;
+    }
+    return std::unique_ptr<Statement>(new VarDeclarationStatement(std::move(decl)));
+}
+
+Modifiers IRGenerator::convertModifiers(const ASTModifiers& modifiers) {
+    return Modifiers(modifiers);

[dogben, 2016/06/23 15:25:08]

Shouldn't we check that modifiers don't conflict?

[ethannicholas, 2016/06/24 21:23:08]

It's on my list. This is just the initial checkin; it wasn't my intent to have
everything foam-padded for safety at this point :-).

+}
+
+std::unique_ptr<VarDeclaration> IRGenerator::convertVarDeclaration(ASTVarDeclaration& decl,
+                                                                   Variable::Storage storage) {
+    std::vector<std::shared_ptr<Variable>> variables;
+    std::vector<std::vector<std::unique_ptr<Expression>>> sizes;
+    std::vector<std::unique_ptr<Expression>> values;
+    std::shared_ptr<Type> baseType = this->convertType(*decl.fType);
+    if (baseType == nullptr) {
+        return nullptr;
+    }
+    for (size_t i = 0; i < decl.fNames.size(); i++) {
+        Modifiers modifiers = this->convertModifiers(decl.fModifiers);

[dogben, 2016/06/23 15:25:09]

nit: could be moved outside the loop?

+        std::shared_ptr<Type> type = baseType;
+        std::vector<std::unique_ptr<Expression>> currentVarSizes;

[dogben, 2016/06/23 15:25:08]

If baseType is an array type, should the sizes be added to currentVarSizes
before iterating over fSizes?

[ethannicholas, 2016/06/24 21:23:08]

baseType can't be an array type, since it's the sizes that make array types. But
I'll add an assert just to make that clear.

[dogben, 2016/06/26 03:57:52]

The GLSL spec contains these example var declaration:
vec4[2] a[3]; // size-3 array of size-2 array of vec4
vec4[3][2] a; // size-3 array of size-2 array of vec4

I'm happy to not support this, but that might affect function declarations with
array parameters.

[ethannicholas, 2016/06/27 20:35:03]

Skia (and every shader I have discovered in the wild) exclusively uses the
C-style 'vec4 v[3]' declaration, so that is what I support for the moment. I
actually prefer the 'vec4[3] v' model, and may push for a change to that in the
future, but I have no intention of supporting a mixture of the two.

+        for (size_t j = 0; j < decl.fSizes[i].size(); j++) {
+            if (decl.fSizes[i][j] != nullptr) {
+                ASTExpression& rawSize = *decl.fSizes[i][j];
+                std::unique_ptr<Expression> size = this->convertExpression(rawSize);
+                if (size == nullptr) {
+                    return nullptr;
+                }
+                std::string name = type->fName;
+                uint64_t count;
+                if (size->fKind == Expression::kIntLiteral_Kind) {
+                    count = ((IntLiteral&) *size).fValue;

[dogben, 2016/06/23 15:25:11]

Check that count is positive?

+                    name += "[" + to_string(count) + "]";
+                } else {
+                    count = -1;

[dogben, 2016/06/23 15:25:10]

Check that size's type is integer?

[ethannicholas, 2016/06/24 21:23:09]

Done.

+                    name += "[]";
+                }
+                type = std::shared_ptr<Type>(new Type(name, Type::kArray_Kind, type, (int) count));
+                currentVarSizes.push_back(std::move(size));
+            } else {
+                currentVarSizes.push_back(nullptr);

[dogben, 2016/06/23 15:25:10]

Don't we need to update type here also?

+            }
+        }
+        sizes.push_back(std::move(currentVarSizes));
+        auto var = std::shared_ptr<Variable>(new Variable(decl.fPosition, modifiers, 

[dogben, 2016/06/23 15:25:10]

nit: std::make_shared

+                                                          decl.fNames[i], type, storage));
+        variables.push_back(var);
+        std::unique_ptr<Expression> value;
+        if (decl.fValues[i] != nullptr) {
+            value = this->convertExpression(*decl.fValues[i]);
+            if (value == nullptr) {
+                return nullptr;
+            }
+            value = this->coerce(std::move(value), type);
+        } else {
+            value = nullptr;

[dogben, 2016/06/23 15:25:09]

nit: redundant

+        }
+        values.push_back(std::move(value));
+        fSymbolTable->add(var->fName, var);

[dogben, 2016/06/23 15:25:10]

nit: move this line before variables.push_back?

+    }
+    return std::unique_ptr<VarDeclaration>(new VarDeclaration(decl.fPosition, variables, 

[dogben, 2016/06/23 15:25:11]

nit: std::move(variables)

+                                                              std::move(sizes), std::move(values)));
+}
+
+std::unique_ptr<Statement> IRGenerator::convertIf(ASTIfStatement& s) {
+    std::unique_ptr<Expression> test = this->coerce(this->convertExpression(*s.fTest), kBool_Type);
+    if (test == nullptr) {
+        return nullptr;
+    }
+    std::unique_ptr<Statement> ifTrue = this->convertStatement(*s.fIfTrue);
+    if (ifTrue == nullptr) {
+        return nullptr;
+    }
+    std::unique_ptr<Statement> ifFalse;
+    if (s.fIfFalse != nullptr) {
+        ifFalse = this->convertStatement(*s.fIfFalse);
+        if (ifFalse == nullptr) {
+            return nullptr;
+        }
+    }
+    return std::unique_ptr<Statement>(new IfStatement(s.fPosition, std::move(test), 
+                                                      std::move(ifTrue), std::move(ifFalse)));
+}
+
+std::unique_ptr<Statement> IRGenerator::convertFor(ASTForStatement& f) {
+    AutoSymbolTable table(this);
+    std::unique_ptr<Statement> initializer = this->convertStatement(*f.fInitializer);
+    if (initializer == nullptr) {
+        return nullptr;
+    }
+    std::unique_ptr<Expression> test = this->coerce(this->convertExpression(*f.fTest), kBool_Type);
+    if (test == nullptr) {
+        return nullptr;
+    }
+    std::unique_ptr<Expression> next = this->convertExpression(*f.fNext);

[dogben, 2016/06/23 15:25:10]

Check that next is not FunctionReference or TypeReference?

+    if (next == nullptr) {
+        return nullptr;
+    }
+    std::unique_ptr<Statement> statement = this->convertStatement(*f.fStatement);
+    if (statement == nullptr) {
+        return nullptr;
+    }
+    return std::unique_ptr<Statement>(new ForStatement(f.fPosition, std::move(initializer), 
+                                                       std::move(test), std::move(next),
+                                                       std::move(statement)));
+}
+
+std::unique_ptr<Statement> IRGenerator::convertWhile(ASTWhileStatement& w) {
+    std::unique_ptr<Expression> test = this->coerce(this->convertExpression(*w.fTest), kBool_Type);
+    if (test == nullptr) {
+        return nullptr;
+    }
+    std::unique_ptr<Statement> statement = this->convertStatement(*w.fStatement);
+    if (statement == nullptr) {
+        return nullptr;
+    }
+    return std::unique_ptr<Statement>(new WhileStatement(w.fPosition, std::move(test),
+                                                         std::move(statement)));
+}
+
+std::unique_ptr<Statement> IRGenerator::convertDo(ASTDoStatement& d) {
+    std::unique_ptr<Expression> test = this->convertExpression(*d.fTest);

[dogben, 2016/06/23 15:25:10]

Other tests are coerced. Why not this one?

[ethannicholas, 2016/06/24 21:23:08]

Oversight. Fixed.

+    if (test == nullptr) {
+        return nullptr;
+    }
+    if (test->fType != kBool_Type) {
+        fErrors.error(d.fPosition, "expected 'bool', but found '" + 
+                                   test->fType->description() + "'");
+        return nullptr;
+    }
+    std::unique_ptr<Statement> statement = this->convertStatement(*d.fStatement);
+    if (statement == nullptr) {
+        return nullptr;
+    }
+    return std::unique_ptr<Statement>(new DoStatement(d.fPosition, std::move(statement), 
+                                                      std::move(test)));
+}
+
+std::unique_ptr<Statement> IRGenerator::convertExpressionStatement(ASTExpressionStatement& s) {
+    std::unique_ptr<Expression> e = this->convertExpression(*s.fExpression);
+    if (e == nullptr) {
+        return nullptr;
+    }
+    return std::unique_ptr<Statement>(new ExpressionStatement(std::move(e)));

[dogben, 2016/06/23 15:25:11]

Check that e is not FunctionReference or TypeReference?

[ethannicholas, 2016/06/24 21:23:09]

Done.

+}
+
+std::unique_ptr<Statement> IRGenerator::convertReturn(ASTReturnStatement& r) {
+    if (r.fExpression) {
+        std::unique_ptr<Expression> result = this->convertExpression(*r.fExpression);
+        if (result == nullptr) {
+            return nullptr;
+        }
+        ASSERT(fCurrentFunction);

[dogben, 2016/06/23 15:25:08]

nit: Move outside of if?

+        if (fCurrentFunction->fReturnType == kVoid_Type) {
+            fErrors.error(result->fPosition, "may not return a value from a void function");
+        } else {
+            result = this->coerce(std::move(result), fCurrentFunction->fReturnType);
+            if (result == nullptr) {
+                return nullptr;
+            }
+        }
+        return std::unique_ptr<Statement>(new ReturnStatement(std::move(result)));
+    } else {
+        if (fCurrentFunction->fReturnType != kVoid_Type) {
+            fErrors.error(r.fPosition, "expected function to return '" +
+                                       fCurrentFunction->fReturnType->description() + "'");
+        }
+        return std::unique_ptr<Statement>(new ReturnStatement(r.fPosition));
+    }
+}
+
+std::unique_ptr<Statement> IRGenerator::convertBreak(ASTBreakStatement& b) {
+    return std::unique_ptr<Statement>(new BreakStatement(b.fPosition));
+}
+
+std::unique_ptr<Statement> IRGenerator::convertContinue(ASTContinueStatement& c) {
+    return std::unique_ptr<Statement>(new ContinueStatement(c.fPosition));
+}
+
+std::unique_ptr<Statement> IRGenerator::convertDiscard(ASTDiscardStatement& d) {
+    return std::unique_ptr<Statement>(new DiscardStatement(d.fPosition));
+}
+
+static std::shared_ptr<Type> expand_generics(std::shared_ptr<Type> type, int i) {

[dogben, 2016/06/23 15:25:09]

nit: This function should probably accept and return Type* rather than
shared_ptr<Type>.

[ethannicholas, 2016/06/24 21:23:09]

I don't think it can. If it returns Type*, then I would have to wrap it in
std::shared_ptr<Type> to use it where it needs to be passed, and then I'd have
an ownership conflict. And since it needs to be able to return its parameter, it
has to take a shared_ptr also.

+    if (type->kind() == Type::kGeneric_Kind) {
+        return type->coercibleTypes()[i];
+    }
+    return type;
+}
+
+static void expand_generics(std::shared_ptr<FunctionDeclaration> decl, 

[dogben, 2016/06/23 15:25:09]

nit: This function is not taking ownership of its arguments, so it should accept
raw pointers rather than shared_ptr. Same comment for many of the
functions/methods in this file. Unless the callee retains a reference after it
returns, it shouldn't accept std::shared_ptr as an argument.

+                            std::shared_ptr<SymbolTable> symbolTable) {
+    for (int i = 0; i < 4; i++) {
+        std::shared_ptr<Type> returnType = expand_generics(decl->fReturnType, i);
+        std::vector<std::shared_ptr<Variable>> parameters;
+        for (auto p : decl->fParameters) {

[dogben, 2016/06/23 15:25:11]

nit: const ref

+            parameters.push_back(std::shared_ptr<Variable>(new Variable(decl->fPosition, 

[dogben, 2016/06/23 15:25:08]

Should this be p->fPosition?

+                                                                        Modifiers(p->fModifiers), 
+                                                                        p->fName,
+                                                                        expand_generics(p->fType,
+                                                                                        i),
+                                                                    Variable::kParameter_Storage)));

[dogben, 2016/06/23 15:25:10]

nit: odd indentation

+        }
+        std::shared_ptr<FunctionDeclaration> expanded(new FunctionDeclaration(decl->fPosition, 
+                                                                              decl->fName, 
+                                                                              parameters, 

[dogben, 2016/06/23 15:25:11]

nit: std::move

+                                                                              returnType));

[dogben, 2016/06/23 15:25:10]

nit: std::move

+        symbolTable->add(expanded->fName, expanded);
+    }
+}
+
+std::unique_ptr<FunctionDefinition> IRGenerator::convertFunction(ASTFunction& f) {
+    std::shared_ptr<SymbolTable> old = fSymbolTable;
+    AutoSymbolTable table(this);
+    bool isGeneric;
+    std::shared_ptr<Type> returnType = this->convertType(*f.fReturnType);
+    if (returnType == nullptr) {
+        return nullptr;
+    }
+    isGeneric = returnType->kind() == Type::kGeneric_Kind;

[dogben, 2016/06/23 15:25:11]

nit: declare and initialize in one statement.

+    std::vector<std::shared_ptr<Variable>> parameters;
+    for (size_t i = 0; i < f.fParameters.size(); i++) {

[dogben, 2016/06/23 15:25:08]

nit: could use range-based for

+        std::shared_ptr<Type> type = this->convertType(*f.fParameters[i]->fType);
+        if (type == nullptr) {
+            return nullptr;
+        }
+        for (int j = (int) f.fParameters[i]->fSizes.size() - 1; j >= 0; j--) {
+            int size = f.fParameters[i]->fSizes[j];
+            type = std::shared_ptr<Type>(new Type(type->name() + "[" + to_string(size) + "]", 
+                                                  Type::kArray_Kind, type, size));

[dogben, 2016/06/23 15:25:09]

nit: std::move(type)

+        }
+        std::string name = f.fParameters[i]->fName;
+        Modifiers modifiers = this->convertModifiers(f.fParameters[i]->fModifiers);
+        Position pos = f.fParameters[i]->fPosition;
+        std::shared_ptr<Variable> var = std::shared_ptr<Variable>(new Variable(pos, 
+                                                                               modifiers, 
+                                                                               name, 

[dogben, 2016/06/23 15:25:09]

nit: std::move

+                                                                               type,

[dogben, 2016/06/23 15:25:10]

nit: std::move

+                                                                     Variable::kParameter_Storage));

[dogben, 2016/06/23 15:25:11]

nit: odd indentation

+        parameters.push_back(var);

[dogben, 2016/06/23 15:25:10]

nit: std::move

+        isGeneric |= type->kind() == Type::kGeneric_Kind;
+    }
+
+    // find existing declaration
+    std::shared_ptr<FunctionDeclaration> decl;
+    auto entry = (*old)[f.fName];
+    if (entry) {
+        std::vector<std::shared_ptr<FunctionDeclaration>> functions;
+        switch (entry->fKind) {
+            case Symbol::kUnresolvedFunction_Kind:
+                functions = std::static_pointer_cast<UnresolvedFunction>(entry)->fFunctions;
+                break;
+            case Symbol::kFunctionDeclaration_Kind:
+                functions.push_back(std::static_pointer_cast<FunctionDeclaration>(entry));
+                break;
+            default:
+                fErrors.error(f.fPosition, "symbol '" + f.fName + "' was already defined");
+                return nullptr;
+        }
+        for (auto other : functions) {

[dogben, 2016/06/23 15:25:09]

nit: const ref

+            ASSERT(other->fName == f.fName);
+            if (parameters.size() == other->fParameters.size()) {
+                bool match = true;
+                for (size_t i = 0; i < parameters.size(); i++) {
+                    if (parameters[i]->fType != other->fParameters[i]->fType) {
+                        match = false;
+                        break;
+                    }
+                }
+                if (match) {
+                    if (returnType != other->fReturnType) {
+                        FunctionDeclaration newDecl = FunctionDeclaration(f.fPosition, 
+                                                                          f.fName, 
+                                                                          parameters, 
+                                                                          returnType);
+                        fErrors.error(f.fPosition, "functions '" + newDecl.description() +
+                                                   "' and '" + other->description() + 
+                                                   "' differ only in return type");
+                        return nullptr;
+                    }
+                    if (other->fDefined) {
+                        fErrors.error(f.fPosition, "duplicate definition of " + 
+                                                   other->description());
+                    }
+                    decl = other;
+                    for (size_t i = 0; i < parameters.size(); i++) {
+                        fSymbolTable->add(parameters[i]->fName, decl->fParameters[i]);                        

[dogben, 2016/06/23 15:25:11]

Are modifiers allowed to differ between declaration and definition (like const
in C++)?

[ethannicholas, 2016/06/24 21:23:09]

Good catch. No, they should not, I will add an error check for that.

+                    }
+                    break;
+                }
+            }
+        }
+    }
+    if (!decl) {
+        // couldn't find an existing declaration
+        decl.reset(new FunctionDeclaration(f.fPosition, f.fName, parameters, returnType));
+        for (auto var : parameters) {

[dogben, 2016/06/23 15:25:09]

nit: const ref

+            fSymbolTable->add(var->fName, var);
+        }
+    }
+    decl->fDefined = true;
+    if (isGeneric) {
+        ASSERT(f.fBody == nullptr);
+        expand_generics(decl, old);
+    } else {
+        old->add(decl->fName, decl);
+        if (f.fBody != nullptr) {
+            ASSERT(fCurrentFunction == nullptr);
+            fCurrentFunction = decl;
+            std::unique_ptr<Block> body = this->convertBlock(*f.fBody);

[dogben, 2016/06/23 15:25:09]

Seems like a very minor issue, but the spec says "A function's parameter
declarations and body together form a single scope nested in the global scope,"
and "...a declared name cannot be redeclared in the same scope..."

convertBlock uses AutoSymbolTable to start a new scope, so the function body is
in a different scope from the parameters, and therefore a local var could shadow
a parameter.

[ethannicholas, 2016/06/24 21:23:09]

I might end up changing this, but in general it is not my intent to slavishly
adhere to the GLSL spec. This is not a GLSL compiler, and the language will
become more different as we proceed.

+            fCurrentFunction = nullptr;
+            if (body == nullptr) {
+                return nullptr;
+            }
+            return std::unique_ptr<FunctionDefinition>(new FunctionDefinition(f.fPosition, decl, 

[dogben, 2016/06/24 17:05:26]

Check for return at end of body? (Maybe easier to add "bool* endsInReturn" param
to convertBlock.)

The spec doesn't seem to say anything about this, so maybe forgetting the return
is allowed and means the return value is undefined?

[ethannicholas, 2016/06/24 21:23:09]

You need control flow analysis to prove that a function returns, and I don't
perform that analysis yet. It's coming.

+                                                                              std::move(body)));
+        }
+    }
+    return nullptr;
+}
+
+std::unique_ptr<InterfaceBlock> IRGenerator::convertInterfaceBlock(ASTInterfaceBlock& intf) {
+    std::shared_ptr<SymbolTable> old = fSymbolTable;
+    AutoSymbolTable table(this);
+    Modifiers mods = this->convertModifiers(intf.fModifiers);
+    std::vector<Type::Field> fields;
+    for (size_t i = 0; i < intf.fDeclarations.size(); i++) {
+        std::unique_ptr<VarDeclaration> decl = this->convertVarDeclaration(*intf.fDeclarations[i], 
+                                                                Variable::kGlobal_Storage);

[dogben, 2016/06/23 15:25:09]

nit: odd indentation

+        for (size_t j = 0; j < decl->fVars.size(); j++) {
+            fields.push_back(Type::Field(decl->fVars[j]->fModifiers, decl->fVars[j]->fName, 

[dogben, 2016/06/23 15:25:08]

Shouldn't we OR in mods.fFlags? And check that they don't conflict?

[ethannicholas, 2016/06/24 21:23:08]

There are probably some illegal combinations we should reject -- I'll look into
that -- but no, we shouldn't be ORing them together. These are members of a
struct, not top-level variables in their own right, and the top-level
declarations go on the struct in the output, rather than on the individual
fields (OpDecorate vs. OpMemberDecorate).

+                                         decl->fVars[j]->fType));
+            if (decl->fValues[j] != nullptr) {
+                fErrors.error(decl->fPosition, "initializers are not permitted on interface block "
+                                               "fields");
+            }
+        }        
+    }
+    std::shared_ptr<Type> type = std::shared_ptr<Type>(new Type(intf.fInterfaceName, fields));
+    std::string name = intf.fValueName.length() > 0 ? intf.fValueName : intf.fInterfaceName;
+    std::shared_ptr<Variable> var = std::shared_ptr<Variable>(new Variable(intf.fPosition, mods, 
+                                                                          name, type,
+                                                                Variable::kGlobal_Storage));

[dogben, 2016/06/23 15:25:11]

nit: odd indentation

+    if (intf.fValueName.length()) {
+        old->add(intf.fValueName, var);
+
+    } else {
+        for (size_t i = 0; i < fields.size(); i++) {
+            std::shared_ptr<Field> field = std::shared_ptr<Field>(new Field(intf.fPosition, var, 
+                                                                            (int) i));
+            old->add(fields[i].fName, field);
+        }
+    }
+    return std::unique_ptr<InterfaceBlock>(new InterfaceBlock(intf.fPosition, var));
+}
+
+std::shared_ptr<Type> IRGenerator::convertType(ASTType& type) {
+    std::shared_ptr<Symbol> result = (*fSymbolTable)[type.fName];
+    if (result != nullptr && result->fKind == Symbol::kType_Kind) {
+        return std::static_pointer_cast<Type>(result);
+    }
+    fErrors.error(type.fPosition, "unknown type '" + type.fName + "'");
+    return nullptr;
+}
+
+std::unique_ptr<Expression> IRGenerator::convertExpression(ASTExpression& expr) {
+    switch (expr.fKind) {
+        case ASTExpression::kIdentifier_Kind:
+            return this->convertIdentifier((ASTIdentifier&) expr);
+        case ASTExpression::kBool_Kind:
+            return std::unique_ptr<Expression>(new BoolLiteral(expr.fPosition,
+                                                               ((ASTBoolLiteral&) expr).fValue));
+        case ASTExpression::kInt_Kind:
+            return std::unique_ptr<Expression>(new IntLiteral(expr.fPosition,
+                                                              ((ASTIntLiteral&) expr).fValue));
+        case ASTExpression::kFloat_Kind:
+            return std::unique_ptr<Expression>(new FloatLiteral(expr.fPosition,
+                                                                ((ASTFloatLiteral&) expr).fValue));
+        case ASTExpression::kBinary_Kind:
+            return this->convertBinaryExpression((ASTBinaryExpression&) expr);
+        case ASTExpression::kPrefix_Kind:
+            return this->convertPrefixExpression((ASTPrefixExpression&) expr);
+        case ASTExpression::kSuffix_Kind:
+            return this->convertSuffixExpression((ASTSuffixExpression&) expr);
+        case ASTExpression::kTernary_Kind:
+            return this->convertTernaryExpression((ASTTernaryExpression&) expr);
+        default:
+            ABORT("unsupported expression type: %d\n", expr.fKind);
+    }
+}
+
+std::unique_ptr<Expression> IRGenerator::convertIdentifier(ASTIdentifier& identifier) {
+    std::shared_ptr<Symbol> result = (*fSymbolTable)[identifier.fText];
+    if (result == nullptr) {
+        fErrors.error(identifier.fPosition, "unknown identifier '" + identifier.fText + "'");
+        return nullptr;
+    }
+    switch (result->fKind) {
+        case Symbol::kFunctionDeclaration_Kind: {
+            std::vector<std::shared_ptr<FunctionDeclaration>> f;
+            f.push_back(std::static_pointer_cast<FunctionDeclaration>(result));
+            return std::unique_ptr<FunctionReference>(new FunctionReference(identifier.fPosition,
+                                                                            f));

[dogben, 2016/06/23 15:25:11]

nit: std::move

+        }
+        case Symbol::kUnresolvedFunction_Kind: {
+            auto f = std::static_pointer_cast<UnresolvedFunction>(result);
+            return std::unique_ptr<FunctionReference>(new FunctionReference(identifier.fPosition,
+                                                                            f->fFunctions));
+        }
+        case Symbol::kVariable_Kind: {
+            std::shared_ptr<Variable> var = std::static_pointer_cast<Variable>(result);
+            this->markReadFrom(var);
+            return std::unique_ptr<VariableReference>(new VariableReference(identifier.fPosition,
+                                                                            var));

[dogben, 2016/06/23 15:25:09]

nit: std::move

+        }
+        case Symbol::kField_Kind: {
+            std::shared_ptr<Field> field = std::static_pointer_cast<Field>(result);
+            VariableReference* base = new VariableReference(identifier.fPosition, field->fOwner);
+            return std::unique_ptr<Expression>(new FieldAccess(std::unique_ptr<Expression>(base),
+                                                               field->fFieldIndex));
+        }
+        case Symbol::kType_Kind: {
+            auto t = std::static_pointer_cast<Type>(result);
+            return std::unique_ptr<TypeReference>(new TypeReference(identifier.fPosition, t));

[dogben, 2016/06/23 15:25:11]

nit: std::move(t)

+        }
+        default:
+            ABORT("unsupported symbol type %d\n", result->fKind);
+    }
+
+}
+
+std::unique_ptr<Expression> IRGenerator::coerce(std::unique_ptr<Expression> expr, 
+                                                std::shared_ptr<Type> type) {
+    if (expr == nullptr) {
+        return nullptr;
+    }
+    if (*expr->fType == *type) {

[dogben, 2016/06/23 15:25:11]

nit: Elsewhere, you compare types by pointer address.

+        return expr;
+    }
+    if (!expr->fType->canCoerceTo(type)) {
+        fErrors.error(expr->fPosition, "expected '" + type->description() + "', but found '" + 
+                                        expr->fType->description() + "'");
+        return nullptr;
+    }
+    if (type->kind() == Type::kScalar_Kind) {

[dogben, 2016/06/23 15:25:09]

Why only scalars?

[ethannicholas, 2016/06/24 21:23:09]

Because we don't currently support compound types that contain anything other
than float, so only scalars actually get coerced.

+        std::vector<std::unique_ptr<Expression>> parameters;

[dogben, 2016/06/23 15:25:11]

nit: s/parameters/arguments/ (or just pass {std::move(expr)} below)

[ethannicholas, 2016/06/24 21:23:08]

This would have been easier had I realized you could create vectors with that
syntax before you told me :-)

+        parameters.push_back(std::move(expr));
+        ASTIdentifier id(Position(), type->description());
+        std::unique_ptr<Expression> ctor = this->convertIdentifier(id);
+        ASSERT(ctor);
+        return this->call(Position(), std::move(ctor), std::move(parameters));
+    }
+    ABORT("cannot coerce %s to %s", expr->fType->description().c_str(), type->name().c_str());

[dogben, 2016/06/23 15:25:10]

nit: s/name()/description()/?

+}
+
+/**
+ * Determines the operand and result types of a binary expression. Returns true if the expression is
+ * legal, false otherwise. If false, the values of the out parameters are undefined.
+ */
+static bool determine_binary_type(Token::Kind op, std::shared_ptr<Type> left, 
+                                  std::shared_ptr<Type> right, 
+                                  std::shared_ptr<Type>* outLeftType,
+                                  std::shared_ptr<Type>* outRightType,
+                                  std::shared_ptr<Type>* outResultType,
+                                  bool tryFlipped) {
+    if (op == Token::STAR || op == Token::STAREQ) {
+        if (left->kind() == Type::kMatrix_Kind && right->kind() == Type::kVector_Kind) {
+            *outLeftType = left;
+            *outRightType = right;
+            *outResultType = right;
+            return left->rows() == right->columns();
+        }  
+        if (left->kind() == Type::kVector_Kind && right->kind() == Type::kMatrix_Kind) {
+            *outLeftType = left;
+            *outRightType = right;
+            *outResultType = left;
+            return left->columns() == right->columns();
+        }  
+    } 

[dogben, 2016/06/23 15:25:09]

Don't we need a special case for matrix * matrix also?

[ethannicholas, 2016/06/24 21:23:08]

Non-square matrices are on my list; obviously there's a lot of the groundwork
laid, but they don't actually work (and we don't use them).

+    bool isLogical;
+    switch (op) {
+        case Token::EQEQ: // fall through
+        case Token::NEQ:  // fall through
+        case Token::LT:   // fall through
+        case Token::GT:   // fall through
+        case Token::LTEQ: // fall through
+        case Token::GTEQ:
+            isLogical = true;
+            break;
+        default:
+            isLogical = false;
+    }
+    // FIXME: need to disallow illegal operations like vec3 > vec3

[dogben, 2016/06/23 15:25:11]

Also, bitwise and shift operations only apply to integer scalars and vectors.

[ethannicholas, 2016/06/24 21:23:09]

Only floats are fully supported for now, I've added a comment to that effect.

+    if (left == right) {
+        *outLeftType = left;
+        *outRightType = left;
+        if (isLogical) {
+            *outResultType = kBool_Type;
+        } else {
+            *outResultType = left;
+        }
+        return true;
+    }
+    if (left->canCoerceTo(right)) {

[dogben, 2016/06/23 15:25:10]

This case is incorrect for shift operations.

+        *outLeftType = right;
+        *outRightType = right;
+        if (isLogical) {
+            *outResultType = kBool_Type;
+        } else {
+            *outResultType = right;
+        }
+        return true;
+    }
+    if ((left->columns() > 1) && (right->kind() == Type::kScalar_Kind)) {

[dogben, 2016/06/23 15:25:11]

This case shouldn't apply to arrays, right?

[ethannicholas, 2016/06/24 21:23:08]

Good catch. This was written long before I added support for arrays.

+        if (determine_binary_type(op, left->componentType(), right, outLeftType, outRightType,
+                                  outResultType, false)) {
+            *outLeftType = (*outLeftType)->toCompound(left->columns(), left->rows());
+            if (!isLogical) {
+                *outResultType = (*outResultType)->toCompound(left->columns(), left->rows());
+            }
+            return true;
+        }
+        return false;
+    }
+    if (tryFlipped) {
+        return determine_binary_type(op, right, left, outRightType, outLeftType, outResultType, 
+                                     false);
+    }
+    return false;
+}
+
+std::unique_ptr<Expression> IRGenerator::convertBinaryExpression(ASTBinaryExpression& expression) {
+    std::unique_ptr<Expression> left = this->convertExpression(*expression.fLeft);
+    if (left == nullptr) {
+        return nullptr;
+    }
+    std::unique_ptr<Expression> right = this->convertExpression(*expression.fRight);
+    if (right == nullptr) {
+        return nullptr;
+    }
+    std::shared_ptr<Type> leftType;
+    std::shared_ptr<Type> rightType;
+    std::shared_ptr<Type> resultType;
+    if (!determine_binary_type(expression.fOperator, left->fType, right->fType, &leftType,

[dogben, 2016/06/23 15:25:10]

Seems like we need special handling for logical operators (&&, ||).

[ethannicholas, 2016/06/24 21:23:09]

In what way?

[dogben, 2016/06/26 03:57:52]

Sorry for being unclear.

Is "an_int || another_int" legal? If so, both sides should be coerced to bool.
Using determine_binary_type would allow both sides to remain ints.

[ethannicholas, 2016/06/27 20:35:03]

No, int cannot be coerced to bool.

[dogben, 2016/06/28 02:14:54]

Sorry for being unclear. If you give "an_int || another_int" to
determine_binary_type, it will happily tell you that leftType, rightType, and
resultType are kInt_Type.

[ethannicholas, 2016/06/30 15:19:28]

Sorry, gotcha.

+                               &rightType, &resultType, true)) {

[dogben, 2016/06/23 15:25:11]

tryFlipped probably shouldn't be true for shift operations.

[ethannicholas, 2016/06/24 21:23:08]

It doesn't actually flip the operation around, it just determines the types as
if it had (so I don't have to duplicate the block of code that checks left
coercing to right and matrix * scalar).

[dogben, 2016/06/26 03:57:53]

Understood. This comment really depends on what you do for shift operations for
the FIXME above, but since we need to check that rhs is an int, I don't see how
tryFlipped would work.

+        fErrors.error(expression.fPosition, "type mismatch: '" + 
+                                            Token::OperatorName(expression.fOperator) + 
+                                            "' cannot operate on '" + left->fType->fName + 
+                                            "', '" + right->fType->fName + "'");
+        return nullptr;
+    }
+    switch (expression.fOperator) {
+        case Token::EQ:           // fall through
+        case Token::PLUSEQ:       // fall through
+        case Token::MINUSEQ:      // fall through
+        case Token::STAREQ:       // fall through
+        case Token::SLASHEQ:      // fall through
+        case Token::PERCENTEQ:    // fall through
+        case Token::SHLEQ:        // fall through
+        case Token::SHREQ:        // fall through
+        case Token::BITWISEOREQ:  // fall through
+        case Token::BITWISEXOREQ: // fall through
+        case Token::BITWISEANDEQ: // fall through
+        case Token::LOGICALOREQ:  // fall through
+        case Token::LOGICALXOREQ: // fall through
+        case Token::LOGICALANDEQ: 
+            this->markWrittenTo(*left);
+        default:
+            break;
+    }
+    return std::unique_ptr<Expression>(new BinaryExpression(expression.fPosition, 
+                                                            this->coerce(std::move(left), leftType), 

[dogben, 2016/06/23 15:25:10]

nit: assert not null? (also for next coerce)

+                                                            expression.fOperator, 
+                                                            this->coerce(std::move(right), 
+                                                                         rightType), 
+                                                            resultType));
+}
+
+std::unique_ptr<Expression> IRGenerator::convertTernaryExpression(ASTTernaryExpression& expression) {
+    std::unique_ptr<Expression> test = this->coerce(this->convertExpression(*expression.fTest), 
+                                                    kBool_Type);
+    if (test == nullptr) {
+        return nullptr;
+    }
+    std::unique_ptr<Expression> ifTrue = this->convertExpression(*expression.fIfTrue);
+    if (ifTrue == nullptr) {
+        return nullptr;
+    }
+    std::unique_ptr<Expression> ifFalse = this->convertExpression(*expression.fIfFalse);
+    if (ifFalse == nullptr) {
+        return nullptr;
+    }
+    std::shared_ptr<Type> trueType;
+    std::shared_ptr<Type> falseType;
+    std::shared_ptr<Type> resultType;
+    if (!determine_binary_type(Token::EQEQ, ifTrue->fType, ifFalse->fType, &trueType,
+                               &falseType, &resultType, true)) {
+        fErrors.error(expression.fPosition, "ternary operator result mismatch: '" + 
+                                            ifTrue->fType->fName + "', '" + 
+                                            ifFalse->fType->fName + "'");
+        return nullptr;
+    }
+    ASSERT(trueType == falseType);
+    ifTrue = this->coerce(std::move(ifTrue), trueType);
+    ifFalse = this->coerce(std::move(ifFalse), falseType);
+    return std::unique_ptr<Expression>(new TernaryExpression(expression.fPosition, 
+                                                             std::move(test),
+                                                             std::move(ifTrue), 
+                                                             std::move(ifFalse)));
+}
+
+std::unique_ptr<Expression> IRGenerator::call(Position position, 
+                                              std::shared_ptr<FunctionDeclaration> function, 
+                                              std::vector<std::unique_ptr<Expression>> parameters) {

[dogben, 2016/06/23 15:25:10]

nit: s/parameters/arguments/

+    if (function->fParameters.size() != parameters.size()) {
+        std::string msg = "call to '" + function->fName + "' expected " + 
+                                 to_string(function->fParameters.size()) + 
+                                 " parameter";
+        if (function->fParameters.size() != 1) {
+            msg += "s";
+        }
+        msg += ", but found " + to_string(parameters.size());
+        fErrors.error(position, msg);
+        return nullptr;
+    }
+    for (size_t i = 0; i < parameters.size(); i++) {
+        parameters[i] = this->coerce(std::move(parameters[i]), function->fParameters[i]->fType);

[dogben, 2016/06/23 15:25:09]

1. How does this work for out/inout parameters?
2. Check that coerce returned non-null?

[ethannicholas, 2016/06/24 21:23:08]

Good catch, was missing a markWrittenTo here.
Don't need an early return for a null coercion, as it will have reported an
error but that doesn't stop us from evaluating the rest of the parameters.

+    }    
+    return std::unique_ptr<FunctionCall>(new FunctionCall(position, function,
+                                                          std::move(parameters)));
+}
+
+/**
+ * Determines the cost of coercing the parameters of a function to the required types. Returns true 
+ * if the cost could be computed, false if the call is not valid. Cost has no particular meaning 
+ * other than "lower costs are preferred".
+ */
+bool IRGenerator::determineCallCost(std::shared_ptr<FunctionDeclaration> function, 
+                                    std::vector<std::unique_ptr<Expression>>& parameters,

[dogben, 2016/06/23 15:25:08]

nit: s/parameters/arguments/
nit: const?

+                                    int* outCost) {
+    if (function->fParameters.size() != parameters.size()) {
+        return false;
+    }
+    int total = 0;
+    for (size_t i = 0; i < parameters.size(); i++) {
+        int cost;
+        if (parameters[i]->fType->determineCoercionCost(function->fParameters[i]->fType, &cost)) {
+            total += cost;
+        } else {
+            return false;
+        }
+    }
+    *outCost = total;
+    return true;
+}
+
+std::unique_ptr<Expression> IRGenerator::call(Position position, 
+                                              std::unique_ptr<Expression> functionValue, 
+                                              std::vector<std::unique_ptr<Expression>> parameters) {

[dogben, 2016/06/23 15:25:09]

nit: s/parameters/arguments/

+    if (functionValue->fKind == Expression::kTypeReference_Kind) {
+        return this->convertConstructor(position, 
+                                        ((TypeReference&) *functionValue).fValue, 
+                                        std::move(parameters));
+    }
+    if (functionValue->fKind != Expression::kFunctionReference_Kind) {
+        fErrors.error(position, "'" + functionValue->description() + "' is not a function");
+        return nullptr;
+    }
+    FunctionReference* ref = (FunctionReference*) functionValue.get();
+    int bestCost = INT_MAX;
+    std::shared_ptr<FunctionDeclaration> best;
+    if (ref->fFunctions.size() > 1) {
+        for (auto f : ref->fFunctions) {

[dogben, 2016/06/23 15:25:11]

nit: const ref

+            int cost;
+            if (this->determineCallCost(f, parameters, &cost) && cost < bestCost) {
+                bestCost = cost;
+                best = f;
+            }
+        }
+        if (best != nullptr) {
+            return this->call(position, best, std::move(parameters));

[dogben, 2016/06/23 15:25:09]

nit: std::move(best)

+        }
+        std::string msg = "no match for " + ref->fFunctions[0]->fName + "(";
+        std::string separator = "";
+        for (size_t i = 0; i < parameters.size(); i++) {
+            msg += separator;
+            separator = ", ";
+            msg += parameters[i]->fType->description();
+        }
+        msg += ")";
+        fErrors.error(position, msg);
+        return nullptr;
+    }
+    return this->call(position, ref->fFunctions[0], std::move(parameters));
+}
+
+std::unique_ptr<Expression> IRGenerator::convertConstructor(Position position, 
+                                                            std::shared_ptr<Type> type, 
+                                                  std::vector<std::unique_ptr<Expression>> params) {

[dogben, 2016/06/23 15:25:10]

nit: s/params/args/
nit: odd indentation

+    if (type == kFloat_Type && params.size() == 1 && 
+        params[0]->fKind == Expression::kIntLiteral_Kind) {
+        int64_t value = ((IntLiteral&) *params[0]).fValue;
+        return std::unique_ptr<Expression>(new FloatLiteral(position, (double) value));
+    }
+    int min = type->rows() * type->columns();
+    int max = type->columns() > 1 ? INT_MAX : min;

[dogben, 2016/06/24 17:05:26]

This doesn't seem like it would work for struct constructors.

[ethannicholas, 2016/06/24 21:23:09]

We don't use them. I've got it on my list of things to add support for.

+    int actual = 0;
+    for (size_t i = 0; i < params.size(); i++) {
+        if (params[i]->fType->kind() == Type::kScalar_Kind) {
+            actual += 1;
+            if (type->kind() != Type::kScalar_Kind) {

[dogben, 2016/06/23 15:25:10]

Is this the correct criteria? I would expect we want to coerce to component type
for matrices, vectors, and arrays, but not for other types, including scalars
and structs.

[ethannicholas, 2016/06/24 21:23:08]

I only officially support float vectors and matrices at the moment (and this is
all Skia uses), so there's never actually any coercion for them. Obviously I've
laid a lot of the groundwork for other types, but I'm not worried about having
it all perfect in the first release.

+                params[i] = coerce(std::move(params[i]), type->componentType());

[dogben, 2016/06/23 15:25:11]

Check that coerce returns non-null?
nit: this->coerce

[ethannicholas, 2016/06/24 21:23:08]

Fixed the nit, but I don't think the null check matters -- if the result is
null, an error will already have been reported, so we might as well continue
checking the other parameters.

+            }
+        } else {
+            actual += params[i]->fType->rows() * params[i]->fType->columns();

[dogben, 2016/06/23 15:25:11]

Check that rows() and columns() are not -1?

[ethannicholas, 2016/06/24 21:23:08]

rows() and columns() will both generate an assertion error if the count is
invalid (which it will only be if someone writes some really weird code). While
this is not ideal, this is just the initial checkin of the compiler and I'm
aware of several ways to write code sufficiently insane that it causes an
assertion failure. I'll be tightening things up over time.

+        }
+    }
+    if ((type->kind() != Type::kVector_Kind || actual != 1) && 

[dogben, 2016/06/23 15:25:09]

nit: Might be a bit clearer to rephrase as:
(actual < min || actual > max) &&
!((type->kind() == Type::kVector_Kind || type->kind() == Type::kMatrix_Kind) &&
(actual == 1))

+        (type->kind() != Type::kMatrix_Kind || actual != 1) && 
+        (actual < min || actual > max)) {

[dogben, 2016/06/23 15:25:10]

Checking my understanding: Are all of the following valid?
vec2(vec3(1, 2, 3, 4))
mat2x3(mat3x2(mat2(vec3(1, 2, 3), 4), vec2(5, 6)))
mat2(mat3(vec4(1, 2, 3, 4), vec3(5, 6, 7), vec2(8, 9)))
mat4(0)

(If yes, this language seems insane.)

[dogben, 2016/06/24 17:05:26]

I happened upon the section of the spec that talks about this. It's insane, but
slightly less insane than I thought. The spec says, "It is a compile-time
error to provide extra arguments beyond this last used argument." My
interpretation is that the following are actually illegal:
vec3(1, 2, 3, 4)
mat2(vec2(1, 2), vec2(3, 4), vec2(5, 6))

[ethannicholas, 2016/06/24 21:23:08]

Yes! This is permitted by GLSL, though I am considering removing this "feature"
in SkSL (unfortunately, Skia actually does create a vector with too many
parameters in at least one spot).
These are currently accepted but shouldn't be. It's on my list to tighten this
up.
Yes! This results in a matrix in which all components are zero. mat4(1) would
give you a standard 4x4 identity matrix.
Yes!

+        fErrors.error(position, "invalid parameters to '" + type->description() + 
+                                "' constructor (expected " + to_string(min) + " scalars, " +
+                                "but found " + to_string(actual) + ")");
+        return nullptr;
+    }
+    if (type->isNumber()) {
+        ASSERT(params.size() == 1);
+        if (params[0]->fType == kBool_Type) {
+            return std::unique_ptr<Expression>(new TernaryExpression(position, std::move(params[0]),

[dogben, 2016/06/23 15:25:11]

nit: position maybe should be params[0]->fPosition

+                                           std::unique_ptr<Expression>(new IntLiteral(position, 1)),

[dogben, 2016/06/23 15:25:10]

nit: odd indentation

+                                         std::unique_ptr<Expression>(new IntLiteral(position, 0))));
+        }
+    }
+    if (params.size() == 1 && params[0]->fType == type) {
+        // parameter is already the right type, just return it
+        return std::move(params[0]);
+    }
+    return std::unique_ptr<Expression>(new Constructor(position, type, std::move(params)));

[dogben, 2016/06/23 15:25:10]

nit: std::move(type)

+}
+
+std::unique_ptr<Expression> IRGenerator::convertPrefixExpression(ASTPrefixExpression& expression) {
+    std::unique_ptr<Expression> base = this->convertExpression(*expression.fOperand);
+    if (base == nullptr) {
+        return nullptr;
+    }
+    switch (expression.fOperator) {
+        case Token::PLUS:
+            if (!base->fType->isNumber() && base->fType->kind() != Type::kVector_Kind) {
+                fErrors.error(expression.fPosition, 
+                              "'+' cannot operate on '" + base->fType->description() + "'");
+                return nullptr;
+            }
+            return base;
+        case Token::MINUS:
+            if (!base->fType->isNumber() && base->fType->kind() != Type::kVector_Kind) {
+                fErrors.error(expression.fPosition, 
+                              "'-' cannot operate on '" + base->fType->description() + "'");
+                return nullptr;
+            }
+            if (base->fKind == Expression::kIntLiteral_Kind) {
+                return std::unique_ptr<Expression>(new IntLiteral(base->fPosition,
+                                                                  -((IntLiteral&) *base).fValue));
+            }
+            if (base->fKind == Expression::kFloatLiteral_Kind) {
+                double value = -((FloatLiteral&) *base).fValue;
+                return std::unique_ptr<Expression>(new FloatLiteral(base->fPosition, value));
+            }
+            return std::unique_ptr<Expression>(new PrefixExpression(Token::MINUS, std::move(base)));
+        case Token::PLUSPLUS:
+            if (!base->fType->isNumber()) {
+                fErrors.error(expression.fPosition, 
+                              "'" + Token::OperatorName(expression.fOperator) + 
+                              "' cannot operate on '" + base->fType->description() + "'");
+                return nullptr;
+            }
+            this->markWrittenTo(*base);
+            break;
+        case Token::MINUSMINUS: 
+            if (!base->fType->isNumber()) {
+                fErrors.error(expression.fPosition, 
+                              "'" + Token::OperatorName(expression.fOperator) + 
+                              "' cannot operate on '" + base->fType->description() + "'");
+                return nullptr;
+            }
+            this->markWrittenTo(*base);
+            break;
+        case Token::NOT:
+            if (base->fType != kBool_Type) {
+                fErrors.error(expression.fPosition, 
+                              "'" + Token::OperatorName(expression.fOperator) + 
+                              "' cannot operate on '" + base->fType->description() + "'");
+                return nullptr;
+            }
+            break;
+        default: 
+            ABORT("unsupported prefix operator\n");
+    }
+    return std::unique_ptr<Expression>(new PrefixExpression(expression.fOperator, 
+                                                            std::move(base)));
+}
+
+std::unique_ptr<Expression> IRGenerator::convertIndex(std::unique_ptr<Expression> base,
+                                                      ASTExpression& index) {
+    if (base->fType->kind() != Type::kArray_Kind && base->fType->kind() != Type::kMatrix_Kind &&
+        base->fType->kind() != Type::kVector_Kind) {
+        fErrors.error(base->fPosition, "expected array, but found '" + base->fType->description() + 
+                                       "'");
+        return nullptr;
+    }
+    std::unique_ptr<Expression> converted = this->convertExpression(index);
+    if (converted == nullptr) {
+        return nullptr;
+    }
+    converted = this->coerce(std::move(converted), kInt_Type);
+    if (converted == nullptr) {
+        return nullptr;
+    }
+    return std::unique_ptr<Expression>(new IndexExpression(std::move(base), std::move(converted)));

[dogben, 2016/06/23 15:25:09]

If index is constant and base size is known, should this check that the index is
in bounds?

[ethannicholas, 2016/06/24 21:23:09]

I was planning to worry about that when I've got a dataflow analyzer in place.

+}
+
+std::unique_ptr<Expression> IRGenerator::convertField(std::unique_ptr<Expression> base,
+                                                      std::string field) {

[dogben, 2016/06/23 15:25:09]

nit: const ref?

+    auto fields = base->fType->fields();
+    for (size_t i = 0; i < fields.size(); i++) {
+        if (fields[i].fName == field) {
+            return std::unique_ptr<Expression>(new FieldAccess(std::move(base), (int) i));
+        }
+    }
+    fErrors.error(base->fPosition, "type '" + base->fType->description() + "' does not have a "

[dogben, 2016/06/23 15:25:11]

The spec says something about arrays having a length() method. Are we not
supporting that?

[ethannicholas, 2016/06/24 21:23:08]

Not yet, at least.

+                                   "field named '" + field + "");
+    return nullptr;
+}
+
+std::unique_ptr<Expression> IRGenerator::convertSwizzle(std::unique_ptr<Expression> base,
+                                                        std::string fields) {

[dogben, 2016/06/23 15:25:09]

nit: const ref?

+    if (base->fType->columns() == 0) {

[dogben, 2016/06/23 15:25:10]

nit: Should this just be an assert?

+        fErrors.error(base->fPosition, "cannot swizzle type '" + base->fType->description() + "'");
+        return nullptr;
+    }
+    std::vector<int> swizzleComponents;
+    for (char c : fields) {
+        switch (c) {
+            case 'x': // fall through
+            case 'r': // fall through
+            case 's': 
+                swizzleComponents.push_back(0);
+                break;
+            case 'y': // fall through
+            case 'g': // fall through
+            case 't':
+                if (base->fType->columns() >= 2) {
+                    swizzleComponents.push_back(1);
+                    break;
+                }
+                // fall through
+            case 'z': // fall through
+            case 'b': // fall through
+            case 'p': 
+                if (base->fType->columns() >= 3) {
+                    swizzleComponents.push_back(2);
+                    break;
+                }
+                // fall through
+            case 'w': // fall through
+            case 'a': // fall through
+            case 'q':
+                if (base->fType->columns() >= 4) {
+                    swizzleComponents.push_back(3);
+                    break;
+                }
+                // fall through
+            default:
+                fErrors.error(base->fPosition, "invalid swizzle component '" + std::string(1, c) +
+                                               "'");
+                return nullptr;
+        }
+    }
+    ASSERT(swizzleComponents.size() > 0);
+    if (swizzleComponents.size() > 4) {
+        fErrors.error(base->fPosition, "too many components in swizzle mask '" + fields + "'");
+        return nullptr;
+    }
+    return std::unique_ptr<Expression>(new Swizzle(std::move(base), swizzleComponents));
+}
+
+std::unique_ptr<Expression> IRGenerator::convertSuffixExpression(ASTSuffixExpression& expression) {
+    std::unique_ptr<Expression> base = this->convertExpression(*expression.fBase);
+    if (base == nullptr) {
+        return nullptr;
+    }
+    switch (expression.fSuffix->fKind) {
+        case ASTSuffix::kIndex_Kind:
+            return this->convertIndex(std::move(base), 
+                                      *((ASTIndexSuffix&) *expression.fSuffix).fExpression);
+        case ASTSuffix::kCall_Kind: {
+            auto rawParameters = &((ASTCallSuffix&) *expression.fSuffix).fParameters;

[dogben, 2016/06/23 15:25:09]

nit: s/parameters/arguments/ in this block

+            std::vector<std::unique_ptr<Expression>> parameters;
+            for (size_t i = 0; i < rawParameters->size(); i++) {
+                std::unique_ptr<Expression> converted = this->convertExpression(
+                                                                              *(*rawParameters)[i]);

[dogben, 2016/06/23 15:25:08]

nit: odd indentation

+                if (converted == nullptr) {
+                    return nullptr;
+                }
+                parameters.push_back(std::move(converted));
+            }
+            return this->call(expression.fPosition, std::move(base), std::move(parameters));
+        }
+        case ASTSuffix::kField_Kind: {
+            std::string field = ((ASTFieldSuffix&) *expression.fSuffix).fField;

[dogben, 2016/06/23 15:25:09]

nit: const ref?

+            switch (base->fType->kind()) {
+                case Type::kVector_Kind:
+                    return this->convertSwizzle(std::move(base), field);
+                case Type::kStruct_Kind:
+                    return this->convertField(std::move(base), field);
+                default:
+                    fErrors.error(base->fPosition, "cannot swizzle value of type '" + 
+                                                   base->fType->description() + "'");
+                    return nullptr;
+            }
+        }
+        case ASTSuffix::kPostIncrement_Kind:
+            if (!base->fType->isNumber()) {
+                fErrors.error(expression.fPosition, 
+                              "'++' cannot operate on '" + base->fType->description() + "'");
+                return nullptr;
+            }
+            this->markWrittenTo(*base);
+            return std::unique_ptr<Expression>(new PostfixExpression(std::move(base), 
+                                                                     Token::PLUSPLUS));
+        case ASTSuffix::kPostDecrement_Kind:
+            if (!base->fType->isNumber()) {
+                fErrors.error(expression.fPosition, 
+                              "'--' cannot operate on '" + base->fType->description() + "'");
+                return nullptr;
+            }
+            this->markWrittenTo(*base);
+            return std::unique_ptr<Expression>(new PostfixExpression(std::move(base), 
+                                                                     Token::MINUSMINUS));
+        default:
+            ABORT("unsupported suffix operator");
+    }
+}
+
+void IRGenerator::markReadFrom(std::shared_ptr<Variable> var) {
+    var->fIsReadFrom = true;
+}
+
+void IRGenerator::markWrittenTo(Expression& expr) {
+    switch (expr.fKind) {
+        case Expression::kVariableReference_Kind:
+            ((VariableReference&) expr).fVariable->fIsWrittenTo = true;
+            break;
+        case Expression::kFieldAccess_Kind:
+            this->markWrittenTo(*((FieldAccess&) expr).fBase);
+            break;
+        case Expression::kSwizzle_Kind:
+            this->markWrittenTo(*((Swizzle&) expr).fBase);
+            break;
+        case Expression::kIndex_Kind:
+            this->markWrittenTo(*((IndexExpression&) expr).fBase);
+            break;
+        default:
+            fErrors.error(expr.fPosition, "cannot assign to '" + expr.description() + "'");
+            break;
+    }
+}
+
+}