From f2bd501ce3bfd9a394c19e925648bcfea6849300 Mon Sep 17 00:00:00 2001 From: John Stiles Date: Fri, 4 Dec 2020 11:58:26 -0500 Subject: [PATCH] Use references instead of pointers for Metal out params. Pointers require decorating the variable with a * to read back the value, which the code generator did not properly handle. There was a special case to add the * but it only supported assignment into the variable, not reading back. References require no special decoration. This change fixes compile errors in Functions.sksl with the "bar" function. (This test marks `x` as an inout but never actually mutates it.) It also allows us to remove a special-case workaround for `frexp`, an intrinsic function which uses a reference for its out-parameter. Additionally, this CL adds a non-inlining copy of "OutParams.sksl" to the Metal test directory, as most of our tests which use out-parameters end up inlining all the code, which hides these sorts of bugs. Change-Id: I31c4db04f6b512b4cd4fe65b3347b82bdbf039cd Reviewed-on: https://skia-review.googlesource.com/c/skia/+/341000 Reviewed-by: Brian Osman Commit-Queue: John Stiles --- gn/sksl_tests.gni | 1 + src/sksl/SkSLMetalCodeGenerator.cpp | 24 +--- tests/sksl/metal/OutParams.sksl | 73 ++++++++++++ tests/sksl/metal/golden/OutParams.metal | 137 +++++++++++++++++++++++ tests/sksl/shared/golden/FrExp.metal | 3 +- tests/sksl/shared/golden/Functions.metal | 4 +- 6 files changed, 215 insertions(+), 27 deletions(-) create mode 100644 tests/sksl/metal/OutParams.sksl create mode 100644 tests/sksl/metal/golden/OutParams.metal diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni index 93e373fb20..f8d730a35a 100644 --- a/gn/sksl_tests.gni +++ b/gn/sksl_tests.gni @@ -184,6 +184,7 @@ sksl_metal_tests = [ "$_tests/sksl/metal/NumericGlobals.sksl", "$_tests/sksl/metal/OpaqueTypeInInterfaceBlock.sksl", "$_tests/sksl/metal/OpaqueTypeInStruct.sksl", + "$_tests/sksl/metal/OutParams.sksl", "$_tests/sksl/metal/OutVarsRequireLocation.sksl", "$_tests/sksl/metal/SamplerGlobals.sksl", ] diff --git a/src/sksl/SkSLMetalCodeGenerator.cpp b/src/sksl/SkSLMetalCodeGenerator.cpp index 1ba9b3eaca..f2994978d1 100644 --- a/src/sksl/SkSLMetalCodeGenerator.cpp +++ b/src/sksl/SkSLMetalCodeGenerator.cpp @@ -279,17 +279,6 @@ void MetalCodeGenerator::writeFunctionCall(const FunctionCall& c) { } else if (builtin && name == "inverse") { SkASSERT(arguments.size() == 1); this->writeInverseHack(*arguments[0]); - } else if (builtin && name == "frexp") { - // Our Metal codegen assumes that out params are pointers, but Metal's built-in frexp - // actually takes a reference for the exponent, not a pointer. We add in a helper function - // here to translate. - SkASSERT(arguments.size() == 2); - auto [iter, newlyCreated] = fHelpers.insert("frexp"); - if (newlyCreated) { - fExtraFunctions.printf( - "float frexp(float arg, thread int* exp) { return frexp(arg, *exp); }\n"); - } - this->write("frexp"); } else if (builtin && name == "dFdx") { this->write("dfdx"); } else if (builtin && name == "dFdy") { @@ -324,14 +313,10 @@ void MetalCodeGenerator::writeFunctionCall(const FunctionCall& c) { this->write("_fragCoord"); separator = ", "; } - const std::vector& parameters = function.parameters(); for (size_t i = 0; i < arguments.size(); ++i) { const Expression& arg = *arguments[i]; this->write(separator); separator = ", "; - if (parameters[i]->modifiers().fFlags & Modifiers::kOut_Flag) { - this->write("&"); - } this->writeExpression(arg, kSequence_Precedence); } this->write(")"); @@ -943,13 +928,6 @@ void MetalCodeGenerator::writeBinaryExpression(const BinaryExpression& b, if (needParens) { this->write("("); } - if (Compiler::IsAssignment(op) && left.is() && - left.as().variable()->storage() == Variable::Storage::kParameter && - left.as().variable()->modifiers().fFlags & Modifiers::kOut_Flag) { - // writing to an out parameter. Since we have to turn those into pointers, we have to - // dereference it here. - this->write("*"); - } if (op == Token::Kind::TK_STAREQ && leftType.isMatrix() && rightType.isMatrix()) { this->writeMatrixTimesEqualHelper(leftType, rightType, b.type()); } @@ -1153,7 +1131,7 @@ bool MetalCodeGenerator::writeFunctionDeclaration(const FunctionDeclaration& f) const Type* type = ¶m->type(); this->writeBaseType(*type); if (param->modifiers().fFlags & Modifiers::kOut_Flag) { - this->write("*"); + this->write("&"); } this->write(" "); this->writeName(param->name()); diff --git a/tests/sksl/metal/OutParams.sksl b/tests/sksl/metal/OutParams.sksl new file mode 100644 index 0000000000..b5c8d2d313 --- /dev/null +++ b/tests/sksl/metal/OutParams.sksl @@ -0,0 +1,73 @@ +/*#pragma settings NoInline*/ + +void out_half (out half v) { v = 1; } +void out_half2(out half2 v) { v = half2(2); } +void out_half3(out half3 v) { v = half3(3); } +void out_half4(out half4 v) { v = half4(4); } + +void out_half2x2(out half2x2 v) { v = half2x2(2); } +void out_half3x3(out half3x3 v) { v = half3x3(3); } +void out_half4x4(out half4x4 v) { v = half4x4(4); } + +void out_int (out int v) { v = 1; } +void out_int2(out int2 v) { v = int2(2); } +void out_int3(out int3 v) { v = int3(3); } +void out_int4(out int4 v) { v = int4(4); } + +void out_float (out float v) { v = 1; } +void out_float2(out float2 v) { v = float2(2); } +void out_float3(out float3 v) { v = float3(3); } +void out_float4(out float4 v) { v = float4(4); } + +void out_float2x2(out float2x2 v) { v = float2x2(2); } +void out_float3x3(out float3x3 v) { v = float3x3(3); } +void out_float4x4(out float4x4 v) { v = float4x4(4); } + +void out_bool (out bool v) { v = true; } +void out_bool2(out bool2 v) { v = bool2(false); } +void out_bool3(out bool3 v) { v = bool3(true); } +void out_bool4(out bool4 v) { v = bool4(false); } + +void main() { + half h; out_half (h); + half2 h2; out_half2(h2); + half3 h3; out_half3(h3); + half4 h4; out_half4(h4); + out_half2(h3.xz); + out_half4(h4.zwxy); + sk_FragColor = half4(h, h2.x, h3.x, h4.x); + + half2x2 h2x2; out_half2x2(h2x2); + half3x3 h3x3; out_half3x3(h3x3); + half4x4 h4x4; out_half4x4(h4x4); + out_half3(h3x3[1]); + out_half(h4x4[3].w); + sk_FragColor = half4(h2x2[0][0], h3x3[0][0], h4x4[0][0], 1); + + int i; out_int (i); + int2 i2; out_int2(i2); + int3 i3; out_int3(i3); + int4 i4; out_int4(i4); + out_int3(i4.xyz); + sk_FragColor = half4(i, i2.x, i3.x, i4.x); + + float f; out_float (f); + float2 f2; out_float2(f2); + float3 f3; out_float3(f3); + float4 f4; out_float4(f4); + out_float2(f3.xy); + sk_FragColor = half4(half(f), half(f2.x), half(f3.x), half(f4.x)); + + float2x2 f2x2; out_float2x2(f2x2); + float3x3 f3x3; out_float3x3(f3x3); + float4x4 f4x4; out_float4x4(f4x4); + out_float(f2x2[0][0]); + sk_FragColor = half4(half(f2x2[0][0]), half(f3x3[0][0]), half(f4x4[0][0]), 1); + + bool b; out_bool (b); + bool2 b2; out_bool2(b2); + bool3 b3; out_bool3(b3); + bool4 b4; out_bool4(b4); + out_bool2(b4.xw); + sk_FragColor = half4(half(b), half(b2.x), half(b3.x), half(b4.x)); +} diff --git a/tests/sksl/metal/golden/OutParams.metal b/tests/sksl/metal/golden/OutParams.metal new file mode 100644 index 0000000000..b2605a59de --- /dev/null +++ b/tests/sksl/metal/golden/OutParams.metal @@ -0,0 +1,137 @@ +#include +#include +using namespace metal; +struct Inputs { +}; +struct Outputs { + float4 sk_FragColor [[color(0)]]; +}; +void out_half(thread float& v) { + v = 1.0; +} +void out_half2(thread float2& v) { + v = float2(2.0); +} +void out_half3(thread float3& v) { + v = float3(3.0); +} +void out_half4(thread float4& v) { + v = float4(4.0); +} +void out_half2x2(thread float2x2& v) { + v = float2x2(2.0); +} +void out_half3x3(thread float3x3& v) { + v = float3x3(3.0); +} +void out_half4x4(thread float4x4& v) { + v = float4x4(4.0); +} +void out_int(thread int& v) { + v = 1; +} +void out_int2(thread int2& v) { + v = int2(2); +} +void out_int3(thread int3& v) { + v = int3(3); +} +void out_int4(thread int4& v) { + v = int4(4); +} +void out_float(thread float& v) { + v = 1.0; +} +void out_float2(thread float2& v) { + v = float2(2.0); +} +void out_float3(thread float3& v) { + v = float3(3.0); +} +void out_float4(thread float4& v) { + v = float4(4.0); +} +void out_float2x2(thread float2x2& v) { + v = float2x2(2.0); +} +void out_float3x3(thread float3x3& v) { + v = float3x3(3.0); +} +void out_float4x4(thread float4x4& v) { + v = float4x4(4.0); +} +void out_bool(thread bool& v) { + v = true; +} +void out_bool2(thread bool2& v) { + v = bool2(false); +} +void out_bool3(thread bool3& v) { + v = bool3(true); +} +void out_bool4(thread bool4& v) { + v = bool4(false); +} +fragment Outputs fragmentMain(Inputs _in [[stage_in]], bool _frontFacing [[front_facing]], float4 _fragCoord [[position]]) { + Outputs _outputStruct; + thread Outputs* _out = &_outputStruct; + float h; + out_half(h); + float2 h2; + out_half2(h2); + float3 h3; + out_half3(h3); + float4 h4; + out_half4(h4); + out_half2(h3.xz); + out_half4(h4.zwxy); + _out->sk_FragColor = float4(h, h2.x, h3.x, h4.x); + float2x2 h2x2; + out_half2x2(h2x2); + float3x3 h3x3; + out_half3x3(h3x3); + float4x4 h4x4; + out_half4x4(h4x4); + out_half3(h3x3[1]); + out_half(h4x4[3].w); + _out->sk_FragColor = float4(h2x2[0][0], h3x3[0][0], h4x4[0][0], 1.0); + int i; + out_int(i); + int2 i2; + out_int2(i2); + int3 i3; + out_int3(i3); + int4 i4; + out_int4(i4); + out_int3(i4.xyz); + _out->sk_FragColor = float4(float(i), float(i2.x), float(i3.x), float(i4.x)); + float f; + out_float(f); + float2 f2; + out_float2(f2); + float3 f3; + out_float3(f3); + float4 f4; + out_float4(f4); + out_float2(f3.xy); + _out->sk_FragColor = float4(f, f2.x, f3.x, f4.x); + float2x2 f2x2; + out_float2x2(f2x2); + float3x3 f3x3; + out_float3x3(f3x3); + float4x4 f4x4; + out_float4x4(f4x4); + out_float(f2x2[0][0]); + _out->sk_FragColor = float4(f2x2[0][0], f3x3[0][0], f4x4[0][0], 1.0); + bool b; + out_bool(b); + bool2 b2; + out_bool2(b2); + bool3 b3; + out_bool3(b3); + bool4 b4; + out_bool4(b4); + out_bool2(b4.xw); + _out->sk_FragColor = float4(b ? 1.0 : 0.0, b2.x ? 1.0 : 0.0, b3.x ? 1.0 : 0.0, b4.x ? 1.0 : 0.0); + return *_out; +} diff --git a/tests/sksl/shared/golden/FrExp.metal b/tests/sksl/shared/golden/FrExp.metal index 643159366d..d5277f7c27 100644 --- a/tests/sksl/shared/golden/FrExp.metal +++ b/tests/sksl/shared/golden/FrExp.metal @@ -6,12 +6,11 @@ struct Inputs { struct Outputs { float4 sk_FragColor [[color(0)]]; }; -float frexp(float arg, thread int* exp) { return frexp(arg, *exp); } fragment Outputs fragmentMain(Inputs _in [[stage_in]], bool _frontFacing [[front_facing]], float4 _fragCoord [[position]]) { Outputs _outputStruct; thread Outputs* _out = &_outputStruct; int exp; - float foo = frexp(0.5, &exp); + float foo = frexp(0.5, exp); _out->sk_FragColor = float4(float(exp)); return *_out; } diff --git a/tests/sksl/shared/golden/Functions.metal b/tests/sksl/shared/golden/Functions.metal index 042ac36914..b6d639a05b 100644 --- a/tests/sksl/shared/golden/Functions.metal +++ b/tests/sksl/shared/golden/Functions.metal @@ -9,7 +9,7 @@ struct Outputs { float foo(float v[2]) { return v[0] * v[1]; } -void bar(thread float* x) { +void bar(thread float& x) { float y[2]; y[0] = x; @@ -20,7 +20,7 @@ fragment Outputs fragmentMain(Inputs _in [[stage_in]], bool _frontFacing [[front Outputs _outputStruct; thread Outputs* _out = &_outputStruct; float x = 10.0; - bar(&x); + bar(x); _out->sk_FragColor = float4(x); return *_out; }