Factor out Inliner candidate list assembly into its own function.

This greatly improves the output from a profiler. It makes it much
easier to determine how much time is spent in searching for candidates,
versus actually inlining them.

It also improves the code readability somewhat by breaking a large
monolithic function into several smaller functions.

Change-Id: I1b3ef6ddbe46af60e673f37ded766f8077ed6b03
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/321376
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>

Author: johnstiles-google

src/sksl/SkSLIRGenerator.cpp

@@ -2116,9 +2116,11 @@ std::unique_ptr<Expression> IRGenerator::call(int offset,
 
     auto funcCall = std::make_unique<FunctionCall>(offset, returnType, function,
                                                    std::move(arguments));
-    if (fCanInline && fInliner->isSafeToInline(*funcCall, fSettings->fInlineThreshold)) {
-        Inliner::InlinedCall inlinedCall =
-                fInliner->inlineCall(funcCall.get(), fSymbolTable.get(), fCurrentFunction);
+    if (fCanInline &&
+        fInliner->isSafeToInline(funcCall->fFunction.fDefinition) &&
+        !fInliner->isLargeFunction(funcCall->fFunction.fDefinition)) {
+        Inliner::InlinedCall inlinedCall = fInliner->inlineCall(funcCall.get(), fSymbolTable.get(),
+                                                                fCurrentFunction);
         if (inlinedCall.fInlinedBody) {
             fExtraStatements.push_back(std::move(inlinedCall.fInlinedBody));
         }

src/sksl/SkSLInliner.cpp

@@ -7,7 +7,7 @@
 
 #include "src/sksl/SkSLInliner.h"
 
-#include "limits.h"
+#include <limits.h>
 #include <memory>
 #include <unordered_set>
 
@@ -592,7 +592,7 @@ Inliner::InlinedCall Inliner::inlineCall(FunctionCall* call,
     SkASSERT(fSettings);
     SkASSERT(fContext);
     SkASSERT(call);
-    SkASSERT(this->isSafeToInline(*call, /*inlineThreshold=*/INT_MAX));
+    SkASSERT(this->isSafeToInline(call->fFunction.fDefinition));
 
     std::vector<std::unique_ptr<Expression>>& arguments = call->fArguments;
     const int offset = call->fOffset;
@@ -745,58 +745,52 @@ Inliner::InlinedCall Inliner::inlineCall(FunctionCall* call,
     return inlinedCall;
 }
 
-bool Inliner::isSafeToInline(const FunctionCall& functionCall, int inlineThreshold) {
+bool Inliner::isSafeToInline(const FunctionDefinition* functionDef) {
     SkASSERT(fSettings);
 
-    if (functionCall.fFunction.fDefinition == nullptr) {
+    if (functionDef == nullptr) {
         // Can't inline something if we don't actually have its definition.
         return false;
     }
-    const FunctionDefinition& functionDef = *functionCall.fFunction.fDefinition;
-    if (inlineThreshold < INT_MAX) {
-        if (!(functionDef.fDeclaration.fModifiers.fFlags & Modifiers::kInline_Flag) &&
-            Analysis::NodeCountExceeds(functionDef, inlineThreshold)) {
-            // The function exceeds our maximum inline size and is not flagged 'inline'.
-            return false;
-        }
-    }
+
     if (!fSettings->fCaps || !fSettings->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);
+        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);
+        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.
-    bool hasReturnInBreakableConstruct = (count_returns_in_breakable_constructs(functionDef) > 0);
+    bool hasReturnInBreakableConstruct = (count_returns_in_breakable_constructs(*functionDef) > 0);
 
     // If we detected returns in breakable constructs, we should also detect an early return.
-    SkASSERT(!hasReturnInBreakableConstruct || has_early_return(functionDef));
+    SkASSERT(!hasReturnInBreakableConstruct || has_early_return(*functionDef));
     return !hasReturnInBreakableConstruct;
 }
 
-bool Inliner::analyze(Program& program) {
-    // A candidate function for inlining, containing everything that `inlineCall` needs.
-    struct InlineCandidate {
-        SymbolTable* fSymbols;                        // the SymbolTable of the candidate
-        std::unique_ptr<Statement>* fParentStmt;      // the parent Statement of the enclosing stmt
-        std::unique_ptr<Statement>* fEnclosingStmt;   // the Statement containing the candidate
-        std::unique_ptr<Expression>* fCandidateExpr;  // the candidate FunctionCall to be inlined
-        FunctionDefinition* fEnclosingFunction;       // the Function containing the candidate
-    };
-
-    // This is structured much like a ProgramVisitor, but does not actually use ProgramVisitor.
-    // The analyzer needs to keep track of the `unique_ptr<T>*` of statements and expressions so
-    // that they can later be replaced, and ProgramVisitor does not provide this; it only provides a
-    // `const T&`.
-    class InlineCandidateAnalyzer {
+// A candidate function for inlining, containing everything that `inlineCall` needs.
+struct InlineCandidate {
+    SymbolTable* fSymbols;                        // the SymbolTable of the candidate
+    std::unique_ptr<Statement>* fParentStmt;      // the parent Statement of the enclosing stmt
+    std::unique_ptr<Statement>* fEnclosingStmt;   // the Statement containing the candidate
+    std::unique_ptr<Expression>* fCandidateExpr;  // the candidate FunctionCall to be inlined
+    FunctionDefinition* fEnclosingFunction;       // the Function containing the candidate
+    bool fIsLargeFunction;                        // does candidate exceed the inline threshold?
+};
+
+struct InlineCandidateList {
+    std::vector<InlineCandidate> fCandidates;
+};
+
+class InlineCandidateAnalyzer {
     public:
         // A list of all the inlining candidates we found during analysis.
-        std::vector<InlineCandidate> fInlineCandidates;
+        InlineCandidateList* fCandidateList;
+
         // A stack of the symbol tables; since most nodes don't have one, expected to be shallower
         // than the enclosing-statement stack.
         std::vector<SymbolTable*> fSymbolTableStack;
@@ -807,14 +801,16 @@ bool Inliner::analyze(Program& program) {
         // The function that we're currently processing (i.e. inlining into).
         FunctionDefinition* fEnclosingFunction = nullptr;
 
-        void visit(Program& program) {
+        void visit(Program& program, InlineCandidateList* candidateList) {
+            fCandidateList = candidateList;
             fSymbolTableStack.push_back(program.fSymbols.get());
 
             for (ProgramElement& pe : program) {
                 this->visitProgramElement(&pe);
             }
 
             fSymbolTableStack.pop_back();
+            fCandidateList = nullptr;
         }
 
         void visitProgramElement(ProgramElement* pe) {
@@ -1072,42 +1068,87 @@ bool Inliner::analyze(Program& program) {
         }
 
         void addInlineCandidate(std::unique_ptr<Expression>* candidate) {
-            fInlineCandidates.push_back(InlineCandidate{fSymbolTableStack.back(),
-                                                        find_parent_statement(fEnclosingStmtStack),
-                                                        fEnclosingStmtStack.back(),
-                                                        candidate,
-                                                        fEnclosingFunction});
+            fCandidateList->fCandidates.push_back(
+                    InlineCandidate{fSymbolTableStack.back(),
+                                    find_parent_statement(fEnclosingStmtStack),
+                                    fEnclosingStmtStack.back(),
+                                    candidate,
+                                    fEnclosingFunction,
+                                    /*isLargeFunction=*/false});
         }
-    };
+};
 
-    InlineCandidateAnalyzer analyzer;
-    analyzer.visit(program);
+bool Inliner::candidateCanBeInlined(const InlineCandidate& candidate, InlinabilityCache* cache) {
+    const FunctionDeclaration& funcDecl = (*candidate.fCandidateExpr)->as<FunctionCall>().fFunction;
 
-    // For each of our candidate function-call sites, check if it is actually safe to inline.
-    // Memoize our results so we don't check a function more than once.
-    std::unordered_map<const FunctionDeclaration*, bool> inlinableMap; // <function, safe-to-inline>
-    for (const InlineCandidate& candidate : analyzer.fInlineCandidates) {
-        const FunctionCall& funcCall = (*candidate.fCandidateExpr)->as<FunctionCall>();
-        const FunctionDeclaration* funcDecl = &funcCall.fFunction;
-        if (inlinableMap.find(funcDecl) == inlinableMap.end()) {
-            // We do not perform inlining on recursive calls to avoid an infinite death spiral of
-            // inlining.
-            int inlineThreshold = (funcDecl->fCallCount.load() > 1) ? fSettings->fInlineThreshold
-                                                                    : INT_MAX;
-            inlinableMap[funcDecl] = this->isSafeToInline(funcCall, inlineThreshold) &&
-                                     !contains_recursive_call(*funcDecl);
-        }
+    auto [iter, wasInserted] = cache->insert({&funcDecl, false});
+    if (wasInserted) {
+        // Recursion is forbidden here to avoid an infinite death spiral of inlining.
+        iter->second = this->isSafeToInline(funcDecl.fDefinition) &&
+                       !contains_recursive_call(funcDecl);
+    }
+
+    return iter->second;
+}
+
+bool Inliner::isLargeFunction(const FunctionDefinition* functionDef) {
+    return Analysis::NodeCountExceeds(*functionDef, fSettings->fInlineThreshold);
+}
+
+bool Inliner::isLargeFunction(const InlineCandidate& candidate, LargeFunctionCache* cache) {
+    const FunctionDeclaration& funcDecl = (*candidate.fCandidateExpr)->as<FunctionCall>().fFunction;
+
+    auto [iter, wasInserted] = cache->insert({&funcDecl, false});
+    if (wasInserted) {
+        iter->second = this->isLargeFunction(funcDecl.fDefinition);
     }
 
+    return iter->second;
+}
+
+void Inliner::buildCandidateList(Program& program, InlineCandidateList* candidateList) {
+    // This is structured much like a ProgramVisitor, but does not actually use ProgramVisitor.
+    // The analyzer needs to keep track of the `unique_ptr<T>*` of statements and expressions so
+    // that they can later be replaced, and ProgramVisitor does not provide this; it only provides a
+    // `const T&`.
+    InlineCandidateAnalyzer analyzer;
+    analyzer.visit(program, candidateList);
+
+    // Remove candidates that are not safe to inline.
+    std::vector<InlineCandidate>& candidates = candidateList->fCandidates;
+    InlinabilityCache cache;
+    candidates.erase(std::remove_if(candidates.begin(),
+                                    candidates.end(),
+                                    [&](const InlineCandidate& candidate) {
+                                        return !this->candidateCanBeInlined(candidate, &cache);
+                                    }),
+                     candidates.end());
+
+    // Determine whether each candidate function exceeds our inlining size threshold or not. These
+    // can still be valid candidates if they are only called one time, so we don't remove them from
+    // the candidate list, but they will not be inlined if they're called more than once.
+    LargeFunctionCache largeFunctionCache;
+    for (InlineCandidate& candidate : candidates) {
+        candidate.fIsLargeFunction = this->isLargeFunction(candidate, &largeFunctionCache);
+    }
+}
+
+bool Inliner::analyze(Program& program) {
+    InlineCandidateList candidateList;
+    this->buildCandidateList(program, &candidateList);
+
     // Inline the candidates where we've determined that it's safe to do so.
     std::unordered_set<const std::unique_ptr<Statement>*> enclosingStmtSet;
     bool madeChanges = false;
-    for (const InlineCandidate& candidate : analyzer.fInlineCandidates) {
+    for (const InlineCandidate& candidate : candidateList.fCandidates) {
         FunctionCall& funcCall = (*candidate.fCandidateExpr)->as<FunctionCall>();
         const FunctionDeclaration* funcDecl = &funcCall.fFunction;
 
-        // If we determined that this candidate was not actually inlinable, skip it.
-        if (!inlinableMap[funcDecl]) {
+        // If the function is large, not marked `inline`, and is called more than once, it's a bad
+        // idea to inline it.
+        if (candidate.fIsLargeFunction &&
+            !(funcDecl->fModifiers.fFlags & Modifiers::kInline_Flag) &&
+            funcDecl->fCallCount.load() > 1) {
             continue;
         }
 

src/sksl/SkSLInliner.h

@@ -20,6 +20,9 @@ class Block;
 class Context;
 struct Expression;
 struct FunctionCall;
+struct FunctionDefinition;
+struct InlineCandidate;
+struct InlineCandidateList;
 struct Statement;
 class SymbolTable;
 struct Variable;
@@ -50,8 +53,11 @@ class Inliner {
     /** Adds a scope to inlined bodies returned by `inlineCall`, if one is required. */
     void ensureScopedBlocks(Statement* inlinedBody, Statement* parentStmt);
 
-    /** Checks whether inlining is viable for a FunctionCall. */
-    bool isSafeToInline(const FunctionCall&, int inlineThreshold);
+    /** Checks whether inlining is viable for a FunctionCall, modulo recursion and function size. */
+    bool isSafeToInline(const FunctionDefinition* functionDef);
+
+    /** Checks whether a function's size exceeds the inline threshold from Settings. */
+    bool isLargeFunction(const FunctionDefinition* functionDef);
 
     /** Inlines any eligible functions that are found. Returns true if any changes are made. */
     bool analyze(Program& program);
@@ -61,6 +67,8 @@ class Inliner {
 
     String uniqueNameForInlineVar(const String& baseName, SymbolTable* symbolTable);
 
+    void buildCandidateList(Program& program, InlineCandidateList* candidateList);
+
     std::unique_ptr<Expression> inlineExpression(int offset,
                                                  VariableRewriteMap* varMap,
                                                  const Expression& expression);
@@ -72,6 +80,12 @@ class Inliner {
                                                const Statement& statement,
                                                bool isBuiltinCode);
 
+    using InlinabilityCache = std::unordered_map<const FunctionDeclaration*, bool>;
+    bool candidateCanBeInlined(const InlineCandidate& candidate, InlinabilityCache* cache);
+
+    using LargeFunctionCache = std::unordered_map<const FunctionDeclaration*, bool>;
+    bool isLargeFunction(const InlineCandidate& candidate, LargeFunctionCache* cache);
+
     const Context* fContext = nullptr;
     const Program::Settings* fSettings = nullptr;
     int fInlineVarCounter = 0;

tests/sksl/inliner/golden/InlineKeywordOverridesThreshold.glsl

@@ -38,7 +38,6 @@ void main() {
         ++y;
     }
 
-
     {
         ++y;
         ++y;
@@ -76,5 +75,4 @@ void main() {
         ++y;
     }
 
-
 }

tests/sksl/inliner/golden/InlineWithNestedBigCalls.glsl

@@ -40,7 +40,6 @@ void main() {
         --_1_x;
         _1_x = 456.0;
     }
-
     float _3_x = 456.0;
     {
         ++_3_x;
@@ -79,7 +78,7 @@ void main() {
         --_3_x;
         _3_x = 123.0;
     }
-
     sk_FragColor = vec4(123.0);
 
+
 }