From b5f0f52831fcf7cf26b053128fc866db06acf46f Mon Sep 17 00:00:00 2001 From: Brian Osman Date: Thu, 23 Jul 2020 13:28:14 -0400 Subject: [PATCH] SkSL: Salt inline identifiers in PipelineStage and FragmentProcessor passes Fixes a bug with name conflicts in the final SkSL. Bug: skia:10526 Change-Id: Ic238f89dd778c186e775ecbaabfbaed9e426274f Reviewed-on: https://skia-review.googlesource.com/c/skia/+/305563 Commit-Queue: Brian Osman Reviewed-by: Michael Ludwig --- .../generated/GrColorMatrixFragmentProcessor.cpp | 8 ++++---- .../generated/GrHighContrastFilterEffect.cpp | 8 ++++---- src/sksl/SkSLIRGenerator.cpp | 16 ++++++++++++++-- tests/SkRuntimeEffectTest.cpp | 14 ++++++++++++++ tests/SkSLFPTest.cpp | 8 ++++---- 5 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/gpu/effects/generated/GrColorMatrixFragmentProcessor.cpp b/src/gpu/effects/generated/GrColorMatrixFragmentProcessor.cpp index 5278c72537..e7d3c35c1d 100644 --- a/src/gpu/effects/generated/GrColorMatrixFragmentProcessor.cpp +++ b/src/gpu/effects/generated/GrColorMatrixFragmentProcessor.cpp @@ -42,12 +42,12 @@ public: fragBuilder->codeAppendf( R"SkSL(half4 inputColor = %s; @if (%s) { - half4 inlineResult0; - half4 inlineArg1_0 = inputColor; + half4 inlineResult_fp_0; + half4 inlineArg_fp_1_0 = inputColor; { - inlineResult0 = half4(inlineArg1_0.xyz / max(inlineArg1_0.w, 9.9999997473787516e-05), inlineArg1_0.w); + inlineResult_fp_0 = half4(inlineArg_fp_1_0.xyz / max(inlineArg_fp_1_0.w, 9.9999997473787516e-05), inlineArg_fp_1_0.w); } - inputColor = inlineResult0; + inputColor = inlineResult_fp_0; } %s = %s * inputColor + %s; diff --git a/src/gpu/effects/generated/GrHighContrastFilterEffect.cpp b/src/gpu/effects/generated/GrHighContrastFilterEffect.cpp index cfcd847548..5af6878382 100644 --- a/src/gpu/effects/generated/GrHighContrastFilterEffect.cpp +++ b/src/gpu/effects/generated/GrHighContrastFilterEffect.cpp @@ -51,12 +51,12 @@ return t < 0.16666666666666666 ? p + ((q - p) * 6.0) * t : (t < 0.5 ? q : (t < 0 fragBuilder->codeAppendf( R"SkSL( half4 inColor = %s; -half4 inlineResult0; -half4 inlineArg1_0 = inColor; +half4 inlineResult_fp_0; +half4 inlineArg_fp_1_0 = inColor; { - inlineResult0 = half4(inlineArg1_0.xyz / max(inlineArg1_0.w, 9.9999997473787516e-05), inlineArg1_0.w); + inlineResult_fp_0 = half4(inlineArg_fp_1_0.xyz / max(inlineArg_fp_1_0.w, 9.9999997473787516e-05), inlineArg_fp_1_0.w); } -half4 color = inlineResult0; +half4 color = inlineResult_fp_0; @if (%s) { color.xyz = color.xyz * color.xyz; diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp index 15d91c3912..fed7c85d22 100644 --- a/src/sksl/SkSLIRGenerator.cpp +++ b/src/sksl/SkSLIRGenerator.cpp @@ -2199,11 +2199,23 @@ std::unique_ptr IRGenerator::inlineCall( // order guarantees. Since we can't use gotos (which are normally used to replace return // statements), we wrap the whole function in a loop and use break statements to jump to the // end. + + // Use unique variable names during the PipelineStage and FragmentProcessor passes. Otherwise, + // the SkSL we make in the resulting FP will have variables with names like 'inlineResult0'. + // If that FP's code is then inlined again when the combined fragment shader SkSL is compiled, + // we'll reuse those names (the counter is reset), creating a conflict. (skbug.com/10526) + const char* inlineSalt = ""; + switch (fKind) { + case Program::kPipelineStage_Kind: inlineSalt = "_ps_"; break; + case Program::kFragmentProcessor_Kind: inlineSalt = "_fp_"; break; + default: break; + } + Variable* resultVar; if (function.fDeclaration.fReturnType != *fContext.fVoid_Type) { std::unique_ptr name(new String()); int varIndex = fInlineVarCounter++; - name->appendf("inlineResult%d", varIndex); + name->appendf("inlineResult%s%d", inlineSalt, varIndex); String* namePtr = (String*) fSymbolTable->takeOwnership(std::move(name)); resultVar = (Variable*) fSymbolTable->takeOwnership(std::unique_ptr( new Variable(-1, Modifiers(), namePtr->c_str(), @@ -2225,7 +2237,7 @@ std::unique_ptr IRGenerator::inlineCall( int argIndex = fInlineVarCounter++; for (int i = 0; i < (int) arguments.size(); ++i) { std::unique_ptr argName(new String()); - argName->appendf("inlineArg%d_%d", argIndex, i); + argName->appendf("inlineArg%s%d_%d", inlineSalt, argIndex, i); String* argNamePtr = (String*) fSymbolTable->takeOwnership(std::move(argName)); Variable* argVar = (Variable*) fSymbolTable->takeOwnership(std::unique_ptr( new Variable(-1, Modifiers(), diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp index 9ec8234dc8..e39c16c4c3 100644 --- a/tests/SkRuntimeEffectTest.cpp +++ b/tests/SkRuntimeEffectTest.cpp @@ -196,6 +196,10 @@ static void test_RuntimeEffect_Shaders(skiatest::Reporter* r, GrContext* context effect.test(0xFF000000, 0xFF00007F, 0xFF007F00, 0xFF007F7F, [](SkCanvas* canvas, SkPaint*) { canvas->rotate(45.0f); }); + // + // Sampling children + // + // Sampling a null child should return the paint color effect.build("in shader child;", "color = sample(child);"); @@ -222,6 +226,16 @@ static void test_RuntimeEffect_Shaders(skiatest::Reporter* r, GrContext* context "color = sample(child, float3x3(0, 1, 0, 1, 0, 0, 0, 0, 1));"); effect.child("child") = rgbwShader; effect.test(0xFF0000FF, 0xFFFF0000, 0xFF00FF00, 0xFFFFFFFF); + + // + // Helper functions + // + + // Test case for inlining in the pipeline-stage and fragment-shader passes (skbug.com/10526): + effect.build("float2 helper(float2 x) { return x + 1; }", + "float2 v = helper(p);" + "color = half4(half2(v), 0, 1);"); + effect.test(0xFF00FFFF); } DEF_TEST(SkRuntimeEffectSimple, r) { diff --git a/tests/SkSLFPTest.cpp b/tests/SkSLFPTest.cpp index e8516dc197..4f46f28f75 100644 --- a/tests/SkSLFPTest.cpp +++ b/tests/SkSLFPTest.cpp @@ -830,12 +830,12 @@ DEF_TEST(SkSLFPFunction, r) { "R\"SkSL(return c.wzyx;\n" ")SkSL\", &flip_name);", "fragBuilder->codeAppendf(\n" - "R\"SkSL(half4 inlineResult0;\n" - "half4 inlineArg1_0 = %s;\n" + "R\"SkSL(half4 inlineResult_fp_0;\n" + "half4 inlineArg_fp_1_0 = %s;\n" "{\n" - " inlineResult0 = inlineArg1_0.wzyx;\n" + " inlineResult_fp_0 = inlineArg_fp_1_0.wzyx;\n" "}\n" - "%s = inlineResult0;\n" + "%s = inlineResult_fp_0;\n" "\n" ")SkSL\"\n" ", args.fInputColor, args.fOutputColor);"