Ensure that inlined statements are properly scoped.

Change-Id: I43479d8543ea4860be45614a65cf8ad4cec307d8
Bug: skia:10687
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/314877
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>

Author: johnstiles-google

src/sksl/SkSLCompiler.cpp

@@ -1642,7 +1642,7 @@ bool Compiler::optimize(Program& program) {
         fIRGenerator->fKind = program.fKind;
         fIRGenerator->fSettings = &program.fSettings;
 
-        // Build the control-flow graph for each function.
+        // Scan and optimize based on the control-flow graph for each function.
         for (ProgramElement& element : program) {
             if (element.fKind == ProgramElement::kFunction_Kind) {
                 this->scanCFG(element.as<FunctionDefinition>());

src/sksl/SkSLIRGenerator.cpp

@@ -534,6 +534,42 @@ std::unique_ptr<Statement> IRGenerator::convertIf(const ASTNode& n) {
                                                       std::move(ifTrue), std::move(ifFalse)));
 }
 
+static void ensure_scoped_blocks(Statement* stmt) {
+    // No changes necessary if this statement isn't actually a block.
+    if (stmt->fKind != Statement::kBlock_Kind) {
+        return;
+    }
+
+    Block& block = stmt->as<Block>();
+
+    // Occasionally, IR generation can lead to Blocks containing multiple statements, but no scope.
+    // If this block is used as the statement for a while/if/for, this isn't actually possible to
+    // represent textually; a scope must be added for the generated code to match the intent. In the
+    // case of Blocks nested inside other Blocks, we add the scope to the outermost block if needed.
+    // Zero-statement blocks have similar issues--if we don't represent the Block textually somehow,
+    // we run the risk of accidentally absorbing the following statement into our loop--so we also
+    // add a scope to these.
+    for (Block* nestedBlock = &block;; ) {
+        if (nestedBlock->fIsScope) {
+            // We found an explicit scope; all is well.
+            return;
+        }
+        if (nestedBlock->fStatements.size() != 1) {
+            // We found a block with multiple (or zero) statements, but no scope? Let's add a scope
+            // to the outermost block.
+            block.fIsScope = true;
+            return;
+        }
+        if (nestedBlock->fStatements[0]->fKind != Statement::kBlock_Kind) {
+            // This block has exactly one thing inside, and it's not another block. No need to scope
+            // it.
+            return;
+        }
+        // We have to go deeper.
+        nestedBlock = &nestedBlock->fStatements[0]->as<Block>();
+    }
+}
+
 std::unique_ptr<Statement> IRGenerator::convertFor(const ASTNode& f) {
     SkASSERT(f.fKind == ASTNode::Kind::kFor);
     AutoLoopLevel level(this);
@@ -571,6 +607,7 @@ std::unique_ptr<Statement> IRGenerator::convertFor(const ASTNode& f) {
     if (!statement) {
         return nullptr;
     }
+    ensure_scoped_blocks(statement.get());
     return std::make_unique<ForStatement>(f.fOffset, std::move(initializer), std::move(test),
                                           std::move(next), std::move(statement), fSymbolTable);
 }
@@ -591,6 +628,7 @@ std::unique_ptr<Statement> IRGenerator::convertWhile(const ASTNode& w) {
     if (!statement) {
         return nullptr;
     }
+    ensure_scoped_blocks(statement.get());
     return std::make_unique<WhileStatement>(w.fOffset, std::move(test), std::move(statement));
 }
 
@@ -602,6 +640,7 @@ std::unique_ptr<Statement> IRGenerator::convertDo(const ASTNode& d) {
     if (!statement) {
         return nullptr;
     }
+    ensure_scoped_blocks(statement.get());
     std::unique_ptr<Expression> test;
     {
         AutoDisableInline disableInline(this);

tests/SkSLFPTest.cpp

@@ -1228,8 +1228,6 @@ while (%s(%s)) {
 }
 
 DEF_TEST(SkSLFPInlinedWhileBodyMustBeInAScope, r) {
-    // NOTE: this test exposes a bug with the inliner. The inlined function body is not wrapped in a
-    // scope, so the inlined code is emitted outside of the while loop.
     test(r,
          *SkSL::ShaderCapsFactory::Default(),
          R"__SkSL__(
@@ -1249,13 +1247,14 @@ R"SkSL(return v + half4(0.125);
 )SkSL", &adjust_name);
         fragBuilder->codeAppendf(
 R"SkSL(%s = half4(0.0);
-while (%s.x < 0.5) half4 _0_adjust;
-{
-    _0_adjust = %s + half4(0.125);
-}
-
-%s = _0_adjust;
+while (%s.x < 0.5) {
+    half4 _0_adjust;
+    {
+        _0_adjust = %s + half4(0.125);
+    }
 
+    %s = _0_adjust;
+}
 )SkSL"
 , args.fOutputColor, args.fOutputColor, args.fOutputColor, args.fOutputColor);
 )__Cpp__"});
@@ -1293,8 +1292,6 @@ do {
 }
 
 DEF_TEST(SkSLFPInlinedDoWhileBodyMustBeInAScope, r) {
-    // NOTE: this test exposes a bug with the inliner. The inlined function body is not wrapped in a
-    // scope, so the emitted inlined code is just invalid.
     test(r,
          *SkSL::ShaderCapsFactory::Default(),
          R"__SkSL__(
@@ -1315,13 +1312,14 @@ R"SkSL(return v + half4(0.125);
 )SkSL", &adjust_name);
         fragBuilder->codeAppendf(
 R"SkSL(%s = half4(0.0);
-do half4 _0_adjust;
-{
-    _0_adjust = %s + half4(0.125);
-}
+do {
+    half4 _0_adjust;
+    {
+        _0_adjust = %s + half4(0.125);
+    }
 
-%s = _0_adjust;
- while (%s.x < 0.5);
+    %s = _0_adjust;
+} while (%s.x < 0.5);
 )SkSL"
 , args.fOutputColor, args.fOutputColor, args.fOutputColor, args.fOutputColor);
 )__Cpp__"});
@@ -1374,8 +1372,6 @@ R"SkSL(for (%s = half4(0.0625);
 }
 
 DEF_TEST(SkSLFPInlinedForBodyMustBeInAScope, r) {
-    // NOTE: this test exposes a bug with the inliner. The inlined function body is not wrapped in a
-    // scope, so the inlined code is emitted outside of the for loop.
     test(r,
          *SkSL::ShaderCapsFactory::Default(),
          R"__SkSL__(
@@ -1392,13 +1388,14 @@ DEF_TEST(SkSLFPInlinedForBodyMustBeInAScope, r) {
          /*expectedCPP=*/{
          R"__Cpp__(fragBuilder->codeAppendf(
 R"SkSL(%s = half4(0.0);
-for (int x = 0;x < 4; ++x) half4 _0_adjust;
-{
-    _0_adjust = %s + half4(0.125);
-}
-
-%s = _0_adjust;
+for (int x = 0;x < 4; ++x) {
+    half4 _0_adjust;
+    {
+        _0_adjust = %s + half4(0.125);
+    }
 
+    %s = _0_adjust;
+}
 )SkSL"
 , args.fOutputColor, args.fOutputColor, args.fOutputColor);
 )__Cpp__"});