From 6eadf137835b929a2e77c2beb7c9f159f6807ec3 Mon Sep 17 00:00:00 2001 From: John Stiles Date: Tue, 8 Sep 2020 10:16:10 -0400 Subject: [PATCH] Change Inliner signature in preparation for analyzer pass. These changes don't seem to add much value in isolation, but they will smooth our transition to inline analysis during optimization. - The passed-in FunctionCall no longer needs to be a unique_ptr. - The fInlinedBody is guaranteed to be a Block now. (This change caused a slight ripple effect in unit test output; in some cases it creates an additional newline in the final code. This is harmless.) - has_early_return is checked earlier, before we've made any mutations to the FunctionCall. This should work around errors that can occur if a function is trying to inline itself. (Ideally we wouldn't be trying to do this at all, but either way, we shouldn't crash.) Change-Id: I0a565d477f680c0ba061df2686a36e42aa75de95 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/315599 Commit-Queue: John Stiles Commit-Queue: Ethan Nicholas Auto-Submit: John Stiles Reviewed-by: Ethan Nicholas --- src/sksl/SkSLIRGenerator.cpp | 3 +- src/sksl/SkSLInliner.cpp | 34 ++++++---------- src/sksl/SkSLInliner.h | 5 ++- tests/SkSLGLSLTest.cpp | 41 ++++++++++--------- tests/SkSLInlinerTest.cpp | 79 +++++++++++++++++++++++++----------- 5 files changed, 94 insertions(+), 68 deletions(-) diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp index 8bc6fe044e..73cf54e0f2 100644 --- a/src/sksl/SkSLIRGenerator.cpp +++ b/src/sksl/SkSLIRGenerator.cpp @@ -2068,8 +2068,7 @@ std::unique_ptr IRGenerator::call(int offset, auto funcCall = std::make_unique(offset, *returnType, function, std::move(arguments)); if (fCanInline && fInliner->isSafeToInline(*funcCall, fSettings->fInlineThreshold)) { - Inliner::InlinedCall inlinedCall = fInliner->inlineCall(std::move(funcCall), - fSymbolTable.get()); + Inliner::InlinedCall inlinedCall = fInliner->inlineCall(funcCall.get(), fSymbolTable.get()); if (inlinedCall.fInlinedBody) { fExtraStatements.push_back(std::move(inlinedCall.fInlinedBody)); } diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp index f3323aa76e..f3ac718836 100644 --- a/src/sksl/SkSLInliner.cpp +++ b/src/sksl/SkSLInliner.cpp @@ -416,7 +416,7 @@ std::unique_ptr Inliner::inlineStatement(int offset, } } -Inliner::InlinedCall Inliner::inlineCall(std::unique_ptr call, +Inliner::InlinedCall Inliner::inlineCall(FunctionCall* call, SymbolTable* symbolTableForCall) { // Inlining is more complicated here than in a typical compiler, because we have to have a // high-level IR and can't just drop statements into the middle of an expression or even use @@ -432,11 +432,17 @@ Inliner::InlinedCall Inliner::inlineCall(std::unique_ptr call, SkASSERT(call); SkASSERT(this->isSafeToInline(*call, /*inlineThreshold=*/INT_MAX)); - int offset = call->fOffset; std::vector>& arguments = call->fArguments; + const int offset = call->fOffset; const FunctionDefinition& function = *call->fFunction.fDefinition; + const bool hasEarlyReturn = has_early_return(function); + InlinedCall inlinedCall; - std::vector> inlinedBody; + inlinedCall.fInlinedBody = std::make_unique(offset, + std::vector>{}, + /*symbols=*/nullptr, + /*isScope=*/false); + std::vector>& inlinedBody = inlinedCall.fInlinedBody->fStatements; auto makeInlineVar = [&](const String& baseName, const Type& type, Modifiers modifiers, std::unique_ptr* initialValue) -> const Variable* { @@ -517,7 +523,6 @@ Inliner::InlinedCall Inliner::inlineCall(std::unique_ptr call, } const Block& body = function.fBody->as(); - bool hasEarlyReturn = has_early_return(function); auto inlineBlock = std::make_unique(offset, std::vector>{}); inlineBlock->fStatements.reserve(body.fStatements.size()); for (const std::unique_ptr& stmt : body.fStatements) { @@ -534,8 +539,8 @@ Inliner::InlinedCall Inliner::inlineCall(std::unique_ptr call, std::move(inlineBlock), std::make_unique(*fContext, offset, /*value=*/false))); } else { - // No early returns, so we can just dump the code in. We need to use a block so we don't get - // name conflicts with locals. + // 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.push_back(std::move(inlineBlock)); } @@ -546,8 +551,8 @@ Inliner::InlinedCall Inliner::inlineCall(std::unique_ptr call, SkASSERT(varMap.find(p) != varMap.end()); if (arguments[i]->fKind == Expression::kVariableReference_Kind && &arguments[i]->as().fVariable == varMap[p]) { - // we didn't create a temporary for this parameter, so there's nothing to copy back - // out + // We didn't create a temporary for this parameter, so there's nothing to copy back + // out. continue; } auto varRef = std::make_unique(offset, *varMap[p]); @@ -570,19 +575,6 @@ Inliner::InlinedCall Inliner::inlineCall(std::unique_ptr call, /*value=*/false); } - switch (inlinedBody.size()) { - case 0: - break; - case 1: - inlinedCall.fInlinedBody = std::move(inlinedBody.front()); - break; - default: - inlinedCall.fInlinedBody = std::make_unique(offset, std::move(inlinedBody), - /*symbols=*/nullptr, - /*isScope=*/false); - break; - } - return inlinedCall; } diff --git a/src/sksl/SkSLInliner.h b/src/sksl/SkSLInliner.h index 03fd835bdf..73467163a0 100644 --- a/src/sksl/SkSLInliner.h +++ b/src/sksl/SkSLInliner.h @@ -15,6 +15,7 @@ namespace SkSL { +struct Block; class Context; struct Expression; struct FunctionCall; @@ -40,10 +41,10 @@ public: * above the statement containing the inlined expression. */ struct InlinedCall { - std::unique_ptr fInlinedBody; + std::unique_ptr fInlinedBody; std::unique_ptr fReplacementExpr; }; - InlinedCall inlineCall(std::unique_ptr, SymbolTable*); + InlinedCall inlineCall(FunctionCall*, SymbolTable*); /** Checks whether inlining is viable for a FunctionCall. */ bool isSafeToInline(const FunctionCall&, int inlineThreshold); diff --git a/tests/SkSLGLSLTest.cpp b/tests/SkSLGLSLTest.cpp index 16724fbe19..404fb77ca0 100644 --- a/tests/SkSLGLSLTest.cpp +++ b/tests/SkSLGLSLTest.cpp @@ -120,6 +120,7 @@ void main() { x = z; } + sk_FragColor = vec4(x); } )__GLSL__"); @@ -1857,25 +1858,27 @@ DEF_TEST(SkSLGeometryShaders, r) { "EmitVertex();" "}", *SkSL::ShaderCapsFactory::NoGSInvocationsSupport(), - "#version 400\n" - "int sk_InvocationID;\n" - "layout (points) in ;\n" - "layout (line_strip, max_vertices = 4) out ;\n" - "void _invoke() {\n" - " {\n" - " gl_Position = gl_in[0].gl_Position + vec4(0.5, 0.0, 0.0, float(sk_InvocationID));\n" - " EmitVertex();\n" - " }\n" - "\n" - " gl_Position = gl_in[0].gl_Position + vec4(-0.5, 0.0, 0.0, float(sk_InvocationID));\n" - " EmitVertex();\n" - "}\n" - "void main() {\n" - " for (sk_InvocationID = 0;sk_InvocationID < 2; sk_InvocationID++) {\n" - " _invoke();\n" - " EndPrimitive();\n" - " }\n" - "}\n", + R"__GLSL__(#version 400 +int sk_InvocationID; +layout (points) in ; +layout (line_strip, max_vertices = 4) out ; +void _invoke() { + { + gl_Position = gl_in[0].gl_Position + vec4(0.5, 0.0, 0.0, float(sk_InvocationID)); + EmitVertex(); + } + + + gl_Position = gl_in[0].gl_Position + vec4(-0.5, 0.0, 0.0, float(sk_InvocationID)); + EmitVertex(); +} +void main() { + for (sk_InvocationID = 0;sk_InvocationID < 2; sk_InvocationID++) { + _invoke(); + EndPrimitive(); + } +} +)__GLSL__", SkSL::Program::kGeometry_Kind); test(r, "layout(points, invocations = 2) in;" diff --git a/tests/SkSLInlinerTest.cpp b/tests/SkSLInlinerTest.cpp index 4bb7be02f1..165c3fdc73 100644 --- a/tests/SkSLInlinerTest.cpp +++ b/tests/SkSLInlinerTest.cpp @@ -81,20 +81,49 @@ DEF_TEST(SkSLFunctionInlineKeywordOverridesThreshold, r) { " ++x; ++x; ++x; ++x; ++x; ++x; ++x; ++x; ++x; ++x; ++x; ++x; ++x; ++x; ++x; ++x; ++x;" "}" "void main() { int y = 0; tooBig(y); }", - "#version 400\n" - "void main() {\n" - " int y = 0;\n" - " {\n" - " ++y;\n ++y;\n ++y;\n ++y;\n ++y;\n ++y;\n" - " ++y;\n ++y;\n ++y;\n ++y;\n ++y;\n ++y;\n" - " ++y;\n ++y;\n ++y;\n ++y;\n ++y;\n ++y;\n" - " ++y;\n ++y;\n ++y;\n ++y;\n ++y;\n ++y;\n" - " ++y;\n ++y;\n ++y;\n ++y;\n ++y;\n ++y;\n" - " ++y;\n ++y;\n ++y;\n ++y;\n" - " }\n" - "\n" - "}\n" - ); + R"__GLSL__(#version 400 +void main() { + int y = 0; + { + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + ++y; + } + + +} +)__GLSL__"); } DEF_TEST(SkSLFunctionUnableToInlineReturnsInsideLoop, r) { @@ -178,16 +207,18 @@ DEF_TEST(SkSLFunctionInlineWithInoutArgument, r) { " outParameter(x);" " sk_FragColor.x = x;" "}", - "#version 400\n" - "out vec4 sk_FragColor;\n" - "void main() {\n" - " float x = 1.0;\n" - " {\n" - " x *= 2.0;\n" - " }\n" - "\n" - " sk_FragColor.x = x;\n" - "}\n"); + R"__GLSL__(#version 400 +out vec4 sk_FragColor; +void main() { + float x = 1.0; + { + x *= 2.0; + } + + + sk_FragColor.x = x; +} +)__GLSL__"); } DEF_TEST(SkSLFunctionInlineWithNestedCall, r) {