AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Revert "Add InlineCandidateAnalyzer to locate candidate functions for inlining."

Browse Source 
This reverts commit ff9dc8240e.

Reason for revert: tree on fire

Original change's description:
> Add InlineCandidateAnalyzer to locate candidate functions for inlining.
> 
> The analyzer will be enabled in followup CLs. At present, if the
> commented-out code is enabled, it will properly identify candidate
> functions that should be inlined.
> 
> Change-Id: Icb6e7c2394a3828df81f75b855c69a25bf297842
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/315217
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>

TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com

Change-Id: I1f07b406c4d56f2fdb6d3c5360f15397fc26d783
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/316448
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This commit is contained in:
John Stiles 2020-09-11 15:02:06 +00:00 committed by Skia Commit-Bot
parent a003e8184e
commit ab5010493d
3 changed files with 11 additions and 333 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
src/sksl/SkSLCompiler.cpp
View File

@ -1676,9 +1676,6 @@ bool Compiler::optimize(Program& program) {
}
}
// Allow the inliner to analyze the program.
madeChanges |= fInliner.analyze(program);
// Remove dead functions. We wait until after analysis so that we still report errors,
// even in unused code.
if (program.fSettings.fRemoveDeadFunctions) {

335
src/sksl/SkSLInliner.cpp
View File

@ -71,7 +71,7 @@ static int count_all_returns(const FunctionDefinition& funcDef) {
[[fallthrough]];
default:
return INHERITED::visitStatement(stmt);
return this->INHERITED::visitStatement(stmt);
}
}
@ -109,7 +109,7 @@ static int count_returns_at_end_of_control_flow(const FunctionDefinition& funcDe
[[fallthrough]];
default:
return INHERITED::visitStatement(stmt);
return this->INHERITED::visitStatement(stmt);
}
}
@ -134,7 +134,7 @@ static int count_returns_in_breakable_constructs(const FunctionDefinition& funcD
case Statement::Kind::kDo:
case Statement::Kind::kFor: {
++fInsideBreakableConstruct;
bool result = INHERITED::visitStatement(stmt);
bool result = this->INHERITED::visitStatement(stmt);
--fInsideBreakableConstruct;
return result;
}
@ -144,7 +144,7 @@ static int count_returns_in_breakable_constructs(const FunctionDefinition& funcD
[[fallthrough]];
default:
return INHERITED::visitStatement(stmt);
return this->INHERITED::visitStatement(stmt);
}
}
@ -629,7 +629,8 @@ Inliner::InlinedCall Inliner::inlineCall(FunctionCall* call,
return inlinedCall;
}
bool Inliner::isSafeToInline(const FunctionCall& functionCall, int inlineThreshold) {
bool Inliner::isSafeToInline(const FunctionCall& functionCall,
int inlineThreshold) {
SkASSERT(fSettings);
if (functionCall.fFunction.fDefinition == nullptr) {
@ -644,6 +645,11 @@ bool Inliner::isSafeToInline(const FunctionCall& functionCall, int inlineThresho
return false;
}
}
if (contains_recursive_call(functionCall.fFunction)) {
// We do not perform inlining on recursive calls to avoid an infinite death spiral of
// inlining.
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.
@ -663,323 +669,4 @@ bool Inliner::isSafeToInline(const FunctionCall& functionCall, int inlineThresho
return !hasReturnInBreakableConstruct;
}
bool Inliner::analyze(Program& program) {
// A candidate function for inlining, containing everything that `inlineCall` needs.
struct InlineCandidate {
SymbolTable* fSymbols;
std::unique_ptr<Statement>* fEnclosingStmt;
std::unique_ptr<Expression>* fCandidateExpr;
};
// 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 {
public:
// A list of all the inlining candidates we found during analysis.
std::vector<InlineCandidate> fInlineCandidates;
// 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;
// A stack of "enclosing" statements--these would be suitable for the inliner to use for
// adding new instructions. Not all statements are suitable (e.g. a for-loop's initializer).
// The inliner might replace a statement with a block containing the statement.
std::vector<std::unique_ptr<Statement>*> fEnclosingStmtStack;
void visit(Program& program) {
fSymbolTableStack.push_back(program.fSymbols.get());
for (ProgramElement& pe : program) {
this->visitProgramElement(&pe);
}
fSymbolTableStack.pop_back();
}
void visitProgramElement(ProgramElement* pe) {
switch (pe->kind()) {
case ProgramElement::Kind::kFunction: {
FunctionDefinition& funcDef = pe->as<FunctionDefinition>();
this->visitStatement(&funcDef.fBody);
break;
}
default:
// The inliner can't operate outside of a function's scope.
break;
}
}
void visitStatement(std::unique_ptr<Statement>* stmt,
bool isViableAsEnclosingStatement = true) {
if (!*stmt) {
return;
}
size_t oldEnclosingStmtStackSize = fEnclosingStmtStack.size();
size_t oldSymbolStackSize = fSymbolTableStack.size();
if (isViableAsEnclosingStatement) {
fEnclosingStmtStack.push_back(stmt);
}
switch ((*stmt)->kind()) {
case Statement::Kind::kBreak:
case Statement::Kind::kContinue:
case Statement::Kind::kDiscard:
case Statement::Kind::kInlineMarker:
case Statement::Kind::kNop:
break;
case Statement::Kind::kBlock: {
Block& block = (*stmt)->as<Block>();
if (block.fSymbols) {
fSymbolTableStack.push_back(block.fSymbols.get());
}
for (std::unique_ptr<Statement>& blockStmt : block.fStatements) {
this->visitStatement(&blockStmt);
}
break;
}
case Statement::Kind::kDo: {
DoStatement& doStmt = (*stmt)->as<DoStatement>();
// The loop body is a candidate for inlining.
this->visitStatement(&doStmt.fStatement);
// The inliner isn't smart enough to inline the test-expression for a do-while
// loop at this time. There are two limitations:
// - We would need to insert the inlined-body block at the very end of the do-
// statement's inner fStatement. We don't support that today, but it's doable.
// - We cannot inline the test expression if the loop uses `continue` anywhere;
// that would skip over the inlined block that evaluates the test expression.
// There isn't a good fix for this--any workaround would be more complex than
// the cost of a function call. However, loops that don't use `continue` would
// still be viable candidates for inlining.
break;
}
case Statement::Kind::kExpression: {
ExpressionStatement& expr = (*stmt)->as<ExpressionStatement>();
this->visitExpression(&expr.fExpression);
break;
}
case Statement::Kind::kFor: {
ForStatement& forStmt = (*stmt)->as<ForStatement>();
if (forStmt.fSymbols) {
fSymbolTableStack.push_back(forStmt.fSymbols.get());
}
// The initializer and loop body are candidates for inlining.
this->visitStatement(&forStmt.fInitializer,
/*isViableAsEnclosingStatement=*/false);
this->visitStatement(&forStmt.fStatement);
// The inliner isn't smart enough to inline the test- or increment-expressions
// of a for loop loop at this time. There are a handful of limitations:
// - We would need to insert the test-expression block at the very beginning of
// the for-loop's inner fStatement, and the increment-expression block at the
// very end. We don't support that today, but it's doable.
// - The for-loop's built-in test-expression would need to be dropped entirely,
// and the loop would be halted via a break statement at the end of the
// inlined test-expression. This is again something we don't support today,
// but it could be implemented.
// - We cannot inline the increment-expression if the loop uses `continue`
// anywhere; that would skip over the inlined block that evaluates the
// increment expression. There isn't a good fix for this--any workaround would
// be more complex than the cost of a function call. However, loops that don't
// use `continue` would still be viable candidates for increment-expression
// inlining.
break;
}
case Statement::Kind::kIf: {
IfStatement& ifStmt = (*stmt)->as<IfStatement>();
this->visitExpression(&ifStmt.fTest);
this->visitStatement(&ifStmt.fIfTrue);
this->visitStatement(&ifStmt.fIfFalse);
break;
}
case Statement::Kind::kReturn: {
ReturnStatement& returnStmt = (*stmt)->as<ReturnStatement>();
this->visitExpression(&returnStmt.fExpression);
break;
}
case Statement::Kind::kSwitch: {
SwitchStatement& switchStmt = (*stmt)->as<SwitchStatement>();
if (switchStmt.fSymbols) {
fSymbolTableStack.push_back(switchStmt.fSymbols.get());
}
this->visitExpression(&switchStmt.fValue);
for (std::unique_ptr<SwitchCase>& switchCase : switchStmt.fCases) {
// The switch-case's fValue cannot be a FunctionCall; skip it.
for (std::unique_ptr<Statement>& caseBlock : switchCase->fStatements) {
this->visitStatement(&caseBlock);
}
}
break;
}
case Statement::Kind::kVarDeclaration: {
VarDeclaration& varDeclStmt = (*stmt)->as<VarDeclaration>();
// Don't need to scan the declaration's sizes; those are always IntLiterals.
this->visitExpression(&varDeclStmt.fValue);
break;
}
case Statement::Kind::kVarDeclarations: {
VarDeclarationsStatement& varDecls = (*stmt)->as<VarDeclarationsStatement>();
for (std::unique_ptr<Statement>& varDecl : varDecls.fDeclaration->fVars) {
this->visitStatement(&varDecl, /*isViableAsEnclosingStatement=*/false);
}
break;
}
case Statement::Kind::kWhile: {
WhileStatement& whileStmt = (*stmt)->as<WhileStatement>();
// The loop body is a candidate for inlining.
this->visitStatement(&whileStmt.fStatement);
// The inliner isn't smart enough to inline the test-expression for a while
// loop at this time. There are two limitations:
// - We would need to insert the inlined-body block at the very beginning of the
// while loop's inner fStatement. We don't support that today, but it's
// doable.
// - The while-loop's built-in test-expression would need to be replaced with a
// `true` BoolLiteral, and the loop would be halted via a break statement at
// the end of the inlined test-expression. This is again something we don't
// support today, but it could be implemented.
break;
}
default:
SkUNREACHABLE;
}
// Pop our symbol and enclosing-statement stacks.
fSymbolTableStack.resize(oldSymbolStackSize);
fEnclosingStmtStack.resize(oldEnclosingStmtStackSize);
}
void visitExpression(std::unique_ptr<Expression>* expr) {
if (!*expr) {
return;
}
switch ((*expr)->kind()) {
case Expression::Kind::kBoolLiteral:
case Expression::Kind::kDefined:
case Expression::Kind::kExternalValue:
case Expression::Kind::kFieldAccess:
case Expression::Kind::kFloatLiteral:
case Expression::Kind::kFunctionReference:
case Expression::Kind::kIntLiteral:
case Expression::Kind::kNullLiteral:
case Expression::Kind::kSetting:
case Expression::Kind::kTypeReference:
case Expression::Kind::kVariableReference:
// Nothing to scan here.
break;
case Expression::Kind::kBinary: {
BinaryExpression& binaryExpr = (*expr)->as<BinaryExpression>();
this->visitExpression(&binaryExpr.fLeft);
// Logical-and and logical-or binary expressions do not inline the right side,
// because that would invalidate short-circuiting. That is, when evaluating
// expressions like these:
// (false && x()) // always false
// (true || y()) // always true
// It is illegal for side-effects from x() or y() to occur. The simplest way to
// enforce that rule is to avoid inlining the right side entirely. However, it
// is safe for other types of binary expression to inline both sides.
bool shortCircuitable = (binaryExpr.fOperator == Token::Kind::TK_LOGICALAND ||
binaryExpr.fOperator == Token::Kind::TK_LOGICALOR);
if (!shortCircuitable) {
this->visitExpression(&binaryExpr.fRight);
}
break;
}
case Expression::Kind::kConstructor: {
Constructor& constructorExpr = (*expr)->as<Constructor>();
for (std::unique_ptr<Expression>& arg : constructorExpr.fArguments) {
this->visitExpression(&arg);
}
break;
}
case Expression::Kind::kExternalFunctionCall: {
ExternalFunctionCall& funcCallExpr = (*expr)->as<ExternalFunctionCall>();
for (std::unique_ptr<Expression>& arg : funcCallExpr.fArguments) {
this->visitExpression(&arg);
}
break;
}
case Expression::Kind::kFunctionCall: {
FunctionCall& funcCallExpr = (*expr)->as<FunctionCall>();
for (std::unique_ptr<Expression>& arg : funcCallExpr.fArguments) {
this->visitExpression(&arg);
}
this->addInlineCandidate(expr);
break;
}
case Expression::Kind::kIndex:{
IndexExpression& indexExpr = (*expr)->as<IndexExpression>();
this->visitExpression(&indexExpr.fBase);
this->visitExpression(&indexExpr.fIndex);
break;
}
case Expression::Kind::kPostfix: {
PostfixExpression& postfixExpr = (*expr)->as<PostfixExpression>();
this->visitExpression(&postfixExpr.fOperand);
break;
}
case Expression::Kind::kPrefix: {
PrefixExpression& prefixExpr = (*expr)->as<PrefixExpression>();
this->visitExpression(&prefixExpr.fOperand);
break;
}
case Expression::Kind::kSwizzle: {
Swizzle& swizzleExpr = (*expr)->as<Swizzle>();
this->visitExpression(&swizzleExpr.fBase);
break;
}
case Expression::Kind::kTernary: {
TernaryExpression& ternaryExpr = (*expr)->as<TernaryExpression>();
// The test expression is a candidate for inlining.
this->visitExpression(&ternaryExpr.fTest);
// The true- and false-expressions cannot be inlined, because we are only
// allowed to evaluate one side.
break;
}
default:
SkUNREACHABLE;
}
}
void addInlineCandidate(std::unique_ptr<Expression>* candidate) {
fInlineCandidates.push_back(InlineCandidate{fSymbolTableStack.back(),
fEnclosingStmtStack.back(), candidate});
}
};
// TODO(johnstiles): the analyzer can detect inlinable functions; actually inlining them will
// be tackled in a followup CL.
InlineCandidateAnalyzer analyzer;
analyzer.visit(program);
std::unordered_map<const FunctionDeclaration*, bool> inlinableMap; // <function, safe-to-inline>
for (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);
/*
if (inlinableMap[funcDecl]) {
printf("-> Inliner discovered valid candidate: %s\n",
String(funcDecl->fName).c_str());
}
*/
}
}
return false;
}
} // namespace SkSL

6
src/sksl/SkSLInliner.h
View File

@ -49,12 +49,6 @@ public:
/** Checks whether inlining is viable for a FunctionCall. */
bool isSafeToInline(const FunctionCall&, int inlineThreshold);
/**
* Analyzes a program to find candidate functions for inlining. Returns true if changes were
* made.
*/
bool analyze(Program& program);
private:
using VariableRewriteMap = std::unordered_map<const Variable*, const Variable*>;