Revert "Support out parameters that use a swizzle."
This reverts commit 435b4826388cfb05a42efce0c0daf7b10695ad06.
Reason for revert: ASAN/UBSAN unhappy.
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>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Ibdda47607f9e6e7f3a7459915067cf5e20919993
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:10756
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/320220
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp
index 3728525..00c6d2b 100644
--- a/src/sksl/SkSLInliner.cpp
+++ b/src/sksl/SkSLInliner.cpp
@@ -219,30 +219,6 @@
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) {
@@ -399,9 +375,9 @@
return expression.clone();
case Expression::Kind::kVariableReference: {
const VariableReference& v = expression.as<VariableReference>();
- auto varMapIter = varMap->find(v.fVariable);
- if (varMapIter != varMap->end()) {
- return clone_with_ref_kind(*varMapIter->second, v.fRefKind);
+ auto found = varMap->find(v.fVariable);
+ if (found != varMap->end()) {
+ return std::make_unique<VariableReference>(offset, found->second, v.fRefKind);
}
return v.clone();
}
@@ -414,7 +390,7 @@
std::unique_ptr<Statement> Inliner::inlineStatement(int offset,
VariableRewriteMap* varMap,
SymbolTable* symbolTableForStatement,
- const Expression& resultExpr,
+ const VariableExpression& resultExpr,
bool haveEarlyReturns,
const Statement& statement) {
auto stmt = [&](const std::unique_ptr<Statement>& s) -> std::unique_ptr<Statement> {
@@ -485,7 +461,7 @@
auto assignment =
std::make_unique<ExpressionStatement>(std::make_unique<BinaryExpression>(
offset,
- clone_with_ref_kind(resultExpr, VariableReference::kWrite_RefKind),
+ resultExpr.cloneWithRefKind(VariableReference::kWrite_RefKind),
Token::Kind::TK_EQ,
expr(r.fExpression),
&resultExpr.type()));
@@ -538,7 +514,7 @@
typePtr,
old->fStorage,
initialValue.get()));
- (*varMap)[old] = std::make_unique<VariableReference>(offset, clone);
+ (*varMap)[old] = clone;
return std::make_unique<VarDeclaration>(clone, std::move(sizes),
std::move(initialValue));
}
@@ -593,14 +569,15 @@
inlinedBody.children().reserve(1 + // Inline marker
1 + // Result variable
arguments.size() + // Function arguments (passing in)
- arguments.size() + // Function arguments (copy out-params back)
- 1); // Inlined code (Block or do-while loop)
+ arguments.size() + // Function arguments (copy out-parameters
+ // back)
+ 1); // Inlined code (either as a 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) -> std::unique_ptr<Expression> {
+ auto makeInlineVar = [&](const String& baseName, const Type* type, Modifiers modifiers,
+ std::unique_ptr<Expression>* initialValue) -> const Variable* {
// $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.
@@ -640,40 +617,35 @@
inlinedBody.children().push_back(std::make_unique<VarDeclarationsStatement>(
std::make_unique<VarDeclarations>(offset, type, std::move(variables))));
- return std::make_unique<VariableReference>(offset, variableSymbol);
+ return variableSymbol;
};
// Create a variable to hold the result in the extra statements (excepting void).
- std::unique_ptr<Expression> resultExpr;
+ VariableExpression resultExpr;
if (function.fDeclaration.fReturnType != *fContext->fVoid_Type) {
std::unique_ptr<Expression> noInitialValue;
- resultExpr = makeInlineVar(String(function.fDeclaration.fName),
- &function.fDeclaration.fReturnType,
- Modifiers{}, &noInitialValue);
- }
+ const Variable* var = makeInlineVar(String(function.fDeclaration.fName),
+ &function.fDeclaration.fReturnType,
+ Modifiers{}, &noInitialValue);
+ resultExpr.fInnerVariable = std::make_unique<VariableReference>(offset, var);
+ }
// 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>()) {
- // ... 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();
+ // 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;
continue;
}
}
- if (isOutParam) {
- argsToCopyBack.push_back(i);
- }
-
varMap[param] = makeInlineVar(String(param->fName), &arguments[i]->type(),
param->fModifiers, &arguments[i]);
}
@@ -683,7 +655,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, hasEarlyReturn, *stmt));
}
if (hasEarlyReturn) {
// Since we output to backends that don't have a goto statement (which would normally be
@@ -700,23 +672,33 @@
inlinedBody.children().push_back(std::move(inlineBlock));
}
- // Copy back the values of `out` parameters into their real destinations.
- for (int i : argsToCopyBack) {
+ // Copy the values of `out` parameters into their destinations.
+ for (size_t i = 0; i < arguments.size(); ++i) {
const Variable* p = function.fDeclaration.fParameters[i];
- 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 (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())));
+ }
}
- if (resultExpr != nullptr) {
- // Return our result variable as our replacement expression.
- SkASSERT(resultExpr->as<VariableReference>().fRefKind == VariableReference::kRead_RefKind);
- inlinedCall.fReplacementExpr = std::move(resultExpr);
+ 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);
} 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.