Support variables in the intrinsic map, clone them into Programs

As a first step, convert sksl_pipeline.sksl to an IntrinsicMap (rather
than inherited element list). This makes the new code operate on
sk_FragCoord (which was previously being shared by all runtime effect
programs).

The new unit test angered TSAN, and now runs without complaint.

Also finish converting the .fp intrinsics over, so those don't need an
inherited element list either. And while doing that, refactor that
parsing to match all of the others. FP was uniquely implementing
processIncludeFile itself, rather than reusing the pattern of other
pre-include parsing.

The meat of the CL is the subtle changes in Compiler, and the logic in
cloneBuiltinVariables. Note that we need to clone the global variable
declaration element (because one of the goals is to get rid of shared
and inherited program elements), but also the variable itself (and the
new copy needs to live in the program's symbol table).

Bug: skia:10589
Bug: skia:10679
Bug: skia:10680
Change-Id: Ied352f8434dac2b8eacb4e515b014b6af7b57d20
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319023
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>

src/sksl/SkSLCompiler.cpp

@@ -88,6 +88,17 @@ static void grab_intrinsics(std::vector<std::unique_ptr<ProgramElement>>* src,
                 iter = src->erase(iter);
                 break;
             }
+            case ProgramElement::Kind::kVar: {
+                // TODO: For now, we only support one variable per declaration. We map names to
+                // declarations, and each declaration pulls in all of it's variables, so this rule
+                // ensures that we never pull in variables that aren't actually used.
+                const VarDeclarations& vd = element->as<VarDeclarations>();
+                SkASSERT(vd.fVars.size() == 1);
+                const Variable* var = vd.fVars[0]->as<VarDeclaration>().fVar;
+                target->insertOrDie(var->fName, std::move(element));
+                iter = src->erase(iter);
+                break;
+            }
             default:
                 // Unsupported element, leave it in the list.
                 ++iter;
@@ -108,6 +119,7 @@ static void reset_call_counts(std::vector<std::unique_ptr<ProgramElement>>* src)
 Compiler::Compiler(Flags flags)
 : fGPUIntrinsics(std::make_unique<IRIntrinsicMap>(/*parent=*/nullptr))
 , fInterpreterIntrinsics(std::make_unique<IRIntrinsicMap>(/*parent=*/nullptr))
+, fPipelineIntrinsics(std::make_unique<IRIntrinsicMap>(fGPUIntrinsics.get()))
 , fFlags(flags)
 , fContext(std::make_shared<Context>())
 , fErrorCount(0) {
@@ -309,22 +321,46 @@ void Compiler::loadGeometryIntrinsics() {
     #endif
 }
 
+void Compiler::loadFPIntrinsics() {
+    if (fFPSymbolTable) {
+        return;
+    }
+    fFPIntrinsics = std::make_unique<IRIntrinsicMap>(fGPUIntrinsics.get());
+    std::vector<std::unique_ptr<ProgramElement>> fpElements;
+    #if !SKSL_STANDALONE
+        {
+            Rehydrator rehydrator(fContext.get(), fGpuSymbolTable, this, SKSL_INCLUDE_sksl_fp,
+                                  SKSL_INCLUDE_sksl_fp_LENGTH);
+            fFPSymbolTable = rehydrator.symbolTable();
+            fpElements = rehydrator.elements();
+        }
+    #else
+        this->processIncludeFile(Program::kFragmentProcessor_Kind, SKSL_FP_INCLUDE, fGpuSymbolTable,
+                                 &fpElements, &fFPSymbolTable);
+    #endif
+    grab_intrinsics(&fpElements, fFPIntrinsics.get());
+    SkASSERT(fpElements.empty());
+}
+
 void Compiler::loadPipelineIntrinsics() {
     if (fPipelineSymbolTable) {
         return;
     }
+    std::vector<std::unique_ptr<ProgramElement>> pipelineIntrinics;
     #if !SKSL_STANDALONE
         {
             Rehydrator rehydrator(fContext.get(), fGpuSymbolTable, this,
                                   SKSL_INCLUDE_sksl_pipeline,
                                   SKSL_INCLUDE_sksl_pipeline_LENGTH);
             fPipelineSymbolTable = rehydrator.symbolTable();
-            fPipelineInclude = rehydrator.elements();
+            pipelineIntrinics = rehydrator.elements();
         }
     #else
         this->processIncludeFile(Program::kPipelineStage_Kind, SKSL_PIPELINE_INCLUDE,
-                                 fGpuSymbolTable, &fPipelineInclude, &fPipelineSymbolTable);
+                                 fGpuSymbolTable, &pipelineIntrinics, &fPipelineSymbolTable);
     #endif
+    grab_intrinsics(&pipelineIntrinics, fPipelineIntrinsics.get());
+    SkASSERT(pipelineIntrinics.empty());
 }
 
 void Compiler::loadInterpreterIntrinsics() {
@@ -1597,46 +1633,17 @@ std::unique_ptr<Program> Compiler::convertProgram(
             fIRGenerator->fIntrinsics = fGPUIntrinsics.get();
             fIRGenerator->start(&settings, fGeometrySymbolTable, inherited);
             break;
-        case Program::kFragmentProcessor_Kind: {
+        case Program::kFragmentProcessor_Kind:
-#if !SKSL_STANDALONE
+            this->loadFPIntrinsics();
-            {
-                Rehydrator rehydrator(fContext.get(), fGpuSymbolTable, this,
-                                      SKSL_INCLUDE_sksl_fp,
-                                      SKSL_INCLUDE_sksl_fp_LENGTH);
-                fFPSymbolTable = rehydrator.symbolTable();
-                fFPInclude = rehydrator.elements();
-            }
-            fFPIntrinsics = std::make_unique<IRIntrinsicMap>(fGPUIntrinsics.get());
-            grab_intrinsics(&fFPInclude, fFPIntrinsics.get());
-
-            inherited = &fFPInclude;
-            fIRGenerator->fIntrinsics = fFPIntrinsics.get();
-            fIRGenerator->start(&settings, fFPSymbolTable, inherited);
-            break;
-#else
             inherited = nullptr;
-            fIRGenerator->start(&settings, fGpuSymbolTable, /*inherited=*/nullptr,
+            fIRGenerator->fIntrinsics = fFPIntrinsics.get();
-                                /*builtin=*/true);
+            fIRGenerator->start(&settings, fFPSymbolTable, /*inherited=*/nullptr);
-            fIRGenerator->fIntrinsics = fGPUIntrinsics.get();
-            std::ifstream in(SKSL_FP_INCLUDE);
-            std::string stdText{std::istreambuf_iterator<char>(in),
-                                std::istreambuf_iterator<char>()};
-            if (in.rdstate()) {
-                printf("error reading %s\n", SKSL_FP_INCLUDE);
-                abort();
-            }
-            const String* source = fRootSymbolTable->takeOwnershipOfString(
-                                                         std::make_unique<String>(stdText.c_str()));
-            fIRGenerator->convertProgram(kind, source->c_str(), source->length(), &elements);
-            fIRGenerator->fIsBuiltinCode = false;
             break;
-#endif
-        }
         case Program::kPipelineStage_Kind:
             this->loadPipelineIntrinsics();
-            inherited = &fPipelineInclude;
+            inherited = nullptr;
-            fIRGenerator->fIntrinsics = fGPUIntrinsics.get();
+            fIRGenerator->fIntrinsics = fPipelineIntrinsics.get();
-            fIRGenerator->start(&settings, fPipelineSymbolTable, inherited);
+            fIRGenerator->start(&settings, fPipelineSymbolTable, /*inherited=*/nullptr);
             break;
         case Program::kGeneric_Kind:
             this->loadInterpreterIntrinsics();

src/sksl/SkSLCompiler.h

@@ -180,10 +180,9 @@ public:
                             std::shared_ptr<SymbolTable>* outSymbolTable);
 
 private:
+    void loadFPIntrinsics();
     void loadGeometryIntrinsics();
-
     void loadInterpreterIntrinsics();
-
     void loadPipelineIntrinsics();
 
     void addDefinition(const Expression* lvalue, std::unique_ptr<Expression>* expr,
@@ -242,10 +241,10 @@ private:
     std::shared_ptr<SymbolTable> fFragmentSymbolTable;
     std::vector<std::unique_ptr<ProgramElement>> fGeometryInclude;
     std::shared_ptr<SymbolTable> fGeometrySymbolTable;
-    std::vector<std::unique_ptr<ProgramElement>> fPipelineInclude;
-    std::shared_ptr<SymbolTable> fPipelineSymbolTable;
 
-    std::vector<std::unique_ptr<ProgramElement>> fFPInclude;
+    std::shared_ptr<SymbolTable> fPipelineSymbolTable;
+    std::unique_ptr<IRIntrinsicMap> fPipelineIntrinsics;
+
     std::shared_ptr<SymbolTable> fFPSymbolTable;
     std::unique_ptr<IRIntrinsicMap> fFPIntrinsics;
 

src/sksl/SkSLIRGenerator.cpp

@@ -2824,6 +2824,75 @@ bool IRGenerator::setRefKind(Expression& expr, VariableReference::RefKind kind)
     return true;
 }
 
+void IRGenerator::cloneBuiltinVariables() {
+    class BuiltinVariableRemapper : public ProgramWriter {
+    public:
+        BuiltinVariableRemapper(IRGenerator* generator) : fGenerator(generator) {}
+
+        bool visitExpression(Expression& e) override {
+            // Look for references to builtin variables.
+            if (e.is<VariableReference>() && e.as<VariableReference>().fVariable->fBuiltin) {
+                const Variable* sharedVar = e.as<VariableReference>().fVariable;
+
+                // If this is the *first* time we've seen this builtin, findAndInclude will return
+                // the corresponding ProgramElement.
+                if (const ProgramElement* sharedDecls =
+                            fGenerator->fIntrinsics->findAndInclude(sharedVar->fName)) {
+                    // Clone the VarDeclarations ProgramElement that declares this variable
+                    std::unique_ptr<ProgramElement> clonedDecls = sharedDecls->clone();
+                    SkASSERT(clonedDecls->is<VarDeclarations>());
+                    VarDeclarations& varDecls = clonedDecls->as<VarDeclarations>();
+                    SkASSERT(varDecls.fVars.size() == 1);
+                    VarDeclaration& varDecl = varDecls.fVars.front()->as<VarDeclaration>();
+
+                    // Now clone the Variable, and add the clone to the Program's symbol table.
+                    // Any initial value expression was cloned as part of the VarDeclarations,
+                    // so we're pointing at a Program-owned expression.
+                    const Variable* clonedVar = fGenerator->fSymbolTable->takeOwnershipOfSymbol(
+                            std::make_unique<Variable>(sharedVar->fOffset, sharedVar->fModifiers,
+                                                       sharedVar->fName, &sharedVar->type(),
+                                                       /*builtin=*/false, sharedVar->fStorage,
+                                                       varDecl.fValue.get()));
+
+                    // Go back and update the VarDeclaration to point at the cloned Variable.
+                    varDecl.fVar = clonedVar;
+
+                    // Remember this new re-mapping...
+                    fRemap.insert({sharedVar, clonedVar});
+
+                    // Add the VarDeclarations to this Program
+                    fNewElements.push_back(std::move(clonedDecls));
+                }
+
+                // TODO: SkASSERT(found), once all pre-includes are converted?
+                auto found = fRemap.find(sharedVar);
+                if (found != fRemap.end()) {
+                    e.as<VariableReference>().setVariable(found->second);
+                }
+            }
+
+            return INHERITED::visitExpression(e);
+        }
+
+        IRGenerator* fGenerator;
+        std::unordered_map<const Variable*, const Variable*> fRemap;
+        std::vector<std::unique_ptr<ProgramElement>> fNewElements;
+
+        using INHERITED = ProgramWriter;
+        using INHERITED::visitProgramElement;
+    };
+
+    if (!fIsBuiltinCode) {
+        BuiltinVariableRemapper remapper(this);
+        for (auto& e : *fProgramElements) {
+            remapper.visitProgramElement(*e);
+        }
+        fProgramElements->insert(fProgramElements->begin(),
+                                 std::make_move_iterator(remapper.fNewElements.begin()),
+                                 std::make_move_iterator(remapper.fNewElements.end()));
+    }
+}
+
 void IRGenerator::convertProgram(Program::Kind kind,
                                  const char* text,
                                  size_t length,
@@ -2892,6 +2961,9 @@ void IRGenerator::convertProgram(Program::Kind kind,
         }
     }
 
+    // Any variables defined in the pre-includes need to be cloned into the Program
+    this->cloneBuiltinVariables();
+
     // Do a final pass looking for dangling FunctionReference or TypeReference expressions
     class FindIllegalExpressions : public ProgramVisitor {
     public:

src/sksl/SkSLIRGenerator.h

@@ -199,6 +199,7 @@ private:
     bool setRefKind(Expression& expr, VariableReference::RefKind kind);
     bool getConstantInt(const Expression& value, int64_t* out);
     void copyIntrinsicIfNeeded(const FunctionDeclaration& function);
+    void cloneBuiltinVariables();
 
     Inliner* fInliner = nullptr;
     std::unique_ptr<ASTFile> fFile;

src/sksl/ir/SkSLVariableReference.cpp

@@ -50,6 +50,12 @@ void VariableReference::setRefKind(RefKind refKind) {
     this->incrementRefs();
 }
 
+void VariableReference::setVariable(const Variable* variable) {
+    this->decrementRefs();
+    fVariable = variable;
+    this->incrementRefs();
+}
+
 std::unique_ptr<Expression> VariableReference::constantPropagate(const IRGenerator& irGenerator,
                                                                  const DefinitionMap& definitions) {
     if (fRefKind != kRead_RefKind) {

src/sksl/ir/SkSLVariableReference.h

@@ -46,6 +46,7 @@ struct VariableReference : public Expression {
     }
 
     void setRefKind(RefKind refKind);
+    void setVariable(const Variable* variable);
 
     bool hasProperty(Property property) const override {
         switch (property) {

tests/SkRuntimeEffectTest.cpp

@@ -18,6 +18,7 @@
 #include "tests/Test.h"
 
 #include <algorithm>
+#include <thread>
 
 DEF_TEST(SkRuntimeEffectInvalid, r) {
     auto test = [r](const char* hdr, const char* body, const char* expected) {
@@ -289,3 +290,23 @@ DEF_TEST(SkRuntimeShaderBuilderReuse, r) {
     b.uniform("x") = 1.0f;
     auto shader_1 = b.makeShader(nullptr, true);
 }
+
+DEF_TEST(SkRuntimeEffectThreaded, r) {
+    // SkRuntimeEffect uses a single compiler instance, but it's mutex locked.
+    // This tests that we can safely use it from more than one thread, and also
+    // that programs don't refer to shared structures owned by the compiler.
+    // skbug.com/10589
+    static constexpr char kSource[] = "half4 main() { return sk_FragCoord.xyxy; }";
+
+    std::thread threads[16];
+    for (auto& thread : threads) {
+        thread = std::thread([r]() {
+            auto [effect, error] = SkRuntimeEffect::Make(SkString(kSource));
+            REPORTER_ASSERT(r, effect);
+        });
+    }
+
+    for (auto& thread : threads) {
+        thread.join();
+    }
+}