Replace Setting IRNodes with actual caps-values during IR generation.
Previously, this happened during optimization, so we couldn't disable
control-flow analysis on any code which used sk_Caps. Now this happens
as soon as the IRNode is rehydrated or instantiated. The logic has
migrated to a static helper function, Setting::Make.
Change-Id: I102557845ccd1b68cc2b12381563f06bc64c621f
Bug: skia:11365, skia:11343, skia:11319
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375298
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
diff --git a/src/sksl/SkSLCompiler.cpp b/src/sksl/SkSLCompiler.cpp
index 68f8986..d10eb18 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -87,6 +87,21 @@
const String* fOldSource;
};
+class AutoProgramConfig {
+public:
+ AutoProgramConfig(std::shared_ptr<Context>& context, ProgramConfig* config)
+ : fContext(context.get()) {
+ SkASSERT(!fContext->fConfig);
+ fContext->fConfig = config;
+ }
+
+ ~AutoProgramConfig() {
+ fContext->fConfig = nullptr;
+ }
+
+ Context* fContext;
+};
+
Compiler::Compiler(const ShaderCapsClass* caps)
: fContext(std::make_shared<Context>(/*errors=*/*this, *caps))
, fInliner(fContext.get())
@@ -256,16 +271,24 @@
LoadedModule Compiler::loadModule(ProgramKind kind,
ModuleData data,
- std::shared_ptr<SymbolTable> base) {
- if (!base) {
- // NOTE: This is a workaround. The only time 'base' is null is when dehydrating includes.
- // In that case, skslc doesn't know which module it's preparing, nor what the correct base
- // module is. We can't use 'Root', because many GPU intrinsics reference private types,
- // like samplers or textures. Today, 'Private' does contain the union of all known types,
- // so this is safe. If we ever have types that only exist in 'Public' (for example), this
- // logic needs to be smarter (by choosing the correct base for the module we're compiling).
+ std::shared_ptr<SymbolTable> base,
+ bool dehydrate) {
+ if (dehydrate) {
+ // NOTE: This is a workaround. When dehydrating includes, skslc doesn't know which module
+ // it's preparing, nor what the correct base module is. We can't use 'Root', because many
+ // GPU intrinsics reference private types, like samplers or textures. Today, 'Private' does
+ // contain the union of all known types, so this is safe. If we ever have types that only
+ // exist in 'Public' (for example), this logic needs to be smarter (by choosing the correct
+ // base for the module we're compiling).
base = fPrivateSymbolTable;
}
+ SkASSERT(base);
+
+ // Built-in modules always use default program settings.
+ ProgramConfig config;
+ config.fKind = kind;
+ config.fSettings.fReplaceSettings = !dehydrate;
+ AutoProgramConfig autoConfig(fContext, &config);
#if defined(SKSL_STANDALONE)
SkASSERT(data.fPath);
@@ -282,13 +305,6 @@
SkASSERT(fIRGenerator->fCanInline);
fIRGenerator->fCanInline = false;
- ProgramConfig config;
- config.fKind = kind;
- config.fSettings.fReplaceSettings = false;
-
- fContext->fConfig = &config;
- SK_AT_SCOPE_EXIT(fContext->fConfig = nullptr);
-
ParsedModule baseModule = {base, /*fIntrinsics=*/nullptr};
IRGenerator::IRBundle ir = fIRGenerator->convertProgram(baseModule, /*isBuiltinCode=*/true,
source->c_str(), source->length(),
@@ -313,7 +329,7 @@
}
ParsedModule Compiler::parseModule(ProgramKind kind, ModuleData data, const ParsedModule& base) {
- LoadedModule module = this->loadModule(kind, data, base.fSymbols);
+ LoadedModule module = this->loadModule(kind, data, base.fSymbols, /*dehydrate=*/false);
this->optimize(module);
// For modules that just declare (but don't define) intrinsic functions, there will be no new
@@ -1436,10 +1452,7 @@
// Update our context to point to the program configuration for the duration of compilation.
auto config = std::make_unique<ProgramConfig>(ProgramConfig{kind, settings});
-
- SkASSERT(!fContext->fConfig);
- fContext->fConfig = config.get();
- SK_AT_SCOPE_EXIT(fContext->fConfig = nullptr);
+ AutoProgramConfig autoConfig(fContext, config.get());
fErrorText = "";
fErrorCount = 0;
@@ -1541,11 +1554,7 @@
// Create a temporary program configuration with default settings.
ProgramConfig config;
config.fKind = module.fKind;
-
- // Update our context to point to this configuration for the duration of compilation.
- SkASSERT(!fContext->fConfig);
- fContext->fConfig = &config;
- SK_AT_SCOPE_EXIT(fContext->fConfig = nullptr);
+ AutoProgramConfig autoConfig(fContext, &config);
// Reset the Inliner.
fInliner.reset(fModifiers.back().get());
@@ -1556,11 +1565,11 @@
bool madeChanges = false;
// Scan and optimize based on the control-flow graph for each function.
- // TODO(skia:11365): we always perform CFG-based optimization here to reduce Settings into
- // their final form. We should do this optimization in our Make functions instead.
- for (const auto& element : module.fElements) {
- if (element->is<FunctionDefinition>()) {
- madeChanges |= this->scanCFG(element->as<FunctionDefinition>(), usage.get());
+ if (config.fSettings.fControlFlowAnalysis) {
+ for (const auto& element : module.fElements) {
+ if (element->is<FunctionDefinition>()) {
+ madeChanges |= this->scanCFG(element->as<FunctionDefinition>(), usage.get());
+ }
}
}
diff --git a/src/sksl/SkSLCompiler.h b/src/sksl/SkSLCompiler.h
index e88ce58..819ac32 100644
--- a/src/sksl/SkSLCompiler.h
+++ b/src/sksl/SkSLCompiler.h
@@ -160,7 +160,8 @@
return ModuleData{/*fPath=*/nullptr, data, size};
}
- LoadedModule loadModule(ProgramKind kind, ModuleData data, std::shared_ptr<SymbolTable> base);
+ LoadedModule loadModule(ProgramKind kind, ModuleData data, std::shared_ptr<SymbolTable> base,
+ bool dehydrate);
ParsedModule parseModule(ProgramKind kind, ModuleData data, const ParsedModule& base);
IRGenerator& irGenerator() {
diff --git a/src/sksl/SkSLDehydrator.cpp b/src/sksl/SkSLDehydrator.cpp
index d348d53..d082814 100644
--- a/src/sksl/SkSLDehydrator.cpp
+++ b/src/sksl/SkSLDehydrator.cpp
@@ -348,7 +348,6 @@
const Setting& s = e->as<Setting>();
this->writeCommand(Rehydrator::kSetting_Command);
this->write(s.name());
- this->write(s.type());
break;
}
case Expression::Kind::kSwizzle: {
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index 985c0ba..4c8102e 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -2492,14 +2492,7 @@
StringFragment field = fieldNode.getString();
const Type& baseType = base->type();
if (baseType == *fContext.fTypes.fSkCaps) {
- if (this->settings().fReplaceSettings && !fIsBuiltinCode) {
- return Setting::GetValue(fContext, fieldNode.fOffset, field);
- }
- const Type* type = Setting::GetType(fContext, fieldNode.fOffset, field);
- if (!type) {
- return nullptr;
- }
- return std::make_unique<Setting>(fieldNode.fOffset, field, type);
+ return Setting::Make(fContext, fieldNode.fOffset, field);
}
switch (baseType.typeKind()) {
case Type::TypeKind::kStruct:
diff --git a/src/sksl/SkSLMain.cpp b/src/sksl/SkSLMain.cpp
index b178145..9e10d44 100644
--- a/src/sksl/SkSLMain.cpp
+++ b/src/sksl/SkSLMain.cpp
@@ -454,8 +454,9 @@
printf("error writing '%s'\n", outputPath.c_str());
return ResultCode::kOutputError;
}
- SkSL::LoadedModule module = compiler.loadModule(
- kind, SkSL::Compiler::MakeModulePath(inputPath.c_str()), nullptr);
+ SkSL::LoadedModule module =
+ compiler.loadModule(kind, SkSL::Compiler::MakeModulePath(inputPath.c_str()),
+ /*base=*/nullptr, /*dehydrate=*/true);
SkSL::Dehydrator dehydrator;
dehydrator.write(*module.fSymbols);
dehydrator.write(module.fElements);
@@ -475,8 +476,8 @@
return ResultCode::kOutputError;
}
} else {
- printf("expected output path to end with one of: .glsl, .metal, .spirv, .asm.frag, "
- ".asm.vert, .asm.geom, .cpp, .h (got '%s')\n", outputPath.c_str());
+ printf("expected output path to end with one of: .glsl, .metal, .spirv, .asm.frag, .skvm, "
+ ".stage, .asm.vert, .asm.geom, .cpp, .h (got '%s')\n", outputPath.c_str());
return ResultCode::kConfigurationError;
}
return ResultCode::kSuccess;
diff --git a/src/sksl/SkSLRehydrator.cpp b/src/sksl/SkSLRehydrator.cpp
index 55949d4..e935d48 100644
--- a/src/sksl/SkSLRehydrator.cpp
+++ b/src/sksl/SkSLRehydrator.cpp
@@ -514,8 +514,7 @@
}
case Rehydrator::kSetting_Command: {
StringFragment name = this->readString();
- const Type* type = this->type();
- return std::make_unique<Setting>(-1, name, type);
+ return Setting::Make(fContext, /*offset=*/-1, name);
}
case Rehydrator::kSwizzle_Command: {
std::unique_ptr<Expression> base = this->expression();
diff --git a/src/sksl/generated/sksl_gpu.dehydrated.sksl b/src/sksl/generated/sksl_gpu.dehydrated.sksl
index 1b3c896..899dcbc 100644
--- a/src/sksl/generated/sksl_gpu.dehydrated.sksl
+++ b/src/sksl/generated/sksl_gpu.dehydrated.sksl
@@ -3950,7 +3950,6 @@
34,
44,
35,114,8,
-40,14,3,
44,
1,
49,167,3,0,65,
@@ -4311,7 +4310,6 @@
34,
44,
35,159,8,
-40,14,3,
1,
49,209,3,0,49,
1,
@@ -4330,7 +4328,6 @@
34,
44,
35,159,8,
-40,14,3,
1,
49,212,3,0,49,
1,
@@ -6138,7 +6135,6 @@
34,
44,
35,255,9,
-40,14,3,
21,
40,168,0,105,2,1,
6,
@@ -6165,7 +6161,6 @@
34,
44,
35,255,9,
-40,14,3,
21,
40,176,0,109,2,1,
6,
diff --git a/src/sksl/ir/SkSLSetting.cpp b/src/sksl/ir/SkSLSetting.cpp
index 65841b4..9f7a85c 100644
--- a/src/sksl/ir/SkSLSetting.cpp
+++ b/src/sksl/ir/SkSLSetting.cpp
@@ -101,7 +101,7 @@
} // namespace
-const Type* Setting::GetType(const Context& context, int offset, const String& name) {
+static const Type* get_type(const Context& context, int offset, const String& name) {
if (const CapsLookupMethod* caps = caps_lookup_table().lookup(name)) {
return caps->type(context);
}
@@ -110,8 +110,8 @@
return nullptr;
}
-std::unique_ptr<Expression> Setting::GetValue(const Context& context, int offset,
- const String& name) {
+static std::unique_ptr<Expression> get_value(const Context& context, int offset,
+ const String& name) {
if (const CapsLookupMethod* caps = caps_lookup_table().lookup(name)) {
return caps->value(context);
}
@@ -120,12 +120,17 @@
return nullptr;
}
-std::unique_ptr<Expression> Setting::constantPropagate(const IRGenerator& irGenerator,
- const DefinitionMap& definitions) {
- if (irGenerator.fContext.fConfig->fSettings.fReplaceSettings) {
- return GetValue(irGenerator.fContext, fOffset, this->name());
+std::unique_ptr<Expression> Setting::Make(const Context& context, int offset, const String& name) {
+ SkASSERT(context.fConfig);
+
+ if (context.fConfig->fSettings.fReplaceSettings) {
+ // Insert the settings value directly into the IR.
+ return get_value(context, offset, name);
}
- return nullptr;
+
+ // Generate a Setting IRNode.
+ const Type* type = get_type(context, offset, name);
+ return type ? std::make_unique<Setting>(offset, name, type) : nullptr;
}
} // namespace SkSL
diff --git a/src/sksl/ir/SkSLSetting.h b/src/sksl/ir/SkSLSetting.h
index 8ffe827..6a0cb50 100644
--- a/src/sksl/ir/SkSLSetting.h
+++ b/src/sksl/ir/SkSLSetting.h
@@ -14,8 +14,9 @@
namespace SkSL {
/**
- * Represents a compile-time constant setting, such as sk_Caps.fbFetchSupport. These are generally
- * collapsed down to their constant representations during the compilation process.
+ * Represents a compile-time constant setting, such as sk_Caps.fbFetchSupport. These IRNodes should
+ * only exist in a dehydrated module. These nodes are replaced with the value of the setting during
+ * rehydration or compilation (i.e., whenever fReplaceSettings is true).
*/
class Setting final : public Expression {
public:
@@ -25,16 +26,10 @@
: INHERITED(offset, kExpressionKind, type)
, fName(std::move(name)) {}
- static const Type* GetType(const Context& context, int offset, const String& name);
-
- static std::unique_ptr<Expression> GetValue(const Context& context, int offset,
- const String& name);
-
- std::unique_ptr<Expression> constantPropagate(const IRGenerator& irGenerator,
- const DefinitionMap& definitions) override;
+ static std::unique_ptr<Expression> Make(const Context& context, int offset, const String& name);
std::unique_ptr<Expression> clone() const override {
- return std::unique_ptr<Expression>(new Setting(fOffset, this->name(), &this->type()));
+ return std::make_unique<Setting>(fOffset, this->name(), &this->type());
}
const String& name() const {
@@ -49,10 +44,6 @@
return false;
}
- bool isCompileTimeConstant() const override {
- return true;
- }
-
private:
String fName;
