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 <brianosman@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
diff --git a/src/gpu/effects/generated/GrColorMatrixFragmentProcessor.cpp b/src/gpu/effects/generated/GrColorMatrixFragmentProcessor.cpp
index 5278c72..e7d3c35 100644
--- a/src/gpu/effects/generated/GrColorMatrixFragmentProcessor.cpp
+++ b/src/gpu/effects/generated/GrColorMatrixFragmentProcessor.cpp
@@ -42,12 +42,12 @@
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 cfcd847..5af6878 100644
--- a/src/gpu/effects/generated/GrHighContrastFilterEffect.cpp
+++ b/src/gpu/effects/generated/GrHighContrastFilterEffect.cpp
@@ -51,12 +51,12 @@
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 15d91c3..fed7c85 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -2199,11 +2199,23 @@
// 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<String> 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<Symbol>(
new Variable(-1, Modifiers(), namePtr->c_str(),
@@ -2225,7 +2237,7 @@
int argIndex = fInlineVarCounter++;
for (int i = 0; i < (int) arguments.size(); ++i) {
std::unique_ptr<String> 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<Symbol>(
new Variable(-1, Modifiers(),
diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp
index 9ec8234..e39c16c 100644
--- a/tests/SkRuntimeEffectTest.cpp
+++ b/tests/SkRuntimeEffectTest.cpp
@@ -196,6 +196,10 @@
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 @@
"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 e8516dc..4f46f28 100644
--- a/tests/SkSLFPTest.cpp
+++ b/tests/SkSLFPTest.cpp
@@ -830,12 +830,12 @@
"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);"