From 60934d98fbc20d83b5e149e72a197ec4f5c61580 Mon Sep 17 00:00:00 2001 From: Tim Foley Date: Thu, 19 Dec 2019 07:11:56 -0800 Subject: Fix invocation of `[mutating]` methods (#1156) The logic for invoking methods (member functions) in `slang-lower-to-ir.cpp` was failing to take into account whether the callee was `[mutating]` or not. Instead, it would always lower the `base` expression in something like `base.f(...)` as an r-value expression, consistent with a non-`[mutating]` method. The incorrect code generation strategy somehow turned out to work in many cases, but it broke in cases where a `[mutating]` method was called on an `inout` parameter. E.g., in this code: ```hlsl struct Stuff { [mutating] void doThing() { ... } } void broken(inout Stuff s) { s.doThing(); } ``` The `broken` function would fail to write back the value mutated by `doThing` to its `s` parameter before returning. The crux of the fix here is inside `visitInvokeExpr()`. Instead of directly calling `lowerRValueExpr` on the base expression of a method/member-function call, we instead compute the "direction" of the `this` parameter in the callee, and use that to emit the argument expression appropriately. In order to enable that change, there are several refactorings included: * The existing `ParameterDirection` and `getParameterDirection()` calls were lifted out from the declaration visitor to the global scope, so that they could be shared between lowering of functions and their call sites. * The logic for determining the "direction" of a `this` parameter was factored out of `collectParameterLists()` into its own `getThisParamDirection()` subroutine (again so that functions and call sites can share matching logic). * The logic for turning an AST expression used as a call argument into IR argument(s)* was pulled out into its own `addCallArgsForParam` *and* was refactored to rely on a `ParameterDirection` instead of directly inspecting the modifiers on a `ParamDecl`. This allows the function to be used for ordinary/direct arguments and the `this` argument, and also ensures that the caller and callee will agree on the direction of parameters. Fixing the way that `[mutating]` methods are called actually broke some test cases, specifically in the cases where a `[mutating]` method was being called on a value with an interface-constrained generic type: ```hlsl interface IThing { [mutating] void doStuff(); } void myFunc(inout T thing) { thing.doStuff(); } ``` Our argument passing for `inout` parameters currently requires that we make a temp copy of `thing` into a local, and then pass that local as argument for the `inout` parameter, before copying back. The issue that arose was that a simple version of the logic uses the type of the `base` expression in `base.someMethod(...)` as the type of the local variable, but for an interface method call the base expression will have been cast to the interface type (we effectively have `((IThing) thing).doStuff()`. The fix here was to query the this type through the member function we are calling, and to share that logic between the function-call and function-declaration cases, to try and make sure they match, which meant even more logic got hoisted out of the declaration-emission logic and to the top level. Note: This change does *not* clean up any other clarity or performance concerns around `out` and `inout` parameters; it is only focused on correctness. --- tests/compute/mutating-and-inout.slang | 49 ++++++++++++++++++++++ .../compute/mutating-and-inout.slang.expected.txt | 4 ++ 2 files changed, 53 insertions(+) create mode 100644 tests/compute/mutating-and-inout.slang create mode 100644 tests/compute/mutating-and-inout.slang.expected.txt (limited to 'tests') diff --git a/tests/compute/mutating-and-inout.slang b/tests/compute/mutating-and-inout.slang new file mode 100644 index 000000000..d06933b77 --- /dev/null +++ b/tests/compute/mutating-and-inout.slang @@ -0,0 +1,49 @@ +// mutating-and-inout.slang + +// Test that calling a `[mutating]` method on an `inout` function parameter works. + +//TEST(compute):COMPARE_COMPUTE: + +//TEST_INPUT:ubuffer(data=[0 0 0 0], stride=4):out,name=outputBuffer +RWStructuredBuffer outputBuffer; + +struct A +{ + int x; + + [mutating] void doThings(inout int y) + { + int tmp = x; + x = y; + y = tmp; + } +} + +int doThings(inout A a, int val) +{ + a.doThings(val); + return val; +} + +int test(int val) +{ + A a = { val }; + int b = val ^ 3; + + int c = doThings(a, b); + + int result = 0; + result = result*16 + a.x; + result = result*16 + b; + result = result*16 + c; + + return result; +} + +[numthreads(4, 1, 1)] +void computeMain(uint3 tid : SV_DispatchThreadID) +{ + int val = tid.x; + val = test(val); + outputBuffer[tid.x] = val; +} diff --git a/tests/compute/mutating-and-inout.slang.expected.txt b/tests/compute/mutating-and-inout.slang.expected.txt new file mode 100644 index 000000000..6c842da55 --- /dev/null +++ b/tests/compute/mutating-and-inout.slang.expected.txt @@ -0,0 +1,4 @@ +330 +221 +112 +3 -- cgit v1.2.3