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 <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index 93e373f..f8d730a 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -184,6 +184,7 @@
"$_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 1ba9b3e..f299497 100644
--- a/src/sksl/SkSLMetalCodeGenerator.cpp
+++ b/src/sksl/SkSLMetalCodeGenerator.cpp
@@ -279,17 +279,6 @@
} 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 @@
this->write("_fragCoord");
separator = ", ";
}
- const std::vector<const Variable*>& 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 @@
if (needParens) {
this->write("(");
}
- if (Compiler::IsAssignment(op) && left.is<VariableReference>() &&
- left.as<VariableReference>().variable()->storage() == Variable::Storage::kParameter &&
- left.as<VariableReference>().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 @@
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 0000000..b5c8d2d
--- /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 0000000..b2605a5
--- /dev/null
+++ b/tests/sksl/metal/golden/OutParams.metal
@@ -0,0 +1,137 @@
+#include <metal_stdlib>
+#include <simd/simd.h>
+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 6431593..d5277f7 100644
--- a/tests/sksl/shared/golden/FrExp.metal
+++ b/tests/sksl/shared/golden/FrExp.metal
@@ -6,12 +6,11 @@
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 042ac36..b6d639a 100644
--- a/tests/sksl/shared/golden/Functions.metal
+++ b/tests/sksl/shared/golden/Functions.metal
@@ -9,7 +9,7 @@
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 @@
Outputs _outputStruct;
thread Outputs* _out = &_outputStruct;
float x = 10.0;
- bar(&x);
+ bar(x);
_out->sk_FragColor = float4(x);
return *_out;
}
