Elide return expression temp-var in vardecl-less blocks.
Previously, a return statement inside a scoped Block would always result
in the return expression being assigned to a temporary variable instead
of replacing the function-call-expression directly. This was done
because there might be variables inside the Block; these would have
fallen out of scope when the expression is migrated to the call site,
resulting in an invalid expression.
We aren't actually examining the return expression so we don't know if
it uses variables from an inner scope at all. (Inspecting the return
expression for variable usage is certainly possible! But it's a fair
amount of code and complexity for a small payoff.)
However, we can very easily get most of the benefit here without paying
for the complexity. In this CL we now look for variable declarations
inside of scoped Blocks. If the code doesn't add any vardecls into
scoped Blocks, there's no risk of scope problems, and we don't need to
use a temp-var to store our return expressions. If any vardecls are
added, we go back to using a temp-var as before.
Change-Id: I4c81400dad2f33db06a1c18eb671ba2140232006
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/346499
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index 9c59bc4..5740e90 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -391,8 +391,8 @@
"$_tests/sksl/inliner/IfWithReturnsCanBeInlined.sksl",
"$_tests/sksl/inliner/InlineKeywordOverridesThreshold.sksl",
"$_tests/sksl/inliner/InlineThreshold.sksl",
+ "$_tests/sksl/inliner/InlinerElidesTempVarForReturnsInsideBlock.sksl",
"$_tests/sksl/inliner/InlinerUsesTempVarForMultipleReturns.sksl",
- "$_tests/sksl/inliner/InlinerUsesTempVarForReturnsInsideBlock.sksl",
"$_tests/sksl/inliner/InlinerUsesTempVarForReturnsInsideBlockWithVar.sksl",
"$_tests/sksl/inliner/InlineWithInoutArgument.sksl",
"$_tests/sksl/inliner/InlineWithModifiedArgument.sksl",
diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp
index f6c5465..af03768 100644
--- a/src/sksl/SkSLInliner.cpp
+++ b/src/sksl/SkSLInliner.cpp
@@ -161,9 +161,8 @@
static const Type* copy_if_needed(const Type* src, SymbolTable& symbolTable) {
if (src->isArray()) {
- const Type* innerType = copy_if_needed(&src->componentType(), symbolTable);
- return symbolTable.takeOwnershipOfSymbol(Type::MakeArrayType(src->name(), *innerType,
- src->columns()));
+ return symbolTable.takeOwnershipOfSymbol(
+ Type::MakeArrayType(src->name(), src->componentType(), src->columns()));
}
return src;
}
@@ -225,11 +224,24 @@
fDeepestReturn = std::max(fDeepestReturn, fScopedBlockDepth);
return (fNumReturns >= fLimit) || INHERITED::visitStatement(stmt);
}
+ case Statement::Kind::kVarDeclaration: {
+ if (fScopedBlockDepth > 1) {
+ fVariablesInBlocks = true;
+ }
+ return INHERITED::visitStatement(stmt);
+ }
case Statement::Kind::kBlock: {
int depthIncrement = stmt.as<Block>().isScope() ? 1 : 0;
fScopedBlockDepth += depthIncrement;
bool result = INHERITED::visitStatement(stmt);
fScopedBlockDepth -= depthIncrement;
+ if (fNumReturns == 0 && fScopedBlockDepth <= 1) {
+ // If closing this block puts us back at the top level, and we haven't
+ // encountered any return statements yet, any vardecls we may have encountered
+ // up until this point can be ignored. They are out of scope now, and they were
+ // never used in a return statement.
+ fVariablesInBlocks = false;
+ }
return result;
}
default:
@@ -241,6 +253,7 @@
int fDeepestReturn = 0;
int fLimit = 0;
int fScopedBlockDepth = 0;
+ bool fVariablesInBlocks = false;
using INHERITED = ProgramVisitor;
};
@@ -249,14 +262,16 @@
Inliner::ReturnComplexity Inliner::GetReturnComplexity(const FunctionDefinition& funcDef) {
int returnsAtEndOfControlFlow = count_returns_at_end_of_control_flow(funcDef);
CountReturnsWithLimit counter{funcDef, returnsAtEndOfControlFlow + 1};
-
if (counter.fNumReturns > returnsAtEndOfControlFlow) {
return ReturnComplexity::kEarlyReturns;
}
- if (counter.fNumReturns > 1 || counter.fDeepestReturn > 1) {
+ if (counter.fNumReturns > 1) {
return ReturnComplexity::kScopedReturns;
}
- return ReturnComplexity::kSingleTopLevelReturn;
+ if (counter.fVariablesInBlocks && counter.fDeepestReturn > 1) {
+ return ReturnComplexity::kScopedReturns;
+ }
+ return ReturnComplexity::kSingleSafeReturn;
}
void Inliner::ensureScopedBlocks(Statement* inlinedBody, Statement* parentStmt) {
@@ -532,12 +547,12 @@
}
}
- // For a function that only contains a single top-level return, we don't need to store
- // the result in a variable at all. Just move the return value right into the result
- // expression.
+ // If a function only contains a single return, and it doesn't reference variables from
+ // inside an Block's scope, we don't need to store the result in a variable at all. Just
+ // replace the function-call expression with the function's return expression.
SkASSERT(resultExpr);
SkASSERT(*resultExpr);
- if (returnComplexity <= ReturnComplexity::kSingleTopLevelReturn) {
+ if (returnComplexity <= ReturnComplexity::kSingleSafeReturn) {
*resultExpr = expr(r.expression());
return std::make_unique<Nop>();
}
diff --git a/src/sksl/SkSLInliner.h b/src/sksl/SkSLInliner.h
index 74da6b6..45aef16 100644
--- a/src/sksl/SkSLInliner.h
+++ b/src/sksl/SkSLInliner.h
@@ -49,7 +49,7 @@
using VariableRewriteMap = std::unordered_map<const Variable*, std::unique_ptr<Expression>>;
enum class ReturnComplexity {
- kSingleTopLevelReturn,
+ kSingleSafeReturn,
kScopedReturns,
kEarlyReturns,
};
diff --git a/tests/sksl/inliner/InlinerUsesTempVarForReturnsInsideBlock.sksl b/tests/sksl/inliner/InlinerElidesTempVarForReturnsInsideBlock.sksl
similarity index 76%
rename from tests/sksl/inliner/InlinerUsesTempVarForReturnsInsideBlock.sksl
rename to tests/sksl/inliner/InlinerElidesTempVarForReturnsInsideBlock.sksl
index 839e462..e7e0ee2 100644
--- a/tests/sksl/inliner/InlinerUsesTempVarForReturnsInsideBlock.sksl
+++ b/tests/sksl/inliner/InlinerElidesTempVarForReturnsInsideBlock.sksl
@@ -4,6 +4,10 @@
inline half4 MakeTempVar(half4 c) {
{
+ half4 d = c * 0.75;
+ c = d;
+ }
+ {
return c.xxxx;
}
}
diff --git a/tests/sksl/inliner/golden/InlineWithUnnecessaryBlocks.glsl b/tests/sksl/inliner/golden/InlineWithUnnecessaryBlocks.glsl
index 5c27d96..e6c68af 100644
--- a/tests/sksl/inliner/golden/InlineWithUnnecessaryBlocks.glsl
+++ b/tests/sksl/inliner/golden/InlineWithUnnecessaryBlocks.glsl
@@ -2,10 +2,6 @@
out vec4 sk_FragColor;
uniform vec4 color;
void main() {
- vec4 _0_blocky;
- {
- _0_blocky = color;
- }
- sk_FragColor = _0_blocky;
+ sk_FragColor = color;
}
diff --git a/tests/sksl/inliner/golden/InlinerElidesTempVarForReturnsInsideBlock.glsl b/tests/sksl/inliner/golden/InlinerElidesTempVarForReturnsInsideBlock.glsl
new file mode 100644
index 0000000..76d11ed
--- /dev/null
+++ b/tests/sksl/inliner/golden/InlinerElidesTempVarForReturnsInsideBlock.glsl
@@ -0,0 +1,12 @@
+#version 400
+out vec4 sk_FragColor;
+uniform vec4 color;
+void main() {
+ vec4 _1_c = color;
+ {
+ vec4 _2_d = _1_c * 0.75;
+ _1_c = _2_d;
+ }
+ sk_FragColor = _1_c.xxxx;
+
+}
diff --git a/tests/sksl/inliner/golden/InlinerUsesTempVarForReturnsInsideBlock.glsl b/tests/sksl/inliner/golden/InlinerUsesTempVarForReturnsInsideBlock.glsl
deleted file mode 100644
index aeb77f7..0000000
--- a/tests/sksl/inliner/golden/InlinerUsesTempVarForReturnsInsideBlock.glsl
+++ /dev/null
@@ -1,11 +0,0 @@
-#version 400
-out vec4 sk_FragColor;
-uniform vec4 color;
-void main() {
- vec4 _0_MakeTempVar;
- {
- _0_MakeTempVar = color.xxxx;
- }
- sk_FragColor = _0_MakeTempVar;
-
-}
