Reorganization of IR generator's API and interaction with compiler

- Move all of IR generator's fields private (except for fContext, which
  is used ~everywhere).
- Eliminate start() and finish(), fold this logic into convertProgram.
  The division of what was set/reset in different places was pretty
  arbitrary. Now, convertProgram does everything. Along that line, have
  it actually return the "outputs" as an IRBundle (a small collection of
  the things that the compiler needs). This seems better than the
  compiler ripping out IR generator's internals.
- IR generator's POD field initialization was a mix of in-class and
  constructor. Move all the constant initialization to declarations.
- No need to look up sk_PerVertex at start (or convertProgram) time, so
  remove fSkPerVertex, and just do the lookup when we're about to use
  it.
- IRGenerator::convertProgram is fairly long now, but all the code is in
  one place. You don't have to think about the order that three
  different member functions are called (along with the caller mutating
  the internal state between those three calls).

- In the compiler, add an AutoSource helper to manage changing and
  restoring the fSource pointer everywhere.
- Rename the loadXXXIntrinsics functions to loadXXXModule, have them
  return the module, and wrap the whole thing up in a single
  moduleForProgramKind() helper.

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

Author: brianosman

src/sksl/SkSLCompiler.cpp

@@ -70,14 +70,26 @@
 
 namespace SkSL {
 
+class AutoSource {
+public:
+    AutoSource(Compiler* compiler, const String* source)
+            : fCompiler(compiler), fOldSource(fCompiler->fSource) {
+        fCompiler->fSource = source;
+    }
+
+    ~AutoSource() { fCompiler->fSource = fOldSource; }
+
+    Compiler* fCompiler;
+    const String* fOldSource;
+};
+
 Compiler::Compiler(Flags flags)
 : fFlags(flags)
 , fContext(std::make_shared<Context>())
 , fErrorCount(0) {
     fRootSymbolTable = std::make_shared<SymbolTable>(this);
-    fIRGenerator =
-            std::make_unique<IRGenerator>(fContext.get(), &fInliner, fRootSymbolTable, *this);
-    #define ADD_TYPE(t) fRootSymbolTable->addWithoutOwnership(fContext->f ## t ## _Type.get())
+    fIRGenerator = std::make_unique<IRGenerator>(fContext.get(), &fInliner, *this);
+#define ADD_TYPE(t) fRootSymbolTable->addWithoutOwnership(fContext->f##t##_Type.get())
     ADD_TYPE(Void);
     ADD_TYPE(Float);
     ADD_TYPE(Float2);
@@ -221,37 +233,52 @@ Compiler::Compiler(Flags flags)
 
 Compiler::~Compiler() {}
 
-void Compiler::loadGeometryIntrinsics() {
+const ParsedModule& Compiler::loadGeometryModule() {
     if (!fGeometryModule.fSymbols) {
         fGeometryModule = this->parseModule(Program::kGeometry_Kind, MODULE_DATA(geom), fGPUModule);
     }
+    return fGeometryModule;
 }
 
-void Compiler::loadFPIntrinsics() {
+const ParsedModule& Compiler::loadFPModule() {
     if (!fFPModule.fSymbols) {
         fFPModule =
                 this->parseModule(Program::kFragmentProcessor_Kind, MODULE_DATA(fp), fGPUModule);
     }
+    return fFPModule;
 }
 
-void Compiler::loadPipelineIntrinsics() {
+const ParsedModule& Compiler::loadPipelineModule() {
     if (!fPipelineModule.fSymbols) {
         fPipelineModule =
                 this->parseModule(Program::kPipelineStage_Kind, MODULE_DATA(pipeline), fGPUModule);
     }
+    return fPipelineModule;
 }
 
-void Compiler::loadInterpreterIntrinsics() {
+const ParsedModule& Compiler::loadInterpreterModule() {
     if (!fInterpreterModule.fSymbols) {
         fInterpreterModule =
                 this->parseModule(Program::kGeneric_Kind, MODULE_DATA(interp), fRootModule);
     }
+    return fInterpreterModule;
+}
+
+const ParsedModule& Compiler::moduleForProgramKind(Program::Kind kind) {
+    switch (kind) {
+        case Program::kVertex_Kind:            return fVertexModule;                 break;
+        case Program::kFragment_Kind:          return fFragmentModule;               break;
+        case Program::kGeometry_Kind:          return this->loadGeometryModule();    break;
+        case Program::kFragmentProcessor_Kind: return this->loadFPModule();          break;
+        case Program::kPipelineStage_Kind:     return this->loadPipelineModule();    break;
+        case Program::kGeneric_Kind:           return this->loadInterpreterModule(); break;
+    }
+    SkUNREACHABLE;
 }
 
 LoadedModule Compiler::loadModule(Program::Kind kind,
                                   ModuleData data,
                                   std::shared_ptr<SymbolTable> base) {
-    LoadedModule module;
     if (!base) {
         base = fRootSymbolTable;
     }
@@ -266,26 +293,26 @@ LoadedModule Compiler::loadModule(Program::Kind kind,
         abort();
     }
     const String* source = fRootSymbolTable->takeOwnershipOfString(std::move(text));
-    fSource = source;
+    AutoSource as(this, source);
     Program::Settings settings;
     SkASSERT(fIRGenerator->fCanInline);
     fIRGenerator->fCanInline = false;
-    fIRGenerator->start(&settings, {base, /*fIntrinsics=*/nullptr}, /*builtin=*/true);
-    fIRGenerator->convertProgram(kind, source->c_str(), source->length(), &module.fElements);
+    ParsedModule baseModule = {base, /*fIntrinsics=*/nullptr};
+    IRGenerator::IRBundle ir = fIRGenerator->convertProgram(
+            kind, &settings, baseModule, /*isBuiltinCode=*/true, source->c_str(), source->length(),
+            /*externalValues=*/nullptr);
+    LoadedModule module = { std::move(ir.fSymbolTable), std::move(ir.fElements) };
     fIRGenerator->fCanInline = true;
     if (this->fErrorCount) {
         printf("Unexpected errors: %s\n", this->fErrorText.c_str());
         SkDEBUGFAILF("%s %s\n", data.fPath, this->fErrorText.c_str());
     }
-    module.fSymbols = fIRGenerator->fSymbolTable;
-    fSource = nullptr;
-    fModifiers.push_back(fIRGenerator->releaseModifiers());
-    fIRGenerator->finish();
+    fModifiers.push_back(std::move(ir.fModifiers));
 #else
     SkASSERT(data.fData && (data.fSize != 0));
     Rehydrator rehydrator(fContext.get(), fIRGenerator->fModifiers.get(), base, this,
                           data.fData, data.fSize);
-    module = { rehydrator.symbolTable(), rehydrator.elements() };
+    LoadedModule module = { rehydrator.symbolTable(), rehydrator.elements() };
     fModifiers.push_back(fIRGenerator->releaseModifiers());
 #endif
 
@@ -1499,50 +1526,24 @@ std::unique_ptr<Program> Compiler::convertProgram(
     fErrorText = "";
     fErrorCount = 0;
     fInliner.reset(fContext.get(), fIRGenerator->fModifiers.get(), &settings);
-    std::vector<std::unique_ptr<ProgramElement>> elements;
-    switch (kind) {
-        case Program::kVertex_Kind:
-            fIRGenerator->start(&settings, fVertexModule);
-            break;
-        case Program::kFragment_Kind:
-            fIRGenerator->start(&settings, fFragmentModule);
-            break;
-        case Program::kGeometry_Kind:
-            this->loadGeometryIntrinsics();
-            fIRGenerator->start(&settings, fGeometryModule);
-            break;
-        case Program::kFragmentProcessor_Kind:
-            this->loadFPIntrinsics();
-            fIRGenerator->start(&settings, fFPModule);
-            break;
-        case Program::kPipelineStage_Kind:
-            this->loadPipelineIntrinsics();
-            fIRGenerator->start(&settings, fPipelineModule);
-            break;
-        case Program::kGeneric_Kind:
-            this->loadInterpreterIntrinsics();
-            fIRGenerator->start(&settings, fInterpreterModule);
-            break;
-    }
-    if (externalValues) {
-        // Add any external values to the symbol table. IRGenerator::start() has pushed a table, so
-        // we're only making these visible to the current Program.
-        for (const auto& ev : *externalValues) {
-            fIRGenerator->fSymbolTable->addWithoutOwnership(ev.get());
-        }
-    }
+
+    // Not using AutoSource, because caller is likely to call errorText() if we fail to compile
     std::unique_ptr<String> textPtr(new String(std::move(text)));
     fSource = textPtr.get();
-    fIRGenerator->convertProgram(kind, textPtr->c_str(), textPtr->size(), &elements);
+
+    const ParsedModule& baseModule = this->moduleForProgramKind(kind);
+
+    IRGenerator::IRBundle ir =
+            fIRGenerator->convertProgram(kind, &settings, baseModule, /*isBuiltinCode=*/false,
+                                         textPtr->c_str(), textPtr->size(), externalValues);
     auto result = std::make_unique<Program>(kind,
                                             std::move(textPtr),
                                             settings,
                                             fContext,
-                                            std::move(elements),
-                                            fIRGenerator->releaseModifiers(),
-                                            fIRGenerator->fSymbolTable,
-                                            fIRGenerator->fInputs);
-    fIRGenerator->finish();
+                                            std::move(ir.fElements),
+                                            std::move(ir.fModifiers),
+                                            std::move(ir.fSymbolTable),
+                                            ir.fInputs);
     if (fErrorCount) {
         return nullptr;
     }
@@ -1617,10 +1618,9 @@ bool Compiler::optimize(Program& program) {
 bool Compiler::toSPIRV(Program& program, OutputStream& out) {
 #ifdef SK_ENABLE_SPIRV_VALIDATION
     StringStream buffer;
-    fSource = program.fSource.get();
+    AutoSource as(this, program.fSource.get());
     SPIRVCodeGenerator cg(fContext.get(), &program, this, &buffer);
     bool result = cg.generateCode();
-    fSource = nullptr;
     if (result) {
         spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_0);
         const String& data = buffer.str();
@@ -1635,10 +1635,9 @@ bool Compiler::toSPIRV(Program& program, OutputStream& out) {
         out.write(data.c_str(), data.size());
     }
 #else
-    fSource = program.fSource.get();
+    AutoSource as(this, program.fSource.get());
     SPIRVCodeGenerator cg(fContext.get(), &program, this, &out);
     bool result = cg.generateCode();
-    fSource = nullptr;
 #endif
     return result;
 }
@@ -1653,10 +1652,9 @@ bool Compiler::toSPIRV(Program& program, String* out) {
 }
 
 bool Compiler::toGLSL(Program& program, OutputStream& out) {
-    fSource = program.fSource.get();
+    AutoSource as(this, program.fSource.get());
     GLSLCodeGenerator cg(fContext.get(), &program, this, &out);
     bool result = cg.generateCode();
-    fSource = nullptr;
     return result;
 }
 
@@ -1695,18 +1693,16 @@ bool Compiler::toMetal(Program& program, String* out) {
 
 #if defined(SKSL_STANDALONE) || GR_TEST_UTILS
 bool Compiler::toCPP(Program& program, String name, OutputStream& out) {
-    fSource = program.fSource.get();
+    AutoSource as(this, program.fSource.get());
     CPPCodeGenerator cg(fContext.get(), &program, this, name, &out);
     bool result = cg.generateCode();
-    fSource = nullptr;
     return result;
 }
 
 bool Compiler::toH(Program& program, String name, OutputStream& out) {
-    fSource = program.fSource.get();
+    AutoSource as(this, program.fSource.get());
     HCodeGenerator cg(fContext.get(), &program, this, name, &out);
     bool result = cg.generateCode();
-    fSource = nullptr;
     return result;
 }
 #endif // defined(SKSL_STANDALONE) || GR_TEST_UTILS
@@ -1715,11 +1711,10 @@ bool Compiler::toH(Program& program, String name, OutputStream& out) {
 
 #if !defined(SKSL_STANDALONE) && SK_SUPPORT_GPU
 bool Compiler::toPipelineStage(Program& program, PipelineStageArgs* outArgs) {
-    fSource = program.fSource.get();
+    AutoSource as(this, program.fSource.get());
     StringStream buffer;
     PipelineStageCodeGenerator cg(fContext.get(), &program, this, &buffer, outArgs);
     bool result = cg.generateCode();
-    fSource = nullptr;
     if (result) {
         outArgs->fCode = buffer.str();
     }
@@ -1729,11 +1724,10 @@ bool Compiler::toPipelineStage(Program& program, PipelineStageArgs* outArgs) {
 
 std::unique_ptr<ByteCode> Compiler::toByteCode(Program& program) {
 #if defined(SK_ENABLE_SKSL_INTERPRETER)
-    fSource = program.fSource.get();
+    AutoSource as(this, program.fSource.get());
     std::unique_ptr<ByteCode> result(new ByteCode());
     ByteCodeGenerator cg(fContext.get(), &program, this, result.get());
     bool success = cg.generateCode();
-    fSource = nullptr;
     if (success) {
         return result;
     }

src/sksl/SkSLCompiler.h

@@ -206,18 +206,18 @@ class SK_API Compiler : public ErrorReporter {
     ParsedModule parseModule(Program::Kind kind, ModuleData data, const ParsedModule& base);
 
 private:
-    void loadFPIntrinsics();
-    void loadGeometryIntrinsics();
-    void loadInterpreterIntrinsics();
-    void loadPipelineIntrinsics();
+    const ParsedModule& loadFPModule();
+    const ParsedModule& loadGeometryModule();
+    const ParsedModule& loadInterpreterModule();
+    const ParsedModule& loadPipelineModule();
+
+    const ParsedModule& moduleForProgramKind(Program::Kind kind);
 
     void addDefinition(const Expression* lvalue, std::unique_ptr<Expression>* expr,
                        DefinitionMap* definitions);
-
     void addDefinitions(const BasicBlock::Node& node, DefinitionMap* definitions);
 
     void scanCFG(CFG* cfg, BlockId block, SkBitSet* processedSet);
-
     void computeDataFlow(CFG* cfg);
 
     /**
@@ -276,6 +276,8 @@ class SK_API Compiler : public ErrorReporter {
     std::shared_ptr<Context> fContext;
     int fErrorCount;
     String fErrorText;
+
+    friend class AutoSource;
 };
 
 #if !defined(SKSL_STANDALONE) && SK_SUPPORT_GPU