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

Improve control-flow support in early-return detection.

Browse Source 
Previously, early-return detection worked by ensuring that a function
had a maximum of one return statement, positioned at the end of the
function. However, this technique did not support if-else statements:

   if (a) { ...; return x; } else { ...; return y; }

This meant that many simple cases were unnecessarily wrapped in a do-
while loop to handle control flow issues that didn't actually occur.

Our early-return detection logic is now more flexible and looks for any
return statements that aren't at an exit point in the control flow,
instead of looking for exactly one return at the end of the code.

Change-Id: Iffe71adf2b9349ce8de42ba8301ccc52abe2882b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/313418
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This commit is contained in:
John Stiles 2020-08-27 10:58:36 -04:00 committed by Skia Commit-Bot
parent be0e42cb8f
commit 318ad4bb19
2 changed files with 102 additions and 87 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

108
src/sksl/SkSLIRGenerator.cpp
View File

@ -2170,48 +2170,65 @@ std::unique_ptr<Statement> IRGenerator::inlineStatement(
}
}
template <bool countTopLevelReturns>
template <bool countTopLevelReturns, bool onlyAtEndOfControlFlow>
static int return_count(const Statement& statement, bool inLoopOrSwitch) {
constexpr auto& recurse = return_count<countTopLevelReturns, onlyAtEndOfControlFlow>;
switch (statement.fKind) {
case Statement::kBlock_Kind: {
const Block& b = statement.as<Block>();
const std::vector<std::unique_ptr<Statement>>& statements = b.fStatements;
if (onlyAtEndOfControlFlow) {
return (!inLoopOrSwitch && !statements.empty())
? recurse(*statements.back(), inLoopOrSwitch)
: 0;
}
int result = 0;
for (const std::unique_ptr<Statement>& s : b.fStatements) {
result += return_count<countTopLevelReturns>(*s, inLoopOrSwitch);
for (const std::unique_ptr<Statement>& s : statements) {
result += recurse(*s, inLoopOrSwitch);
}
return result;
}
case Statement::kDo_Kind: {
const DoStatement& d = statement.as<DoStatement>();
return return_count<countTopLevelReturns>(*d.fStatement, /*inLoopOrSwitch=*/true);
return recurse(*d.fStatement, /*inLoopOrSwitch=*/true);
}
case Statement::kFor_Kind: {
const ForStatement& f = statement.as<ForStatement>();
return return_count<countTopLevelReturns>(*f.fStatement, /*inLoopOrSwitch=*/true);
return recurse(*f.fStatement, /*inLoopOrSwitch=*/true);
}
case Statement::kIf_Kind: {
const IfStatement& i = statement.as<IfStatement>();
int result = return_count<countTopLevelReturns>(*i.fIfTrue, inLoopOrSwitch);
int result = recurse(*i.fIfTrue, inLoopOrSwitch);
if (i.fIfFalse) {
result += return_count<countTopLevelReturns>(*i.fIfFalse, inLoopOrSwitch);
result += recurse(*i.fIfFalse, inLoopOrSwitch);
}
return result;
}
case Statement::kReturn_Kind:
return (countTopLevelReturns || inLoopOrSwitch) ? 1 : 0;
case Statement::kSwitch_Kind: {
const SwitchStatement& ss = statement.as<SwitchStatement>();
int result = 0;
for (const std::unique_ptr<SwitchCase>& sc : ss.fCases) {
for (const std::unique_ptr<Statement>& s : sc->fStatements) {
result += return_count<countTopLevelReturns>(*s, /*inLoopOrSwitch=*/true);
if (!onlyAtEndOfControlFlow) {
// Fallthrough is not considered, so this is considered to have one return even
// though the `return` statement applies to both the `case 0` and `case 1` blocks.
// switch (x) { case 0: case 1: return; }
const SwitchStatement& switchStmt = statement.as<SwitchStatement>();
for (const std::unique_ptr<SwitchCase>& switchCase : switchStmt.fCases) {
for (const std::unique_ptr<Statement>& caseStmt : switchCase->fStatements) {
result += recurse(*caseStmt, /*inLoopOrSwitch=*/true);
}
}
}
return result;
}
case Statement::kWhile_Kind: {
const WhileStatement& w = statement.as<WhileStatement>();
return return_count<countTopLevelReturns>(*w.fStatement, /*inLoopOrSwitch=*/true);
return recurse(*w.fStatement, /*inLoopOrSwitch=*/true);
}
case Statement::kBreak_Kind:
case Statement::kContinue_Kind:
@ -2227,45 +2244,29 @@ static int return_count(const Statement& statement, bool inLoopOrSwitch) {
}
}
static bool has_early_return(const FunctionDefinition& f) {
int returnCount =
return_count</*countTopLevelReturns=*/true>(*f.fBody, /*inLoopOrSwitch=*/false);
static int count_all_returns(const FunctionDefinition& funcDef) {
return return_count</*countTopLevelReturns=*/true,
/*onlyAtEndOfControlFlow=*/false>(*funcDef.fBody, /*inLoopOrSwitch=*/false);
}
static int count_returns_at_end_of_control_flow(const FunctionDefinition& funcDef) {
return return_count</*countTopLevelReturns=*/true,
/*onlyAtEndOfControlFlow=*/true>(*funcDef.fBody, /*inLoopOrSwitch=*/false);
}
static int count_returns_in_breakable_constructs(const FunctionDefinition& funcDef) {
return return_count</*countTopLevelReturns=*/false,
/*onlyAtEndOfControlFlow=*/false>(*funcDef.fBody, /*inLoopOrSwitch=*/false);
}
static bool has_early_return(const FunctionDefinition& funcDef) {
int returnCount = count_all_returns(funcDef);
if (returnCount == 0) {
return false;
}
if (returnCount > 1) {
return true;
}
// Descend into the last statement of the block to see if it ends with a return statement.
// Sometimes the last statement of the function is actually another block, so we may need to
// descend more than once.
const Block* block = &f.fBody->as<Block>();
for (;;) {
if (block->fStatements.empty()) {
// The function ended with a totally empty block, but we know there's a return statement
// earlier in the function, so there must be an early return.
return true;
}
const Statement& lastStatement = *block->fStatements.back();
if (lastStatement.fKind == Statement::kReturn_Kind) {
// The last statement is a return; it's not early.
return false;
}
if (lastStatement.fKind != Statement::kBlock_Kind) {
// The last statement is not a sub-block but also not a return. We know there's a return
// statement earlier in the function, which must be an early return.
return true;
}
// The last statement is itself another block. We have to go deeper.
block = &lastStatement.as<Block>();
}
}
static bool has_return_in_breakable_construct(const FunctionDefinition& f) {
int returnCount =
return_count</*countTopLevelReturns=*/false>(*f.fBody, /*inLoopOrSwitch=*/false);
return returnCount > 0;
int returnsAtEndOfControlFlow = count_returns_at_end_of_control_flow(funcDef);
return returnCount > returnsAtEndOfControlFlow;
}
std::unique_ptr<Expression> IRGenerator::inlineCall(
@ -2421,11 +2422,20 @@ bool IRGenerator::isSafeToInline(const FunctionDefinition& functionDef) {
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.
return !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);
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.
return !has_return_in_breakable_construct(functionDef);
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));
return !hasReturnInBreakableConstruct;
}
std::unique_ptr<Expression> IRGenerator::call(int offset,

81
tests/SkSLFPTest.cpp
View File

@ -1056,6 +1056,38 @@ R"SkSL(half4 _inlineResulthalf4fliphalf40;
)__Cpp__"});
}
DEF_TEST(SkSLFPSwitchWithMultipleReturnsInside, r) {
test(r,
*SkSL::ShaderCapsFactory::Default(),
R"__SkSL__(
uniform half4 color;
half4 switchy(half4 c) {
switch (int(c.x)) {
case 0: return c.yyyy;
default: return c.zzzz;
}
}
void main() {
sk_OutColor = switchy(color);
}
)__SkSL__",
/*expectedH=*/{},
/*expectedCPP=*/{
R"__Cpp__(fragBuilder->emitFunction(kHalf4_GrSLType, "switchy", 1, switchy_args,
R"SkSL(switch (int(c.x)) {
case 0:
return c.yyyy;
default:
return c.zzzz;
}
)SkSL", &switchy_name);
fragBuilder->codeAppendf(
R"SkSL(%s = %s(%s);
)SkSL"
, args.fOutputColor, switchy_name.c_str(), args.fUniformHandler->getUniformCStr(colorVar));
)__Cpp__"});
}
DEF_TEST(SkSLFPSwitchWithReturnInsideCannotBeInlined, r) {
test(r,
*SkSL::ShaderCapsFactory::Default(),
@ -1249,15 +1281,9 @@ DEF_TEST(SkSLFPIfStatementWithReturnInsideCanBeInlined, r) {
/*expectedCPP=*/{
R"__Cpp__(fragBuilder->codeAppendf(
R"SkSL(half4 _inlineResulthalf4branchyhalf40;
do {
if (%s.z == %s.w) {
_inlineResulthalf4branchyhalf40 = %s.yyyy;
break;
} else {
_inlineResulthalf4branchyhalf40 = %s.zzzz;
break;
}
} while (false);
{
if (%s.z == %s.w) _inlineResulthalf4branchyhalf40 = %s.yyyy; else _inlineResulthalf4branchyhalf40 = %s.zzzz;
}
%s = _inlineResulthalf4branchyhalf40;
)SkSL"
@ -1374,52 +1400,31 @@ DEF_TEST(SkSLFPEarlyReturnDetectionSupportsIfElse, r) {
R"__Cpp__(fragBuilder->codeAppendf(
R"SkSL(half4 _inlineResulthalf4branchyhalf40;
half4 _inlineArghalf4branchyhalf41_0 = %s;
do {
{
_inlineArghalf4branchyhalf41_0 *= 0.5;
if (_inlineArghalf4branchyhalf41_0.x > 0.0) {
_inlineResulthalf4branchyhalf40 = _inlineArghalf4branchyhalf41_0.xxxx;
break;
} else if (_inlineArghalf4branchyhalf41_0.y > 0.0) {
_inlineResulthalf4branchyhalf40 = _inlineArghalf4branchyhalf41_0.yyyy;
break;
} else if (_inlineArghalf4branchyhalf41_0.z > 0.0) {
_inlineResulthalf4branchyhalf40 = _inlineArghalf4branchyhalf41_0.zzzz;
break;
} else {
_inlineResulthalf4branchyhalf40 = _inlineArghalf4branchyhalf41_0.wwww;
break;
}
} while (false);
if (_inlineArghalf4branchyhalf41_0.x > 0.0) _inlineResulthalf4branchyhalf40 = _inlineArghalf4branchyhalf41_0.xxxx; else if (_inlineArghalf4branchyhalf41_0.y > 0.0) _inlineResulthalf4branchyhalf40 = _inlineArghalf4branchyhalf41_0.yyyy; else if (_inlineArghalf4branchyhalf41_0.z > 0.0) _inlineResulthalf4branchyhalf40 = _inlineArghalf4branchyhalf41_0.zzzz; else _inlineResulthalf4branchyhalf40 = _inlineArghalf4branchyhalf41_0.wwww;
}
half4 _inlineResulthalf4branchyAndBlockyhalf42;
do {
{
{
{
if (%s.x > 0.0) {
half4 d = %s * 0.5;
{
_inlineResulthalf4branchyAndBlockyhalf42 = d.xxxx;
break;
}
_inlineResulthalf4branchyAndBlockyhalf42 = d.xxxx;
} else {
{
{
if (%s.x < 0.0) {
{
_inlineResulthalf4branchyAndBlockyhalf42 = %s.wwww;
break;
}
_inlineResulthalf4branchyAndBlockyhalf42 = %s.wwww;
} else {
{
_inlineResulthalf4branchyAndBlockyhalf42 = %s.yyyy;
break;
}
_inlineResulthalf4branchyAndBlockyhalf42 = %s.yyyy;
}
}
}
}
}
}
} while (false);
}
%s = _inlineResulthalf4branchyhalf40 * _inlineResulthalf4branchyAndBlockyhalf42;
)SkSL"