ByteCode: Support out params in local function calls

Change-Id: I1d133259264adfdc872b0f4aeaa9390363c46341
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/222040
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

Author: brianosman

src/sksl/SkSLByteCode.cpp

@@ -146,6 +146,7 @@ static const uint8_t* disassemble_instruction(const uint8_t* ip) {
         VECTOR_DISASSEMBLE(kRemainderF, "remainderf")
         VECTOR_DISASSEMBLE(kRemainderS, "remainders")
         VECTOR_DISASSEMBLE(kRemainderU, "remainderu")
+        case ByteCodeInstruction::kReserve: printf("reserve %d", READ8()); break;
         case ByteCodeInstruction::kReturn: printf("return %d", READ8()); break;
         case ByteCodeInstruction::kScalarToMatrix: {
             int cols = READ8();
@@ -334,6 +335,7 @@ struct StackFrame {
     const uint8_t* fCode;
     const uint8_t* fIP;
     VValue* fStack;
+    int fParameterCount;
 };
 
 static F32 mix(F32 start, F32 end, F32 t) {
@@ -404,19 +406,16 @@ void innerRun(const ByteCode* byteCode, const ByteCodeFunction* f, VValue* stack
                 break;
 
             case ByteCodeInstruction::kCall: {
-                // Precursor code has pushed all parameters to the stack. Update our bottom of
-                // stack to point at the first parameter, and our sp to point past those parameters
-                // (plus space for locals).
+                // Precursor code reserved space for the return value, and pushed all parameters to
+                // the stack. Update our bottom of stack to point at the first parameter, and our
+                // sp to point past those parameters (plus space for locals).
                 int target = READ8();
                 const ByteCodeFunction* fun = byteCode->fFunctions[target].get();
                 if (skvx::any(mask())) {
-                    frames.push_back({ code, ip, stack });
+                    frames.push_back({ code, ip, stack, fun->fParameterCount });
                     ip = code = fun->fCode.data();
                     stack = sp - fun->fParameterCount + 1;
                     sp = stack + fun->fParameterCount + fun->fLocalCount - 1;
-                } else {
-                    sp -= fun->fParameterCount;
-                    sp += fun->fReturnCount;
                 }
                 break;
             }
@@ -715,6 +714,10 @@ void innerRun(const ByteCode* byteCode, const ByteCodeFunction* f, VValue* stack
             VECTOR_BINARY_FN(kRemainderS, fSigned, vec_mod<I32>)
             VECTOR_BINARY_FN(kRemainderU, fUnsigned, vec_mod<U32>)
 
+            case ByteCodeInstruction::kReserve:
+                sp += READ8();
+                break;
+
             case ByteCodeInstruction::kReturn: {
                 int count = READ8();
                 if (frames.empty()) {
@@ -742,14 +745,17 @@ void innerRun(const ByteCode* byteCode, const ByteCodeFunction* f, VValue* stack
                     }
                     return;
                 } else {
-                    // When we were called, 'stack' was positioned at the old top-of-stack (where
-                    // our parameters were placed). So copy our return values to that same spot.
-                    memmove(stack, sp - count + 1, count * sizeof(VValue));
-
-                    // Now move the stack pointer to the end of the just-pushed return values,
-                    // and restore everything else.
+                    // When we were called, the caller reserved stack space for their copy of our
+                    // return value, then 'stack' was positioned after that, where our parameters
+                    // were placed. Copy our return values to their reserved area.
+                    memcpy(stack - count, sp - count + 1, count * sizeof(VValue));
+
+                    // Now move the stack pointer to the end of the passed-in parameters. This odd
+                    // calling convention requires the caller to pop the arguments after calling,
+                    // but allows them to store any out-parameters back during that unwinding.
+                    // After that sequence finishes, the return value will be the top of the stack.
                     const StackFrame& frame(frames.back());
-                    sp = stack + count - 1;
+                    sp = stack + frame.fParameterCount - 1;
                     stack = frame.fStack;
                     code = frame.fCode;
                     ip = frame.fIP;

src/sksl/SkSLByteCode.h

@@ -90,6 +90,8 @@ enum class ByteCodeInstruction : uint16_t {
     VECTOR(kRemainderF),
     VECTOR(kRemainderS),
     VECTOR(kRemainderU),
+    // Followed by a byte indicating the number of slots to reserve on the stack (for later return)
+    kReserve,
     // Followed by a byte indicating the number of slots being returned
     kReturn,
     // Followed by two bytes indicating columns and rows of matrix (2, 3, or 4 each).

src/sksl/SkSLByteCodeGenerator.cpp

@@ -637,15 +637,63 @@ void ByteCodeGenerator::writeIntrinsicCall(const FunctionCall& c) {
 }
 
 void ByteCodeGenerator::writeFunctionCall(const FunctionCall& f) {
-    for (const auto& arg : f.fArguments) {
-        this->writeExpression(*arg);
-    }
+    // Builtins have simple signatures...
     if (f.fFunction.fBuiltin) {
+        for (const auto& arg : f.fArguments) {
+            this->writeExpression(*arg);
+        }
         this->writeIntrinsicCall(f);
         return;
     }
+
+    // Otherwise, we may need to deal with out parameters, so the sequence is trickier...
+    if (int returnCount = SlotCount(f.fType)) {
+        this->write(ByteCodeInstruction::kReserve);
+        this->write8(returnCount);
+    }
+
+    int argCount = f.fArguments.size();
+    std::vector<std::unique_ptr<LValue>> lvalues;
+    for (int i = 0; i < argCount; ++i) {
+        const auto& param = f.fFunction.fParameters[i];
+        const auto& arg = f.fArguments[i];
+        if (param->fModifiers.fFlags & Modifiers::kOut_Flag) {
+            lvalues.emplace_back(this->getLValue(*arg));
+            lvalues.back()->load();
+        } else {
+            this->writeExpression(*arg);
+        }
+    }
+
     this->write(ByteCodeInstruction::kCall);
     fCallTargets.emplace_back(this, f.fFunction);
+
+    // After the called function returns, the stack will still contain our arguments. We have to
+    // pop them (storing any out parameters back to their lvalues as we go). We glob together slot
+    // counts for all parameters that aren't out-params, so we can pop them in one big chunk.
+    int popCount = 0;
+    auto pop = [&]() {
+        if (popCount > 4) {
+            this->write(ByteCodeInstruction::kPopN);
+            this->write8(popCount);
+        } else if (popCount > 0) {
+            this->write(vector_instruction(ByteCodeInstruction::kPop, popCount));
+        }
+        popCount = 0;
+    };
+
+    for (int i = argCount - 1; i >= 0; --i) {
+        const auto& param = f.fFunction.fParameters[i];
+        const auto& arg = f.fArguments[i];
+        if (param->fModifiers.fFlags & Modifiers::kOut_Flag) {
+            pop();
+            lvalues.back()->store(true);
+            lvalues.pop_back();
+        } else {
+            popCount += SlotCount(arg->fType);
+        }
+    }
+    pop();
 }
 
 void ByteCodeGenerator::writeIntLiteral(const IntLiteral& i) {

tests/SkSLInterpreterTest.cpp

@@ -661,6 +661,23 @@ DEF_TEST(SkSLInterpreterFunctions, r) {
     REPORTER_ASSERT(r, fibOut == 13);
 }
 
+DEF_TEST(SkSLInterpreterOutParams, r) {
+    test(r,
+         "void oneAlpha(inout half4 color) { color.a = 1; }"
+         "void main(inout half4 color) { oneAlpha(color); }",
+         0, 0, 0, 0, 0, 0, 0, 1);
+    test(r,
+         "half2 tricky(half x, half y, inout half2 color, half z) {"
+         "    color.xy = color.yx;"
+         "    return half2(x + y, z);"
+         "}"
+         "void main(inout half4 color) {"
+         "    half2 t = tricky(1, 2, color.rb, 5);"
+         "    color.ga = t;"
+         "}",
+         1, 2, 3, 4, 3, 3, 1, 5);
+}
+
 DEF_TEST(SkSLInterpreterMathFunctions, r) {
     float value, expected;