Replace inliner do-while loops with for loops.

do-while loops aren't compatible with GLSL ES2. For-loops which run
only one time should work exactly the same for our purposes. We expect
such a loop to be unrolled by every driver, so it shouldn't come at any
performance cost.

Change-Id: Ia8de5fcab8128c34da97eaeaf81f91ad1ac36ce4
Bug: skia:11097
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345159
Reviewed-by: Brian Osman <[email protected]>
Commit-Queue: Brian Osman <[email protected]>
Auto-Submit: John Stiles <[email protected]>

Author: johnstiles-google

gn/sksl_tests.gni

@@ -447,7 +447,7 @@ sksl_settings_tests = [
   "$_tests/sksl/glsl/TypePrecision.sksl",
   "$_tests/sksl/inliner/ExponentialGrowth.sksl",
   "$_tests/sksl/inliner/InlinerCanBeDisabled.sksl",
-  "$_tests/sksl/inliner/InlinerWrapsEarlyReturnsWithDoWhileBlock.sksl",
+  "$_tests/sksl/inliner/InlinerWrapsEarlyReturnsWithForLoop.sksl",
   "$_tests/sksl/shared/Derivatives.sksl",
   "$_tests/sksl/shared/DerivativesFlipY.sksl",
   "$_tests/sksl/workarounds/AbsInt.sksl",

src/sksl/SkSLCompiler.cpp

@@ -87,7 +87,7 @@ class AutoSource {
 Compiler::Compiler(const ShaderCapsClass* caps, Flags flags)
         : fContext(std::make_shared<Context>())
         , fCaps(caps)
-        , fInliner(fContext.get(), fCaps)
+        , fInliner(fContext.get())
         , fFlags(flags)
         , fErrorCount(0) {
     SkASSERT(fCaps);

src/sksl/SkSLInliner.cpp

@@ -520,15 +520,15 @@ std::unique_ptr<Statement> Inliner::inlineStatement(int offset,
                     StatementArray block;
                     block.reserve_back(2);
                     block.push_back(std::move(assignment));
-                    block.push_back(std::make_unique<BreakStatement>(offset));
-                    return std::make_unique<Block>(offset, std::move(block), /*symbols=*/nullptr,
-                                                   /*isScope=*/true);
+                    block.push_back(std::make_unique<ContinueStatement>(offset));
+                    return std::make_unique<Block>(offset, std::move(block),
+                                                   /*symbols=*/nullptr, /*isScope=*/true);
                 } else {
                     return std::move(assignment);
                 }
             } else {
                 if (haveEarlyReturns) {
-                    return std::make_unique<BreakStatement>(offset);
+                    return std::make_unique<ContinueStatement>(offset);
                 } else {
                     return std::make_unique<Nop>();
                 }
@@ -577,6 +577,51 @@ std::unique_ptr<Statement> Inliner::inlineStatement(int offset,
     }
 }
 
+Inliner::InlineVariable Inliner::makeInlineVariable(const String& baseName,
+                                                    const Type* type,
+                                                    SymbolTable* symbolTable,
+                                                    Modifiers modifiers,
+                                                    bool isBuiltinCode,
+                                                    std::unique_ptr<Expression>* initialValue) {
+    // $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.
+    if (type == fContext->fFloatLiteral_Type.get()) {
+        SkDEBUGFAIL("found a $floatLiteral type while inlining");
+        type = fContext->fFloat_Type.get();
+    } else if (type == fContext->fIntLiteral_Type.get()) {
+        SkDEBUGFAIL("found an $intLiteral type while inlining");
+        type = fContext->fInt_Type.get();
+    }
+
+    // Provide our new variable with a unique name, and add it to our symbol table.
+    const String* namePtr = symbolTable->takeOwnershipOfString(
+            std::make_unique<String>(this->uniqueNameForInlineVar(baseName, symbolTable)));
+    StringFragment nameFrag{namePtr->c_str(), namePtr->length()};
+
+    // Create our new variable and add it to the symbol table.
+    InlineVariable result;
+    result.fVarSymbol =
+            symbolTable->add(std::make_unique<Variable>(/*offset=*/-1,
+                                                        fModifiers->addToPool(Modifiers()),
+                                                        nameFrag,
+                                                        type,
+                                                        isBuiltinCode,
+                                                        Variable::Storage::kLocal,
+                                                        initialValue->get()));
+
+    // Prepare the variable declaration (taking extra care with `out` params to not clobber any
+    // initial value).
+    if (*initialValue && (modifiers.fFlags & Modifiers::kOut_Flag)) {
+        result.fVarDecl = std::make_unique<VarDeclaration>(result.fVarSymbol, type, /*arraySize=*/0,
+                                                           (*initialValue)->clone());
+    } else {
+        result.fVarDecl = std::make_unique<VarDeclaration>(result.fVarSymbol, type, /*arraySize=*/0,
+                                                           std::move(*initialValue));
+    }
+    return result;
+}
+
 Inliner::InlinedCall Inliner::inlineCall(FunctionCall* call,
                                          std::shared_ptr<SymbolTable> symbolTable,
                                          const FunctionDeclaration* caller) {
@@ -610,59 +655,20 @@ Inliner::InlinedCall Inliner::inlineCall(FunctionCall* call,
             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)
+            1);                 // Block for inlined code
 
     inlinedBody.children().push_back(std::make_unique<InlineMarker>(&call->function()));
 
-    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.
-        if (type == fContext->fFloatLiteral_Type.get()) {
-            SkDEBUGFAIL("found a $floatLiteral type while inlining");
-            type = fContext->fFloat_Type.get();
-        } else if (type == fContext->fIntLiteral_Type.get()) {
-            SkDEBUGFAIL("found an $intLiteral type while inlining");
-            type = fContext->fInt_Type.get();
-        }
-
-        // Provide our new variable with a unique name, and add it to our symbol table.
-        const String* namePtr = symbolTable->takeOwnershipOfString(std::make_unique<String>(
-                this->uniqueNameForInlineVar(baseName, symbolTable.get())));
-        StringFragment nameFrag{namePtr->c_str(), namePtr->length()};
-
-        // Add our new variable to the symbol table.
-        const Variable* variableSymbol = symbolTable->add(std::make_unique<Variable>(
-                                                 /*offset=*/-1, fModifiers->addToPool(Modifiers()),
-                                                 nameFrag, type, caller->isBuiltin(),
-                                                 Variable::Storage::kLocal, initialValue->get()));
-
-        // Prepare the variable declaration (taking extra care with `out` params to not clobber any
-        // initial value).
-        std::unique_ptr<Statement> variable;
-        if (initialValue && (modifiers.fFlags & Modifiers::kOut_Flag)) {
-            variable = std::make_unique<VarDeclaration>(variableSymbol, type, /*arraySize=*/0,
-                                                        (*initialValue)->clone());
-        } else {
-            variable = std::make_unique<VarDeclaration>(variableSymbol, type, /*arraySize=*/0,
-                                                        std::move(*initialValue));
-        }
-
-        // Add the new variable-declaration statement to our block of extra statements.
-        inlinedBody.children().push_back(std::move(variable));
-
-        return std::make_unique<VariableReference>(offset, variableSymbol);
-    };
-
     // Create a variable to hold the result in the extra statements (excepting void).
     std::unique_ptr<Expression> resultExpr;
     if (function.declaration().returnType() != *fContext->fVoid_Type) {
         std::unique_ptr<Expression> noInitialValue;
-        resultExpr = makeInlineVar(String(function.declaration().name()),
-                                   &function.declaration().returnType(),
-                                   Modifiers{}, &noInitialValue);
+        InlineVariable var = this->makeInlineVariable(function.declaration().name(),
+                                                      &function.declaration().returnType(),
+                                                      symbolTable.get(), Modifiers{},
+                                                      caller->isBuiltin(), &noInitialValue);
+        inlinedBody.children().push_back(std::move(var.fVarDecl));
+        resultExpr = std::make_unique<VariableReference>(/*offset=*/-1, var.fVarSymbol);
    }
 
     // Create variables in the extra statements to hold the arguments, and assign the arguments to
@@ -682,44 +688,81 @@ Inliner::InlinedCall Inliner::inlineCall(FunctionCall* call,
                 continue;
             }
         }
