Migrate Type-cloning logic from the Inliner into Type.

The inliner needs to clone Types from one SymbolTable to another when
cloning blocks of code. However, it seems like a poor division of
responsibility for the inliner to need to know how to clone every Type
correctly. It makes more sense for this logic to exist within the Type
class itself.

Change-Id: I4a383d5e22d5084eb35992a0b5c24865d2030282
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/354662
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>

src/sksl/SkSLInliner.cpp

@@ -158,18 +158,6 @@ static bool contains_recursive_call(const FunctionDeclaration& funcDecl) {
     return ContainsRecursiveCall{}.visit(funcDecl);
 }
 
-static const Type* copy_if_needed(const Type* type, SymbolTable* symbolTable) {
-    if (type->isArray()) {
-        const Symbol* copiedType = (*symbolTable)[type->name()];
-        if (!copiedType) {
-            copiedType = symbolTable->add(Type::MakeArrayType(type->name(), type->componentType(),
-                                                              type->columns()));
-        }
-        return &copiedType->as<Type>();
-    }
-    return type;
-}
-
 static std::unique_ptr<Statement>* find_parent_statement(
         const std::vector<std::unique_ptr<Statement>*>& stmtStack) {
     SkASSERT(!stmtStack.empty());
@@ -348,12 +336,12 @@ std::unique_ptr<Expression> Inliner::inlineExpression(int offset,
     switch (expression.kind()) {
         case Expression::Kind::kBinary: {
             const BinaryExpression& binaryExpr = expression.as<BinaryExpression>();
-            const Type* type = copy_if_needed(&binaryExpr.type(), symbolTableForExpression);
+            return std::make_unique<BinaryExpression>(
-            return std::make_unique<BinaryExpression>(offset,
+                    offset,
-                                                      expr(binaryExpr.left()),
+                    expr(binaryExpr.left()),
-                                                      binaryExpr.getOperator(),
+                    binaryExpr.getOperator(),
-                                                      expr(binaryExpr.right()),
+                    expr(binaryExpr.right()),
-                                                      type);
+                    binaryExpr.type().clone(symbolTableForExpression));
         }
         case Expression::Kind::kBoolLiteral:
         case Expression::Kind::kIntLiteral:
@@ -361,8 +349,9 @@ std::unique_ptr<Expression> Inliner::inlineExpression(int offset,
             return expression.clone();
         case Expression::Kind::kConstructor: {
             const Constructor& constructor = expression.as<Constructor>();
-            const Type* type = copy_if_needed(&constructor.type(), symbolTableForExpression);
+            return std::make_unique<Constructor>(offset,
-            return std::make_unique<Constructor>(offset, type, argList(constructor.arguments()));
+                                                 constructor.type().clone(symbolTableForExpression),
+                                                 argList(constructor.arguments()));
         }
         case Expression::Kind::kExternalFunctionCall: {
             const ExternalFunctionCall& externalCall = expression.as<ExternalFunctionCall>();
@@ -377,8 +366,9 @@ std::unique_ptr<Expression> Inliner::inlineExpression(int offset,
         }
         case Expression::Kind::kFunctionCall: {
             const FunctionCall& funcCall = expression.as<FunctionCall>();
-            const Type* type = copy_if_needed(&funcCall.type(), symbolTableForExpression);
+            return std::make_unique<FunctionCall>(offset,
-            return std::make_unique<FunctionCall>(offset, type, &funcCall.function(),
+                                                  funcCall.type().clone(symbolTableForExpression),
+                                                  &funcCall.function(),
                                                   argList(funcCall.arguments()));
         }
         case Expression::Kind::kFunctionReference:
@@ -526,15 +516,13 @@ std::unique_ptr<Statement> Inliner::inlineStatement(int offset,
             }
 
             // For more complex functions, assign their result into a variable.
-            const Type* resultType = copy_if_needed(&resultExpr->get()->type(),
-                                                    symbolTableForStatement);
             auto assignment =
                     std::make_unique<ExpressionStatement>(std::make_unique<BinaryExpression>(
                             offset,
                             clone_with_ref_kind(**resultExpr, VariableReference::RefKind::kWrite),
                             Token::Kind::TK_EQ,
                             expr(r.expression()),
-                            resultType));
+                            (*resultExpr)->type().clone(symbolTableForStatement)));
 
             // Early returns are wrapped in a for loop; we need to synthesize a continue statement
             // to "leave" the function.
@@ -566,26 +554,26 @@ std::unique_ptr<Statement> Inliner::inlineStatement(int offset,
         case Statement::Kind::kVarDeclaration: {
             const VarDeclaration& decl = statement.as<VarDeclaration>();
             std::unique_ptr<Expression> initialValue = expr(decl.value());
-            int arraySize = decl.arraySize();
+            const Variable& variable = decl.var();
-            const Variable& old = decl.var();
+
             // We assign unique names to inlined variables--scopes hide most of the problems in this
             // regard, but see `InlinerAvoidsVariableNameOverlap` for a counterexample where unique
             // names are important.
-            auto name = std::make_unique<String>(fMangler.uniqueName(String(old.name()),
+            auto name = std::make_unique<String>(fMangler.uniqueName(variable.name(),
                                                                      symbolTableForStatement));
             const String* namePtr = symbolTableForStatement->takeOwnershipOfString(std::move(name));
-            const Type* baseTypePtr = copy_if_needed(&decl.baseType(), symbolTableForStatement);
+            const Variable* clonedVar = symbolTableForStatement->takeOwnershipOfSymbol(
-            const Type* typePtr = copy_if_needed(&old.type(), symbolTableForStatement);
-            const Variable* clone = symbolTableForStatement->takeOwnershipOfSymbol(
                     std::make_unique<Variable>(offset,
-                                               &old.modifiers(),
+                                               &variable.modifiers(),
                                                namePtr->c_str(),
-                                               typePtr,
+                                               variable.type().clone(symbolTableForStatement),
                                                isBuiltinCode,
-                                               old.storage(),
+                                               variable.storage(),
                                                initialValue.get()));
-            (*varMap)[&old] = std::make_unique<VariableReference>(offset, clone);
+            (*varMap)[&variable] = std::make_unique<VariableReference>(offset, clonedVar);
-            return std::make_unique<VarDeclaration>(clone, baseTypePtr, arraySize,
+            return std::make_unique<VarDeclaration>(clonedVar,
+                                                    decl.baseType().clone(symbolTableForStatement),
+                                                    decl.arraySize(),
                                                     std::move(initialValue));
         }
         default:

src/sksl/ir/SkSLType.cpp

@@ -5,7 +5,10 @@
  * found in the LICENSE file.
  */
 
+#include "src/sksl/ir/SkSLType.h"
+
 #include "src/sksl/SkSLContext.h"
+#include "src/sksl/ir/SkSLSymbolTable.h"
 #include "src/sksl/ir/SkSLType.h"
 
 namespace SkSL {
@@ -207,4 +210,31 @@ const Type& Type::toCompound(const Context& context, int columns, int rows) cons
     return *context.fTypes.fVoid;
 }
 
+const Type* Type::clone(SymbolTable* symbolTable) const {
+    // Many types are built-ins, and exist in every SymbolTable by default.
+    if (this->isInBuiltinTypes()) {
+        return this;
+    }
+    // Even if the type isn't a built-in, it might already exist in the SymbolTable.
+    const Symbol* clonedSymbol = (*symbolTable)[this->name()];
+    if (clonedSymbol != nullptr) {
+        const Type& clonedType = clonedSymbol->as<Type>();
+        SkASSERT(clonedType.typeKind() == this->typeKind());
+        return &clonedType;
+    }
+    // This type actually needs to be cloned into the destination SymbolTable.
+    switch (this->typeKind()) {
+        case TypeKind::kArray:
+            return symbolTable->add(Type::MakeArrayType(this->name(), this->componentType(),
+                                                        this->columns()));
+
+        case TypeKind::kStruct:
+        case TypeKind::kEnum:
+            // TODO: implement Type cloning for structs and enums.
+        default:
+            SkDEBUGFAILF("don't know how to clone type '%s'", this->description().c_str());
+            return nullptr;
+    }
+}
+
 }  // namespace SkSL

src/sksl/ir/SkSLType.h

@@ -116,6 +116,19 @@ public:
                                               columns));
     }
 
+    /** Creates a clone of this Type, if needed, and inserts it into a different symbol table. */
+    const Type* clone(SymbolTable* symbolTable) const;
+
+    /**
+     * Returns true if this type is known to come from BuiltinTypes. If this returns true, the Type
+     * will always be available in the root SymbolTable and never needs to be copied to migrate an
+     * Expression from one location to another. If it returns false, the Type might not exist in a
+     * separate SymbolTable and you'll need to consider copying it.
+     */
+    bool isInBuiltinTypes() const {
+        return !(this->isArray() || this->isStruct() || this->isEnum());
+    }
+
     String displayName() const {
         return this->scalarTypeForLiteral().name();
     }