Disallow inlining a function with out-parameters.
It is difficult to do this both efficiently and correctly while honoring
GLSL semantics (which require the lvalues to be kept distinct, even when
they point to the same variable). We could make it work by making copies
of every out parameter in each direction (going in for inouts, and
coming out for outs and inouts).
However, this could be self-defeating if it makes it harder for the
driver to track variable lifetimes. Simply opting out of inlining these
functions entirely seems like the best tradeoff; let the driver optimize
them if it can, and we can enjoy reduced complexity in the SkSL inliner.
Change-Id: I62f7b4550cc181cfe789e4f2ff4e408ba1baf9cb
Bug: skia:11326
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/370257
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp
index 7fd4a8f..b5c0a94 100644
--- a/src/sksl/SkSLInliner.cpp
+++ b/src/sksl/SkSLInliner.cpp
@@ -542,6 +542,9 @@
type = &type->scalarTypeForLiteral();
}
+ // Out parameters aren't supported.
+ SkASSERT(!(modifiers.fFlags & Modifiers::kOut_Flag));
+
// Provide our new variable with a unique name, and add it to our symbol table.
const String* namePtr = symbolTable->takeOwnershipOfString(
std::make_unique<String>(fMangler.uniqueName(baseName, symbolTable)));
@@ -556,15 +559,9 @@
isBuiltinCode,
Variable::Storage::kLocal);
- // Prepare the variable declaration (taking extra care with `out` params to not clobber any
- // initial value).
- if (*initialValue && (modifiers.fFlags & Modifiers::kOut_Flag)) {
- result.fVarDecl = std::make_unique<VarDeclaration>(var.get(), type, /*arraySize=*/0,
- (*initialValue)->clone());
- } else {
- result.fVarDecl = std::make_unique<VarDeclaration>(var.get(), type, /*arraySize=*/0,
- std::move(*initialValue));
- }
+ // Create our variable declaration.
+ result.fVarDecl = std::make_unique<VarDeclaration>(var.get(), type, /*arraySize=*/0,
+ std::move(*initialValue));
var->setDeclaration(&result.fVarDecl->as<VarDeclaration>());
result.fVarSymbol = symbolTable->add(std::move(var));
return result;
@@ -619,23 +616,17 @@
// Create variables in the extra statements to hold the arguments, and assign the arguments to
// them.
VariableRewriteMap varMap;
- std::vector<int> argsToCopyBack;
- for (int i = 0; i < (int) arguments.size(); ++i) {
- const Variable* param = function.declaration().parameters()[i];
- bool isOutParam = param->modifiers().fFlags & Modifiers::kOut_Flag;
-
+ for (int i = 0; i < arguments.count(); ++i) {
// If this argument can be inlined trivially (e.g. a swizzle, or a constant array index)...
+ const Variable* param = function.declaration().parameters()[i];
if (Analysis::IsTrivialExpression(*arguments[i])) {
- // ... and it's an `out` param, or it isn't written to within the inline function...
- if (isOutParam || !Analysis::StatementWritesToVariable(*function.body(), *param)) {
+ // ... and isn't written to within the inline function...
+ if (!Analysis::StatementWritesToVariable(*function.body(), *param)) {
// ... we don't need to copy it at all! We can just use the existing expression.
varMap[param] = arguments[i]->clone();
continue;
}
}
- if (isOutParam) {
- argsToCopyBack.push_back(i);
- }
InlineVariable var = this->makeInlineVariable(param->name(), &arguments[i]->type(),
symbolTable.get(), param->modifiers(),
caller->isBuiltin(), &arguments[i]);
@@ -646,25 +637,13 @@
const Block& body = function.body()->as<Block>();
StatementArray* inlineStatements = &inlinedBlockStmts;
- inlineStatements->reserve_back(body.children().size() + argsToCopyBack.size());
+ inlineStatements->reserve_back(body.children().size());
for (const std::unique_ptr<Statement>& stmt : body.children()) {
inlineStatements->push_back(this->inlineStatement(offset, &varMap, symbolTable.get(),
&resultExpr, returnComplexity, *stmt,
caller->isBuiltin()));
}
- // Copy back the values of `out` parameters into their real destinations.
- for (int i : argsToCopyBack) {
- const Variable* p = function.declaration().parameters()[i];
- SkASSERT(varMap.find(p) != varMap.end());
- inlineStatements->push_back(ExpressionStatement::Make(
- *fContext,
- BinaryExpression::Make(*fContext,
- clone_with_ref_kind(*arguments[i], VariableRefKind::kWrite),
- Token::Kind::TK_EQ,
- std::move(varMap[p]))));
- }
-
// Wrap all of the generated statements in a block. We need a real Block here, so we can't use
// MakeUnscoped. This is because we need to add another child statement to the Block later.
inlinedCall.fInlinedBody = Block::Make(offset, std::move(inlinedBlockStmts),
@@ -712,6 +691,13 @@
return false;
}
+ // We don't allow inlining a function with out parameters. (See skia:11326 for rationale.)
+ for (const Variable* param : functionDef->declaration().parameters()) {
+ if (param->modifiers().fFlags & Modifiers::Flag::kOut_Flag) {
+ return false;
+ }
+ }
+
// We don't have a mechanism to simulate early returns, so we can't inline if there is one.
return GetReturnComplexity(*functionDef) < ReturnComplexity::kEarlyReturns;
}