Eliminate dead local variables from dehydrated code.
This should address the root cause of http://review.skia.org/538045.
We currently don't eliminate unreferenced names from the symbol table,
although that'd be free money if we wanted to do it.
Change-Id: I712e23e6a4f3cb48c2b5ae68f042e13031d5f632
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/540038
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: 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 0a96652..29cce65 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -383,7 +383,7 @@
AutoProgramConfig autoConfig(fContext, &config);
SkASSERT(data.fData && (data.fSize != 0));
Rehydrator rehydrator(*this, data.fData, data.fSize, std::move(base));
- LoadedModule result = { kind, rehydrator.symbolTable(), rehydrator.elements() };
+ LoadedModule module = {kind, rehydrator.symbolTable(), rehydrator.elements()};
#else
SkASSERT(this->errorCount() == 0);
SkASSERT(data.fPath);
@@ -394,20 +394,21 @@
abort();
}
ParsedModule baseModule = {std::move(base), /*fElements=*/nullptr};
- LoadedModule result = DSLParser(this, settings, kind,
- std::move(text)).moduleInheritingFrom(std::move(baseModule));
+ LoadedModule module = DSLParser(this, settings, kind, std::move(text))
+ .moduleInheritingFrom(std::move(baseModule));
if (this->errorCount()) {
printf("Unexpected errors: %s\n", this->fErrorText.c_str());
SkDEBUGFAILF("%s %s\n", data.fPath, this->fErrorText.c_str());
}
+ this->optimizeModuleForDehydration(module, baseModule);
#endif
- return result;
+ return module;
}
ParsedModule Compiler::parseModule(ProgramKind kind, ModuleData data, const ParsedModule& base) {
LoadedModule module = this->loadModule(kind, data, base.fSymbols, /*dehydrate=*/false);
- this->optimize(module, base);
+ this->optimizeRehydratedModule(module, base);
// For modules that just declare (but don't define) intrinsic functions, there will be no new
// program elements. In that case, we can share our parent's element map:
@@ -568,7 +569,7 @@
}
}
-bool Compiler::optimize(LoadedModule& module, const ParsedModule& base) {
+bool Compiler::optimizeRehydratedModule(LoadedModule& module, const ParsedModule& base) {
SkASSERT(!this->errorCount());
// Create a temporary program configuration with default settings.
@@ -590,6 +591,31 @@
return this->errorCount() == 0;
}
+bool Compiler::optimizeModuleForDehydration(LoadedModule& module, const ParsedModule& base) {
+ SkASSERT(!this->errorCount());
+
+ // Create a temporary program configuration with default settings.
+ ProgramConfig config;
+ config.fIsBuiltinCode = true;
+ config.fKind = module.fKind;
+ AutoProgramConfig autoConfig(fContext, &config);
+
+ std::unique_ptr<ProgramUsage> usage = Analysis::GetUsage(module, base);
+
+ while (Transform::EliminateDeadLocalVariables(*fContext, module, usage.get())) {
+ // Removing dead variables may cause more variables to become unreferenced. Try again.
+ }
+
+ // TODO: run EliminateUnreachableCode pass
+
+ // Note that we intentionally don't attempt to eliminate unreferenced global variables or
+ // functions here, since those can be referenced by the finished program even if they're
+ // unreferenced now. We also don't run the inliner to avoid growing the program; that is done in
+ // `optimizeRehydratedModule` above.
+
+ return this->errorCount() == 0;
+}
+
bool Compiler::optimize(Program& program) {
// The optimizer only needs to run when it is enabled.
if (!program.fConfig->fSettings.fOptimize) {
diff --git a/src/sksl/SkSLCompiler.h b/src/sksl/SkSLCompiler.h
index ed33fb1..eb20bdf 100644
--- a/src/sksl/SkSLCompiler.h
+++ b/src/sksl/SkSLCompiler.h
@@ -250,8 +250,11 @@
/** Performs final checks to confirm that a fully-assembled/optimized is valid. */
bool finalize(Program& program);
- /** Optimize the module. */
- bool optimize(LoadedModule& module, const ParsedModule& base);
+ /** Optimize a module in preparation for dehydration. */
+ bool optimizeModuleForDehydration(LoadedModule& module, const ParsedModule& base);
+
+ /** Optimize a module after rehydrating it. */
+ bool optimizeRehydratedModule(LoadedModule& module, const ParsedModule& base);
/** Flattens out function calls when it is safe to do so. */
bool runInliner(const std::vector<std::unique_ptr<ProgramElement>>& elements,
diff --git a/src/sksl/generated/sksl_graphite_frag.dehydrated.sksl b/src/sksl/generated/sksl_graphite_frag.dehydrated.sksl
index 88451b6..a39df99 100644
--- a/src/sksl/generated/sksl_graphite_frag.dehydrated.sksl
+++ b/src/sksl/generated/sksl_graphite_frag.dehydrated.sksl
@@ -447,10 +447,7 @@
51,255,255,110,0,0,
36,
51,255,255,110,0,2,0,0,0,
-56,80,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,3,0,0,0,
+40,
32,0,
1,
57,7,0,0,16,
@@ -1654,122 +1651,35 @@
7,0,
3,0,
11,0,30,
-56,129,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,0,0,0,0,
-56,130,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,1,0,0,0,
-56,131,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,2,0,0,0,
-56,132,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,3,0,0,0,
-56,133,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,4,0,0,0,
-56,134,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,5,0,0,0,
-56,135,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,6,0,0,0,
-56,136,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,7,0,0,0,
-56,137,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,8,0,0,0,
-56,138,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,9,0,0,0,
-56,139,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,10,0,0,0,
-56,140,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,11,0,0,0,
-56,141,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,12,0,0,0,
-56,142,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,13,0,0,0,
-56,143,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,14,0,0,0,
-56,144,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,15,0,0,0,
-56,145,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,16,0,0,0,
-56,146,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,17,0,0,0,
-56,147,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,18,0,0,0,
-56,148,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,19,0,0,0,
-56,149,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,20,0,0,0,
-56,150,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,21,0,0,0,
-56,151,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,22,0,0,0,
-56,152,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,23,0,0,0,
-56,153,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,24,0,0,0,
-56,154,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,25,0,0,0,
-56,155,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,26,0,0,0,
-56,156,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,27,0,0,0,
-56,157,0,
-51,255,255,110,0,0,
-36,
-51,255,255,110,0,28,0,0,0,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
+40,
49,0,
52,1,29,0,
51,129,0,
diff --git a/src/sksl/transform/BUILD.bazel b/src/sksl/transform/BUILD.bazel
index 26f61a7..ef08690 100644
--- a/src/sksl/transform/BUILD.bazel
+++ b/src/sksl/transform/BUILD.bazel
@@ -68,10 +68,12 @@
deps = [
":SkSLProgramWriter_hdr",
":SkSLTransform_hdr",
+ "//include/core:SkSpan_hdr",
"//include/core:SkTypes_hdr",
"//include/private:SkSLProgramElement_hdr",
"//include/private:SkSLStatement_hdr",
"//include/private:SkTHash_hdr",
+ "//src/sksl:SkSLCompiler_hdr",
"//src/sksl:SkSLProgramSettings_hdr",
"//src/sksl/ir:SkSLExpressionStatement_hdr",
"//src/sksl/ir:SkSLExpression_hdr",
@@ -111,4 +113,5 @@
name = "SkSLTransform_hdr",
hdrs = ["SkSLTransform.h"],
visibility = ["//:__subpackages__"],
+ deps = ["//include/core:SkSpan_hdr"],
)
diff --git a/src/sksl/transform/SkSLEliminateDeadLocalVariables.cpp b/src/sksl/transform/SkSLEliminateDeadLocalVariables.cpp
index 73110b8..3394340 100644
--- a/src/sksl/transform/SkSLEliminateDeadLocalVariables.cpp
+++ b/src/sksl/transform/SkSLEliminateDeadLocalVariables.cpp
@@ -5,10 +5,12 @@
* found in the LICENSE file.
*/
+#include "include/core/SkSpan.h"
#include "include/core/SkTypes.h"
#include "include/private/SkSLProgramElement.h"
#include "include/private/SkSLStatement.h"
#include "include/private/SkTHash.h"
+#include "src/sksl/SkSLCompiler.h"
#include "src/sksl/SkSLProgramSettings.h"
#include "src/sksl/ir/SkSLExpression.h"
#include "src/sksl/ir/SkSLExpressionStatement.h"
@@ -22,12 +24,14 @@
#include <memory>
#include <utility>
-#include <vector>
namespace SkSL {
+
class Context;
-bool Transform::EliminateDeadLocalVariables(Program& program, ProgramUsage* usage) {
+static bool eliminate_dead_local_variables(const Context& context,
+ SkSpan<std::unique_ptr<ProgramElement>> elements,
+ ProgramUsage* usage) {
class DeadLocalVariableEliminator : public ProgramWriter {
public:
DeadLocalVariableEliminator(const Context& context, ProgramUsage* usage)
@@ -87,24 +91,36 @@
using INHERITED = ProgramWriter;
};
- DeadLocalVariableEliminator visitor{*program.fContext, usage};
+ DeadLocalVariableEliminator visitor{context, usage};
- if (program.fConfig->fSettings.fRemoveDeadVariables) {
- for (auto& [var, counts] : usage->fVariableCounts) {
- if (DeadLocalVariableEliminator::CanEliminate(var, counts)) {
- // This program contains at least one dead local variable.
- // Scan the program for any dead local variables and eliminate them all.
- for (std::unique_ptr<ProgramElement>& pe : program.fOwnedElements) {
- if (pe->is<FunctionDefinition>()) {
- visitor.visitProgramElement(*pe);
- }
+ for (auto& [var, counts] : usage->fVariableCounts) {
+ if (DeadLocalVariableEliminator::CanEliminate(var, counts)) {
+ // This program contains at least one dead local variable.
+ // Scan the program for any dead local variables and eliminate them all.
+ for (std::unique_ptr<ProgramElement>& pe : elements) {
+ if (pe->is<FunctionDefinition>()) {
+ visitor.visitProgramElement(*pe);
}
- break;
}
+ break;
}
}
return visitor.fMadeChanges;
}
+bool Transform::EliminateDeadLocalVariables(const Context& context,
+ LoadedModule& module,
+ ProgramUsage* usage) {
+ return eliminate_dead_local_variables(context, SkMakeSpan(module.fElements), usage);
+}
+
+bool Transform::EliminateDeadLocalVariables(Program& program, ProgramUsage* usage) {
+ return program.fConfig->fSettings.fRemoveDeadVariables
+ ? eliminate_dead_local_variables(*program.fContext,
+ SkMakeSpan(program.fOwnedElements),
+ usage)
+ : false;
+}
+
} // namespace SkSL
diff --git a/src/sksl/transform/SkSLTransform.h b/src/sksl/transform/SkSLTransform.h
index b2d1f90..99fad7c 100644
--- a/src/sksl/transform/SkSLTransform.h
+++ b/src/sksl/transform/SkSLTransform.h
@@ -8,12 +8,14 @@
#ifndef SKSL_TRANSFORM
#define SKSL_TRANSFORM
+#include "include/core/SkSpan.h"
#include <memory>
#include <vector>
namespace SkSL {
class Context;
+struct LoadedModule;
struct Program;
class ProgramElement;
class ProgramUsage;
@@ -44,6 +46,7 @@
* Eliminates variables in a program which are never read or written (past their initializer).
* Preserves side effects from initializers, if any. Returns true if any changes were made.
*/
+bool EliminateDeadLocalVariables(const Context& context, LoadedModule& module, ProgramUsage* usage);
bool EliminateDeadLocalVariables(Program& program, ProgramUsage* usage);
bool EliminateDeadGlobalVariables(Program& program, ProgramUsage* usage);
