Various cleanup related to symbol tables
- Remove a spurious symbol table inserted by convertProgram. start()
already pushes a symbol table, and this was pushing a second one,
which didn't seem necessary. (The Parser can inject symbols for types
it discovers, but I can't justify those needing to be in a different
table than the rest of the program elements?)
- The convertProgram one had a comment indicating that it was popped by
the Compiler. That wasn't true, so this gets us one step closer to
balance.
- The one in start() is meant to be balanced by a pop in finish(), but
no one ever called finish(). Add that call in, and also rearrange
things so that the base symbol table is a parameter to start(), rather
than just setting it on the IR generator. (There's more of this
pattern around, but I wanted to limit the scope of this CL).
- When dehydrating the include files, we had logic to work around the
extra symbol table (absorbing the symbols) - that's not needed now.
- Simplify some other logic in processIncludeFile (no need to make so
many string copies). Always just put the incoming include file strings
into the root table, also. It's largely irrelevant where they go.
Change-Id: I18d897af3d5fa6506e11024beb9bb70e6cc5b538
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319038
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/src/sksl/SkSLCompiler.cpp b/src/sksl/SkSLCompiler.cpp
index 8deb632..f38da67 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -334,22 +334,14 @@
std::vector<std::unique_ptr<ProgramElement>>* outElements,
std::shared_ptr<SymbolTable>* outSymbolTable) {
std::ifstream in(path);
- std::string stdText{std::istreambuf_iterator<char>(in),
- std::istreambuf_iterator<char>()};
+ std::unique_ptr<String> text = std::make_unique<String>(std::istreambuf_iterator<char>(in),
+ std::istreambuf_iterator<char>());
if (in.rdstate()) {
printf("error reading %s\n", path);
abort();
}
- if (!base) {
- base = fIRGenerator->fSymbolTable;
- }
- SkASSERT(base);
- const String* source = base->takeOwnershipOfString(std::make_unique<String>(stdText.c_str()));
+ const String* source = fRootSymbolTable->takeOwnershipOfString(std::move(text));
fSource = source;
- std::shared_ptr<SymbolTable> old = fIRGenerator->fSymbolTable;
- if (base) {
- fIRGenerator->fSymbolTable = std::move(base);
- }
Program::Settings settings;
#if !defined(SKSL_STANDALONE) & SK_SUPPORT_GPU
GrContextOptions opts;
@@ -358,7 +350,7 @@
#endif
SkASSERT(fIRGenerator->fCanInline);
fIRGenerator->fCanInline = false;
- fIRGenerator->start(&settings, nullptr, true);
+ fIRGenerator->start(&settings, base ? base : fRootSymbolTable, nullptr, true);
fIRGenerator->convertProgram(kind, source->c_str(), source->length(), outElements);
fIRGenerator->fCanInline = true;
if (this->fErrorCount) {
@@ -369,7 +361,7 @@
#ifdef SK_DEBUG
fSource = nullptr;
#endif
- fIRGenerator->fSymbolTable = std::move(old);
+ fIRGenerator->finish();
}
// add the definition created by assigning to the lvalue to the definition set
@@ -1582,22 +1574,19 @@
switch (kind) {
case Program::kVertex_Kind:
inherited = &fVertexInclude;
- fIRGenerator->fSymbolTable = fVertexSymbolTable;
fIRGenerator->fIntrinsics = fGPUIntrinsics.get();
- fIRGenerator->start(&settings, inherited);
+ fIRGenerator->start(&settings, fVertexSymbolTable, inherited);
break;
case Program::kFragment_Kind:
inherited = &fFragmentInclude;
- fIRGenerator->fSymbolTable = fFragmentSymbolTable;
fIRGenerator->fIntrinsics = fGPUIntrinsics.get();
- fIRGenerator->start(&settings, inherited);
+ fIRGenerator->start(&settings, fFragmentSymbolTable, inherited);
break;
case Program::kGeometry_Kind:
this->loadGeometryIntrinsics();
inherited = &fGeometryInclude;
- fIRGenerator->fSymbolTable = fGeometrySymbolTable;
fIRGenerator->fIntrinsics = fGPUIntrinsics.get();
- fIRGenerator->start(&settings, inherited);
+ fIRGenerator->start(&settings, fGeometrySymbolTable, inherited);
break;
case Program::kFragmentProcessor_Kind: {
#if !SKSL_STANDALONE
@@ -1612,14 +1601,13 @@
grab_intrinsics(&fFPInclude, fFPIntrinsics.get());
inherited = &fFPInclude;
- fIRGenerator->fSymbolTable = fFPSymbolTable;
fIRGenerator->fIntrinsics = fFPIntrinsics.get();
- fIRGenerator->start(&settings, inherited);
+ fIRGenerator->start(&settings, fFPSymbolTable, inherited);
break;
#else
inherited = nullptr;
- fIRGenerator->fSymbolTable = fGpuSymbolTable;
- fIRGenerator->start(&settings, /*inherited=*/nullptr, /*builtin=*/true);
+ fIRGenerator->start(&settings, fGpuSymbolTable, /*inherited=*/nullptr,
+ /*builtin=*/true);
fIRGenerator->fIntrinsics = fGPUIntrinsics.get();
std::ifstream in(SKSL_FP_INCLUDE);
std::string stdText{std::istreambuf_iterator<char>(in),
@@ -1628,7 +1616,7 @@
printf("error reading %s\n", SKSL_FP_INCLUDE);
abort();
}
- const String* source = fGpuSymbolTable->takeOwnershipOfString(
+ const String* source = fRootSymbolTable->takeOwnershipOfString(
std::make_unique<String>(stdText.c_str()));
fIRGenerator->convertProgram(kind, source->c_str(), source->length(), &elements);
fIRGenerator->fIsBuiltinCode = false;
@@ -1638,16 +1626,14 @@
case Program::kPipelineStage_Kind:
this->loadPipelineIntrinsics();
inherited = &fPipelineInclude;
- fIRGenerator->fSymbolTable = fPipelineSymbolTable;
fIRGenerator->fIntrinsics = fGPUIntrinsics.get();
- fIRGenerator->start(&settings, inherited);
+ fIRGenerator->start(&settings, fPipelineSymbolTable, inherited);
break;
case Program::kGeneric_Kind:
this->loadInterpreterIntrinsics();
inherited = nullptr;
- fIRGenerator->fSymbolTable = fInterpreterSymbolTable;
fIRGenerator->fIntrinsics = fInterpreterIntrinsics.get();
- fIRGenerator->start(&settings, /*inherited=*/nullptr);
+ fIRGenerator->start(&settings, fInterpreterSymbolTable, /*inherited=*/nullptr);
break;
}
std::unique_ptr<String> textPtr(new String(std::move(text)));
@@ -1661,6 +1647,7 @@
std::move(elements),
fIRGenerator->fSymbolTable,
fIRGenerator->fInputs);
+ fIRGenerator->finish();
if (fErrorCount) {
return nullptr;
}
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index 18440dc..21b9c12 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -163,9 +163,11 @@
}
void IRGenerator::start(const Program::Settings* settings,
+ std::shared_ptr<SymbolTable> baseSymbolTable,
std::vector<std::unique_ptr<ProgramElement>>* inherited,
bool isBuiltinCode) {
fSettings = settings;
+ fSymbolTable = std::move(baseSymbolTable);
fIsBuiltinCode = isBuiltinCode;
fCapsMap.clear();
if (settings->fCaps) {
@@ -2867,7 +2869,6 @@
if (fErrors.errorCount()) {
return;
}
- this->pushSymbolTable(); // this is popped by Compiler upon completion
SkASSERT(fFile);
for (const auto& decl : fFile->root()) {
switch (decl.fKind) {
diff --git a/src/sksl/SkSLIRGenerator.h b/src/sksl/SkSLIRGenerator.h
index 6db7dfe..1a0a928 100644
--- a/src/sksl/SkSLIRGenerator.h
+++ b/src/sksl/SkSLIRGenerator.h
@@ -114,6 +114,7 @@
* settings.
*/
void start(const Program::Settings* settings,
+ std::shared_ptr<SymbolTable> baseSymbolTable,
std::vector<std::unique_ptr<ProgramElement>>* inherited,
bool isBuiltinCode = false);
diff --git a/src/sksl/SkSLMain.cpp b/src/sksl/SkSLMain.cpp
index 21c57bc..bb6cbd3 100644
--- a/src/sksl/SkSLMain.cpp
+++ b/src/sksl/SkSLMain.cpp
@@ -309,13 +309,6 @@
std::vector<std::unique_ptr<SkSL::ProgramElement>> elements;
compiler.processIncludeFile(kind, argv[1], nullptr, &elements, &symbols);
SkSL::Dehydrator dehydrator;
- for (int i = symbols->fParent->fOwnedSymbols.size() - 1; i >= 0; --i) {
- symbols->fOwnedSymbols.insert(symbols->fOwnedSymbols.begin(),
- std::move(symbols->fParent->fOwnedSymbols[i]));
- }
- for (const auto& p : *symbols->fParent) {
- symbols->addWithoutOwnership(p.first, p.second);
- }
dehydrator.write(*symbols);
dehydrator.write(elements);
SkSL::String baseName = base_name(argv[1], "", ".sksl");
