Reland "Support out parameters that use a swizzle."
This is a reland of 435b4826388cfb05a42efce0c0daf7b10695ad06
inlineStatement now takes a `const Expression* resultExpr` instead of
`const Expression& resultExpr` because resultExpr will be null for a
void function.
Original change's description:
> Support out parameters that use a swizzle.
>
> This CL also removes the `VariableExpression` class that was briefly
> added in a prior CL. This class was intended to support cloning an
> expression while changing the refKind of a VariableReference inside of
> the expression, but it added state and complexity. In this CL, rather
> than track this via extra state, the inliner just recurses into the
> expression as needed to find its VariableReference. Since most relevant
> expressions are just a VariableReference anyway, this is inexpensive.
>
> Change-Id: Id4d926b7d7520b5e6ce455446c05a6d59ef62a84
> Bug: skia:10756
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319917
> Commit-Queue: John Stiles <johnstiles@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
Bug: skia:10756
Change-Id: I35f76c21eccf0ba2ab47e4313e131f7aa26980fa
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/320223
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp
index 00c6d2b..5b5f004 100644
--- a/src/sksl/SkSLInliner.cpp
+++ b/src/sksl/SkSLInliner.cpp
@@ -219,6 +219,30 @@
return nullptr;
}
+std::unique_ptr<Expression> clone_with_ref_kind(const Expression& expr,
+ VariableReference::RefKind refKind) {
+ std::unique_ptr<Expression> clone = expr.clone();
+ class SetRefKindInExpression : public ProgramVisitor {
+ public:
+ SetRefKindInExpression(VariableReference::RefKind refKind) : fRefKind(refKind) {}
+ bool visitExpression(const Expression& expr) override {
+ if (expr.is<VariableReference>()) {
+ // TODO: create a const-savvy ProgramVisitor and remove const_cast
+ const_cast<VariableReference&>(expr.as<VariableReference>()).setRefKind(fRefKind);
+ }
+ return INHERITED::visitExpression(expr);
+ }
+
+ private:
+ VariableReference::RefKind fRefKind;
+
+ using INHERITED = ProgramVisitor;
+ };
+
+ SetRefKindInExpression{refKind}.visitExpression(*clone);
+ return clone;
+}
+
} // namespace
void Inliner::ensureScopedBlocks(Statement* inlinedBody, Statement* parentStmt) {
@@ -375,9 +399,9 @@
return expression.clone();
case Expression::Kind::kVariableReference: {
const VariableReference& v = expression.as<VariableReference>();
- auto found = varMap->find(v.fVariable);
- if (found != varMap->end()) {
- return std::make_unique<VariableReference>(offset, found->second, v.fRefKind);
+ auto varMapIter = varMap->find(v.fVariable);
+ if (varMapIter != varMap->end()) {
+ return clone_with_ref_kind(*varMapIter->second, v.fRefKind);
}
return v.clone();
}
@@ -390,7 +414,7 @@
std::unique_ptr<Statement> Inliner::inlineStatement(int offset,
VariableRewriteMap* varMap,
SymbolTable* symbolTableForStatement,
- const VariableExpression& resultExpr,
+ const Expression* resultExpr,
bool haveEarlyReturns,
const Statement& statement) {
auto stmt = [&](const std::unique_ptr<Statement>& s) -> std::unique_ptr<Statement> {
@@ -458,13 +482,14 @@
case Statement::Kind::kReturn: {
const ReturnStatement& r = statement.as<ReturnStatement>();
if (r.fExpression) {
+ SkASSERT(resultExpr);
auto assignment =
std::make_unique<ExpressionStatement>(std::make_unique<BinaryExpression>(
offset,
- resultExpr.cloneWithRefKind(VariableReference::kWrite_RefKind),
+ clone_with_ref_kind(*resultExpr, VariableReference::kWrite_RefKind),
Token::Kind::TK_EQ,
expr(r.fExpression),
- &resultExpr.type()));
+ &resultExpr->type()));
if (haveEarlyReturns) {
std::vector<std::unique_ptr<Statement>> block;
block.push_back(std::move(assignment));
@@ -514,7 +539,7 @@
typePtr,
old->fStorage,
initialValue.get()));
- (*varMap)[old] = clone;
+ (*varMap)[old] = std::make_unique<VariableReference>(offset, clone);
return std::make_unique<VarDeclaration>(clone, std::move(sizes),
std::move(initialValue));
}
@@ -569,15 +594,14 @@
inlinedBody.children().reserve(1 + // Inline marker
1 + // Result variable
arguments.size() + // Function arguments (passing in)
- arguments.size() + // Function arguments (copy out-parameters
- // back)
- 1); // Inlined code (either as a Block or do-while
- // loop)
+ arguments.size() + // Function arguments (copy out-params back)
+ 1); // Inlined code (Block or do-while loop)
inlinedBody.children().push_back(std::make_unique<InlineMarker>(call->fFunction));
- auto makeInlineVar = [&](const String& baseName, const Type* type, Modifiers modifiers,
- std::unique_ptr<Expression>* initialValue) -> const Variable* {
+ auto makeInlineVar =
+ [&](const String& baseName, const Type* type, Modifiers modifiers,
+ std::unique_ptr<Expression>* initialValue) -> std::unique_ptr<Expression> {
// $floatLiteral or $intLiteral aren't real types that we can use for scratch variables, so
// replace them if they ever appear here. If this happens, we likely forgot to coerce a type
// somewhere during compilation.
@@ -617,35 +641,40 @@
inlinedBody.children().push_back(std::make_unique<VarDeclarationsStatement>(
std::make_unique<VarDeclarations>(offset, type, std::move(variables))));
- return variableSymbol;
+ return std::make_unique<VariableReference>(offset, variableSymbol);
};
// Create a variable to hold the result in the extra statements (excepting void).
- VariableExpression resultExpr;
+ std::unique_ptr<Expression> resultExpr;
if (function.fDeclaration.fReturnType != *fContext->fVoid_Type) {
std::unique_ptr<Expression> noInitialValue;
- const Variable* var = makeInlineVar(String(function.fDeclaration.fName),
- &function.fDeclaration.fReturnType,
- Modifiers{}, &noInitialValue);
- resultExpr.fInnerVariable = std::make_unique<VariableReference>(offset, var);
- }
+ resultExpr = makeInlineVar(String(function.fDeclaration.fName),
+ &function.fDeclaration.fReturnType,
+ Modifiers{}, &noInitialValue);
+ }
// 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.fDeclaration.fParameters[i];
+ bool isOutParam = param->fModifiers.fFlags & Modifiers::kOut_Flag;
+ // If this is a plain VariableReference...
if (arguments[i]->is<VariableReference>()) {
- // The argument is just a variable, so we only need to copy it if it's an out parameter
- // or it's written to within the function.
- if ((param->fModifiers.fFlags & Modifiers::kOut_Flag) ||
- !Analysis::StatementWritesToVariable(*function.fBody, *param)) {
- varMap[param] = arguments[i]->as<VariableReference>().fVariable;
+ // ... and it's an `out` param, or it isn't written to within the inline function...
+ if (isOutParam || !Analysis::StatementWritesToVariable(*function.fBody, *param)) {
+ // ... we don't need to copy it at all! We can just use the existing variable.
+ varMap[param] = arguments[i]->as<VariableReference>().clone();
continue;
}
}
+ if (isOutParam) {
+ argsToCopyBack.push_back(i);
+ }
+
varMap[param] = makeInlineVar(String(param->fName), &arguments[i]->type(),
param->fModifiers, &arguments[i]);
}
@@ -655,7 +684,7 @@
inlineBlock->children().reserve(body.children().size());
for (const std::unique_ptr<Statement>& stmt : body.children()) {
inlineBlock->children().push_back(this->inlineStatement(
- offset, &varMap, symbolTableForCall, resultExpr, hasEarlyReturn, *stmt));
+ offset, &varMap, symbolTableForCall, resultExpr.get(), hasEarlyReturn, *stmt));
}
if (hasEarlyReturn) {
// Since we output to backends that don't have a goto statement (which would normally be
@@ -672,33 +701,23 @@
inlinedBody.children().push_back(std::move(inlineBlock));
}
- // Copy the values of `out` parameters into their destinations.
- for (size_t i = 0; i < arguments.size(); ++i) {
+ // Copy back the values of `out` parameters into their real destinations.
+ for (int i : argsToCopyBack) {
const Variable* p = function.fDeclaration.fParameters[i];
- if (p->fModifiers.fFlags & Modifiers::kOut_Flag) {
- SkASSERT(varMap.find(p) != varMap.end());
- if (arguments[i]->is<VariableReference>() &&
- arguments[i]->as<VariableReference>().fVariable == varMap[p]) {
- // We didn't create a temporary for this parameter, so there's nothing to copy back
- // out.
- continue;
- }
- auto varRef = std::make_unique<VariableReference>(offset, varMap[p]);
- inlinedBody.children().push_back(std::make_unique<ExpressionStatement>(
- std::make_unique<BinaryExpression>(offset,
- arguments[i]->clone(),
- Token::Kind::TK_EQ,
- std::move(varRef),
- &arguments[i]->type())));
- }
+ SkASSERT(varMap.find(p) != varMap.end());
+ inlinedBody.children().push_back(
+ std::make_unique<ExpressionStatement>(std::make_unique<BinaryExpression>(
+ offset,
+ clone_with_ref_kind(*arguments[i], VariableReference::kWrite_RefKind),
+ Token::Kind::TK_EQ,
+ std::move(varMap[p]),
+ &arguments[i]->type())));
}
- if (function.fDeclaration.fReturnType != *fContext->fVoid_Type) {
- // Return a reference to the result variable as our replacement expression.
- resultExpr.fInnerVariable->setRefKind(VariableReference::kRead_RefKind);
- inlinedCall.fReplacementExpr = resultExpr.fOuterExpression
- ? std::move(resultExpr.fOuterExpression)
- : std::move(resultExpr.fInnerVariable);
+ if (resultExpr != nullptr) {
+ // Return our result variable as our replacement expression.
+ SkASSERT(resultExpr->as<VariableReference>().fRefKind == VariableReference::kRead_RefKind);
+ inlinedCall.fReplacementExpr = std::move(resultExpr);
} else {
// It's a void function, so it doesn't actually result in anything, but we have to return
// something non-null as a standin.