commit f2bd501ce3bfd9a394c19e925648bcfea6849300
author John Stiles <johnstiles@google.com> Fri Dec 04 11:58:26 2020 -0500
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Fri Dec 04 20:22:55 2020 +0000
tree 880b0f30dc250778353197648976abb418796d7c
parent bc3c41b8742aa3430b5da0ac97de42f0fe25fe85

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>

src/sksl/SkSLMetalCodeGenerator.cpp

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 = &param->type();
         this->writeBaseType(*type);
         if (param->modifiers().fFlags & Modifiers::kOut_Flag) {
-            this->write("*");
+            this->write("&");
         }
         this->write(" ");
         this->writeName(param->name());