From c75abb84327cb4700d933a995f3550f36fe1699d Mon Sep 17 00:00:00 2001 From: John Stiles Date: Mon, 14 Sep 2020 18:24:12 -0400 Subject: [PATCH] Assign unique names to inlined variable declarations. Change-Id: I6097f50e7be1fa2f7772f6c454410ecbf3470eea Bug: skia:10722 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/316977 Commit-Queue: John Stiles Auto-Submit: John Stiles Reviewed-by: Brian Osman --- src/sksl/SkSLInliner.cpp | 51 ++++--- src/sksl/SkSLInliner.h | 2 + tests/SkSLInlinerTest.cpp | 130 +++++++++--------- tests/sksl/glsl/golden/Functions.glsl | 14 +- .../InlinerAvoidsVariableNameOverlap.glsl | 14 +- 5 files changed, 110 insertions(+), 101 deletions(-) diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp index c59d805373..5ede14d44e 100644 --- a/src/sksl/SkSLInliner.cpp +++ b/src/sksl/SkSLInliner.cpp @@ -263,6 +263,28 @@ void Inliner::reset(const Context& context, const Program::Settings& settings) { fInlineVarCounter = 0; } +String Inliner::uniqueNameForInlineVar(const String& baseName, SymbolTable* symbolTable) { + // If the base name starts with an underscore, like "_coords", we can't append another + // underscore, because OpenGL disallows two consecutive underscores anywhere in the string. But + // in the general case, using the underscore as a splitter reads nicely enough that it's worth + // putting in this special case. + const char* splitter = baseName.startsWith("_") ? "" : "_"; + + // Append a unique numeric prefix to avoid name overlap. Check the symbol table to make sure + // we're not reusing an existing name. (Note that within a single compilation pass, this check + // isn't fully comprehensive, as code isn't always generated in top-to-bottom order.) + String uniqueName; + for (;;) { + uniqueName = String::printf("_%d%s%s", fInlineVarCounter++, splitter, baseName.c_str()); + StringFragment frag{uniqueName.data(), uniqueName.length()}; + if ((*symbolTable)[frag] == nullptr) { + break; + } + } + + return uniqueName; +} + std::unique_ptr Inliner::inlineExpression(int offset, VariableRewriteMap* varMap, const Expression& expression) { @@ -466,9 +488,11 @@ std::unique_ptr Inliner::inlineStatement(int offset, } std::unique_ptr initialValue = expr(decl.fValue); const Variable* old = decl.fVar; - // need to copy the var name in case the originating function is discarded and we lose - // its symbols - std::unique_ptr name(new String(old->fName)); + // We assign unique names to inlined variables--scopes hide most of the problems in this + // regard, but see `InlinerAvoidsVariableNameOverlap` for a counterexample where unique + // names are important. + auto name = std::make_unique( + this->uniqueNameForInlineVar(String(old->fName), symbolTableForStatement)); const String* namePtr = symbolTableForStatement->takeOwnershipOfString(std::move(name)); const Type* typePtr = copy_if_needed(&old->type(), *symbolTableForStatement); const Variable* clone = symbolTableForStatement->takeOwnershipOfSymbol( @@ -551,25 +575,8 @@ Inliner::InlinedCall Inliner::inlineCall(FunctionCall* call, type = fContext->fInt_Type.get(); } - // If the base name starts with an underscore, like "_coords", we can't append another - // underscore, because some OpenGL platforms error out when they see two consecutive - // underscores (anywhere in the string!). But in the general case, using the underscore as - // a splitter reads nicely enough that it's worth putting in this special case. - const char* splitter = baseName.startsWith("_") ? "_X" : "_"; - - // Append a unique numeric prefix to avoid name overlap. Check the symbol table to make sure - // we're not reusing an existing name. (Note that within a single compilation pass, this - // check isn't fully comprehensive, as code isn't always generated in top-to-bottom order.) - String uniqueName; - for (;;) { - uniqueName = String::printf("_%d%s%s", fInlineVarCounter++, splitter, baseName.c_str()); - StringFragment frag{uniqueName.data(), uniqueName.length()}; - if ((*symbolTableForCall)[frag] == nullptr) { - break; - } - } - - // Add our new variable's name to the symbol table. + // Provide our new variable with a unique name, and add it to our symbol table. + String uniqueName = this->uniqueNameForInlineVar(baseName, symbolTableForCall); const String* namePtr = symbolTableForCall->takeOwnershipOfString( std::make_unique(std::move(uniqueName))); StringFragment nameFrag{namePtr->c_str(), namePtr->length()}; diff --git a/src/sksl/SkSLInliner.h b/src/sksl/SkSLInliner.h index f8dad30dde..9c1e043015 100644 --- a/src/sksl/SkSLInliner.h +++ b/src/sksl/SkSLInliner.h @@ -55,6 +55,8 @@ public: private: using VariableRewriteMap = std::unordered_map; + String uniqueNameForInlineVar(const String& baseName, SymbolTable* symbolTable); + std::unique_ptr inlineExpression(int offset, VariableRewriteMap* varMap, const Expression& expression); diff --git a/tests/SkSLInlinerTest.cpp b/tests/SkSLInlinerTest.cpp index e901cd1ab9..be12d7b069 100644 --- a/tests/SkSLInlinerTest.cpp +++ b/tests/SkSLInlinerTest.cpp @@ -725,13 +725,13 @@ uniform vec4 color; void main() { vec4 _0_switchy; { - vec4 result; + vec4 _1_result; switch (int(color.x)) { case 0: - result = color.yyyy; + _1_result = color.yyyy; } - result = color.zzzz; - _0_switchy = result; + _1_result = color.zzzz; + _0_switchy = _1_result; } sk_FragColor = _0_switchy; @@ -793,16 +793,16 @@ uniform vec4 color; void main() { vec4 _0_switchy; { - vec4 result; + vec4 _1_result; switch (int(color.x)) { case 1: - result = color.yyyy; + _1_result = color.yyyy; break; default: - result = color.zzzz; + _1_result = color.zzzz; break; } - _0_switchy = result; + _0_switchy = _1_result; } sk_FragColor = _0_switchy; @@ -834,12 +834,12 @@ uniform vec4 color; void main() { vec4 _0_loopy; { - vec4 pix; - for (int x = 0;x < 5; ++x) { - if (x == int(color.w)) pix = color.yyyy; + vec4 _1_pix; + for (int _2_x = 0;_2_x < 5; ++_2_x) { + if (_2_x == int(color.w)) _1_pix = color.yyyy; } - pix = color.zzzz; - _0_loopy = pix; + _1_pix = color.zzzz; + _0_loopy = _1_pix; } sk_FragColor = _0_loopy; @@ -873,88 +873,88 @@ DEF_TEST(SkSLFPInlinerManglesOverlappingNames, r) { /*expectedGLSL=*/R"__GLSL__(#version 400 uniform vec4 color; vec4 main() { - float _2_fma; - float _3_a = color.x; - float _4_b = color.y; - float _5_c = color.z; + float _3_fma; + float _4_a = color.x; + float _5_b = color.y; + float _6_c = color.z; { - float _0_mul; + float _7_0_mul; { - _0_mul = _3_a * _4_b; + _7_0_mul = _4_a * _5_b; } - float _1_add; + float _8_1_add; { - float c = _0_mul + _5_c; - _1_add = c; + float _9_2_c = _7_0_mul + _6_c; + _8_1_add = _9_2_c; } - _2_fma = _1_add; + _3_fma = _8_1_add; } - float a = _2_fma; - - float _6_fma; - float _7_a = color.y; - float _8_b = color.z; - float _9_c = color.w; - { - float _0_mul; - { - _0_mul = _7_a * _8_b; - } - - float _1_add; - { - float c = _0_mul + _9_c; - _1_add = c; - } - - _6_fma = _1_add; - - } - - float b = _6_fma; + float a = _3_fma; float _10_fma; - float _11_a = color.z; - float _12_b = color.w; - float _13_c = color.x; + float _11_a = color.y; + float _12_b = color.z; + float _13_c = color.w; { - float _0_mul; + float _14_0_mul; { - _0_mul = _11_a * _12_b; + _14_0_mul = _11_a * _12_b; } - float _1_add; + float _15_1_add; { - float c = _0_mul + _13_c; - _1_add = c; + float _16_2_c = _14_0_mul + _13_c; + _15_1_add = _16_2_c; } - _10_fma = _1_add; + _10_fma = _15_1_add; } - float c = _10_fma; + float b = _10_fma; - float _14_mul; + float _17_fma; + float _18_a = color.z; + float _19_b = color.w; + float _20_c = color.x; { - _14_mul = c * c; + float _21_0_mul; + { + _21_0_mul = _18_a * _19_b; + } + + float _22_1_add; + { + float _23_2_c = _21_0_mul + _20_c; + _22_1_add = _23_2_c; + } + + _17_fma = _22_1_add; + } - float _15_mul; + float c = _17_fma; + + float _24_mul; { - _15_mul = b * c; + _24_mul = c * c; } - float _16_mul; + float _25_mul; { - _16_mul = a * _15_mul; + _25_mul = b * c; } - return vec4(a, b, _14_mul, _16_mul); + float _26_mul; + { + _26_mul = a * _25_mul; + } + + return vec4(a, b, _24_mul, _26_mul); } )__GLSL__"); @@ -1108,8 +1108,8 @@ void main() { { { if (color.x > 0.0) { - vec4 d = color * 0.5; - _2_branchyAndBlocky = d.xxxx; + vec4 _3_d = color * 0.5; + _2_branchyAndBlocky = _3_d.xxxx; } else { { { diff --git a/tests/sksl/glsl/golden/Functions.glsl b/tests/sksl/glsl/golden/Functions.glsl index 77919a32a7..789e8c14c5 100644 --- a/tests/sksl/glsl/golden/Functions.glsl +++ b/tests/sksl/glsl/golden/Functions.glsl @@ -5,17 +5,17 @@ out mediump vec4 sk_FragColor; void main() { highp float x = 10.0; { - highp float y[2], z; - y[0] = 10.0; - y[1] = 20.0; - highp float _0_foo; + highp float _1_y[2], _2_z; + _1_y[0] = 10.0; + _1_y[1] = 20.0; + highp float _3_0_foo; { - _0_foo = y[0] * y[1]; + _3_0_foo = _1_y[0] * _1_y[1]; } - z = _0_foo; + _2_z = _3_0_foo; - x = z; + x = _2_z; } diff --git a/tests/sksl/inliner/golden/InlinerAvoidsVariableNameOverlap.glsl b/tests/sksl/inliner/golden/InlinerAvoidsVariableNameOverlap.glsl index 544b8fa834..5c69301c3a 100644 --- a/tests/sksl/inliner/golden/InlinerAvoidsVariableNameOverlap.glsl +++ b/tests/sksl/inliner/golden/InlinerAvoidsVariableNameOverlap.glsl @@ -3,19 +3,19 @@ precision mediump float; precision mediump sampler2D; in mediump vec2 x; mediump vec4 main() { - mediump vec2 _1_InlineA; + mediump vec2 _2_InlineA; { - mediump vec2 reusedName = x + vec2(1.0, 2.0); - mediump vec2 _0_InlineB; + mediump vec2 _3_reusedName = x + vec2(1.0, 2.0); + mediump vec2 _4_0_InlineB; { - mediump vec2 reusedName = reusedName + vec2(3.0, 4.0); - _0_InlineB = reusedName; + mediump vec2 _5_1_reusedName = _3_reusedName + vec2(3.0, 4.0); + _4_0_InlineB = _5_1_reusedName; } - _1_InlineA = _0_InlineB; + _2_InlineA = _4_0_InlineB; } - return _1_InlineA.xyxy; + return _2_InlineA.xyxy; }