-
         if (isOutParam) {
             argsToCopyBack.push_back(i);
         }
-
-        varMap[param] = makeInlineVar(String(param->name()), &arguments[i]->type(),
-                                      param->modifiers(), &arguments[i]);
+        InlineVariable var = this->makeInlineVariable(param->name(), &arguments[i]->type(),
+                                                      symbolTable.get(), param->modifiers(),
+                                                      caller->isBuiltin(), &arguments[i]);
+        inlinedBody.children().push_back(std::move(var.fVarDecl));
+        varMap[param] = std::make_unique<VariableReference>(/*offset=*/-1, var.fVarSymbol);
     }
 
     const Block& body = function.body()->as<Block>();
-    auto inlineBlock = std::make_unique<Block>(offset, StatementArray{},
-                                               /*symbols=*/nullptr, /*isScope=*/hasEarlyReturn);
-    inlineBlock->children().reserve_back(body.children().size());
-    for (const std::unique_ptr<Statement>& stmt : body.children()) {
-        inlineBlock->children().push_back(this->inlineStatement(offset, &varMap, symbolTable.get(),
-                                                                resultExpr.get(), hasEarlyReturn,
-                                                                *stmt, caller->isBuiltin()));
-    }
+    StatementArray* inlineStatements;
+
     if (hasEarlyReturn) {
         // Since we output to backends that don't have a goto statement (which would normally be
-        // used to perform an early return), we fake it by wrapping the function in a
-        // do { } while (false); and then use break statements to jump to the end in order to
-        // emulate a goto.
-        inlinedBody.children().push_back(std::make_unique<DoStatement>(
+        // used to perform an early return), we fake it by wrapping the function in a single-
+        // iteration for loop, and use a continue statement to jump to the end of the loop
+        // prematurely.
+
+        // int _1_loop = 0;
+        symbolTable = std::make_shared<SymbolTable>(std::move(symbolTable), caller->isBuiltin());
+        const Type* intType = fContext->fInt_Type.get();
+        std::unique_ptr<Expression> initialValue = std::make_unique<IntLiteral>(/*offset=*/-1,
+                                                                                /*value=*/0,
+                                                                                intType);
+        InlineVariable loopVar = this->makeInlineVariable("loop", intType, symbolTable.get(),
+                                                          Modifiers{}, caller->isBuiltin(),
+                                                          &initialValue);
+
+        // _1_loop < 1;
+        std::unique_ptr<Expression> test = std::make_unique<BinaryExpression>(
                 /*offset=*/-1,
-                std::move(inlineBlock),
-                std::make_unique<BoolLiteral>(*fContext, offset, /*value=*/false)));
+                std::make_unique<VariableReference>(/*offset=*/-1, loopVar.fVarSymbol),
+                Token::Kind::TK_LT,
+                std::make_unique<IntLiteral>(/*offset=*/-1, /*value=*/1, intType),
+                fContext->fBool_Type.get());
+
+        // _1_loop++
+        std::unique_ptr<Expression> increment = std::make_unique<PostfixExpression>(
+                std::make_unique<VariableReference>(/*offset=*/-1, loopVar.fVarSymbol,
+                                                    VariableReference::RefKind::kReadWrite),
+                Token::Kind::TK_PLUSPLUS);
+
+        // {...}
+        auto innerBlock = std::make_unique<Block>(offset, StatementArray{},
+                                                  /*symbols=*/nullptr, /*isScope=*/true);
+        inlineStatements = &innerBlock->children();
+
+        // for (int _1_loop = 0; _1_loop < 1; _1_loop++) {...}
+        inlinedBody.children().push_back(std::make_unique<ForStatement>(/*offset=*/-1,
+                                                                        std::move(loopVar.fVarDecl),
+                                                                        std::move(test),
+                                                                        std::move(increment),
+                                                                        std::move(innerBlock),
+                                                                        symbolTable));
     } else {
-        // No early returns, so we can just dump the code in. We still need to keep the block so we
-        // don't get name conflicts with locals.
-        inlinedBody.children().push_back(std::move(inlineBlock));
+        // No early returns, so we can just dump the code into a scopeless block.
+        auto innerBlock = std::make_unique<Block>(offset, StatementArray{},
+                                                  /*symbols=*/nullptr, /*isScope=*/false);
+        inlineStatements = &innerBlock->children();
+        inlinedBody.children().push_back(std::move(innerBlock));
+    }
+
+    inlineStatements->reserve_back(body.children().size() + argsToCopyBack.size());
+    for (const std::unique_ptr<Statement>& stmt : body.children()) {
+        inlineStatements->push_back(this->inlineStatement(offset, &varMap, symbolTable.get(),
+                                                          resultExpr.get(), hasEarlyReturn, *stmt,
+                                                          caller->isBuiltin()));
     }
 
     // Copy back the values of `out` parameters into their real destinations.
     for (int i : argsToCopyBack) {
         const Variable* p = function.declaration().parameters()[i];
         SkASSERT(varMap.find(p) != varMap.end());
-        inlinedBody.children().push_back(
+        inlineStatements->push_back(
                 std::make_unique<ExpressionStatement>(std::make_unique<BinaryExpression>(
                         offset,
                         clone_with_ref_kind(*arguments[i], VariableReference::RefKind::kWrite),
@@ -762,18 +805,8 @@ bool Inliner::isSafeToInline(const FunctionDefinition* functionDef) {
         return false;
     }
 
-    if (!fCaps || !fCaps->canUseDoLoops()) {
-        // We don't have do-while loops. We use do-while loops to simulate early returns, so we
-        // can't inline functions that have an early return.
-        bool hasEarlyReturn = has_early_return(*functionDef);
-
-        // If we didn't detect an early return, there shouldn't be any returns in breakable
-        // constructs either.
-        SkASSERT(hasEarlyReturn || count_returns_in_breakable_constructs(*functionDef) == 0);
-        return !hasEarlyReturn;
-    }
-    // We have do-while loops, but we don't have any mechanism to simulate early returns within a
-    // breakable construct (switch/for/do/while), so we can't inline if there's a return inside one.
+    // We don't have any mechanism to simulate early returns within a breakable construct
+    // (switch/for/do/while), so we can't inline if there's a return inside one.
     bool hasReturnInBreakableConstruct = (count_returns_in_breakable_constructs(*functionDef) > 0);
 
     // If we detected returns in breakable constructs, we should also detect an early return.

src/sksl/SkSLInliner.h

@@ -36,7 +36,7 @@ class Variable;
  */
 class Inliner {
 public:
-    Inliner(const Context* context, const ShaderCapsClass* caps) : fContext(context), fCaps(caps) {}
+    Inliner(const Context* context) : fContext(context) {}
 
     void reset(ModifiersPool* modifiers, const Program::Settings*);
 
@@ -85,6 +85,18 @@ class Inliner {
                            std::shared_ptr<SymbolTable>,
                            const FunctionDeclaration* caller);
 
+    /** Creates a scratch variable for the inliner to use. */
+    struct InlineVariable {
+        const Variable*             fVarSymbol;
+        std::unique_ptr<Statement>  fVarDecl;
+    };
+    InlineVariable makeInlineVariable(const String& baseName,
+                                      const Type* type,
+                                      SymbolTable* symbolTable,
+                                      Modifiers modifiers,
+                                      bool isBuiltinCode,
+                                      std::unique_ptr<Expression>* initialValue);
+
     /** Adds a scope to inlined bodies returned by `inlineCall`, if one is required. */
     void ensureScopedBlocks(Statement* inlinedBody, Statement* parentStmt);
 
@@ -94,7 +106,6 @@ class Inliner {
     const Context* fContext = nullptr;
     ModifiersPool* fModifiers = nullptr;
     const Program::Settings* fSettings = nullptr;
-    const ShaderCapsClass* fCaps = nullptr;
     int fInlineVarCounter = 0;
     int fInlinedStatementCounter = 0;
 };

tests/sksl/inliner/InlinerWrapsEarlyReturnsWithDoWhileBlock.sksl renamed to tests/sksl/inliner/InlinerWrapsEarlyReturnsWithForLoop.sksl

@@ -3,20 +3,20 @@ out vec4 sk_FragColor;
 uniform vec4 color;
 void main() {
     vec4 _0_returny;
-    do {
+    for (int _1_loop = 0;_1_loop < 1; _1_loop++) {
         if (color.x > color.y) {
             _0_returny = color.xxxx;
-            break;
+            continue;
         }
         if (color.y > color.z) {
             _0_returny = color.yyyy;
-            break;
+            continue;
         }
         {
             _0_returny = color.zzzz;
-            break;
+            continue;
         }
-    } while (false);
+    }
     sk_FragColor = _0_returny;
 
 }

tests/sksl/inliner/golden/InlinerWrapsEarlyReturnsWithDoWhileBlockStandaloneSettings.glsl

@@ -0,0 +1,22 @@
+
+out vec4 sk_FragColor;
+uniform vec4 color;
+void main() {
+    vec4 _0_returny;
+    for (int _1_loop = 0;_1_loop < 1; _1_loop++) {
+        if (color.x > color.y) {
+            _0_returny = color.xxxx;
+            continue;
+        }
+        if (color.y > color.z) {
+            _0_returny = color.yyyy;
+            continue;
+        }
+        {
+            _0_returny = color.zzzz;
+            continue;
+        }
+    }
+    sk_FragColor = _0_returny;
+
+}