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>

gn/sksl_tests.gni

@@ -391,8 +391,8 @@ sksl_inliner_tests = [
   "$_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",

src/sksl/SkSLInliner.cpp

@@ -161,9 +161,8 @@ static bool contains_recursive_call(const FunctionDeclaration& funcDecl) {
 
 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 @@ public:
                 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 @@ public:
     int fDeepestReturn = 0;
     int fLimit = 0;
     int fScopedBlockDepth = 0;
+    bool fVariablesInBlocks = false;
     using INHERITED = ProgramVisitor;
 };
 
@@ -249,14 +262,16 @@ public:
 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 @@ std::unique_ptr<Statement> Inliner::inlineStatement(int offset,
                 }
             }
 
-            // 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>();
             }

src/sksl/SkSLInliner.h

@@ -49,7 +49,7 @@ private:
     using VariableRewriteMap = std::unordered_map<const Variable*, std::unique_ptr<Expression>>;
 
     enum class ReturnComplexity {
-        kSingleTopLevelReturn,
+        kSingleSafeReturn,
         kScopedReturns,
         kEarlyReturns,
     };

tests/sksl/inliner/InlinerElidesTempVarForReturnsInsideBlock.sksl

@@ -3,6 +3,10 @@
 uniform half4 color;
 
 inline half4 MakeTempVar(half4 c) {
+    {
+        half4 d = c * 0.75;
+        c = d;
+    }
     {
         return c.xxxx;
     }

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;
 
 }

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;
+
+}

tests/sksl/inliner/golden/InlinerUsesTempVarForReturnsInsideBlock.glsl

@@ -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;
-
-}