Revision d7515c30209cc995573d9b0258de1716c30c6012 authored by Tim Foley on 23 May 2018, 18:49:18 UTC, committed by GitHub on 23 May 2018, 18:49:18 UTC
Fixes #568

The problem occurs when an entry point declares multiple `out` parameters:

```hlsl
void myVS( out float4 a : A, out float4 b : B )
{
    ...
    a = whatever;
    b = somethingElse;
    ...
    if(done)
    {
        return; // explicit return
    }
    ...
    // implicit return
}
```

Slang translates code like this by introducing a GLSL global `out` parameter for each of `a` and `b`, rewriting the logic inside the entry point to use a local temporary instead of the real parameters, and then assigning from the locals to the globals at every `return` site:

```glsl
out vec4 g_a;
out vec4 g_b;

void main()
{
    // insertion location (1)
    vec4 t_a;
    vec4 t_b;
    ...
    t_a = whatever;
    t_b = somethingElse;
    ...
    if(done)
    {
        // insertion location(2)
        g_a = t_a;
        g_b = t_b;
        return; // explicit return
    }
    ...
    // insertion location (3)
    g_a = t_a;
    g_b = t_b;
    // implicit return
}
```

Note that there are three different places (for this example) where code gets inserted to make the translation work. We insert declarations of local variables at the top of the function, and then insert the copy from the temporariesto the globals at each `return` site (implicit or explicit).

The bug in this case was that the pass was setting the insertion location to (1) outside of the loop for parameters, so that when it was done with `a` and moved on to `b`, it would end up inseting the temporary `t_b` at the last location used (location (3) in this example), and this would result in invalid code, because `t_b` gets used before it is declared.

This bug has been around for a while, but it has largely been masked by the fact that so few shaders use multiple `out` parameters, and also because Slang's SSA-ification pass would often be able to eliminate the local variable anyway, so that the bug never bites the user. The reason it surfaced now for a user shader was because we introduced `swizzledStore`, which currently inhibits SSA-ification, so that some temporaries that used to get eliminated are now retained so that they can break things.

The fix in this case is small: we use the existing `IRBuilder` only for insertions at location (1) and construct a new builder on the fly for all the insertions at `return` sites. I have not included a test case yet, because our end-to-end Vulkan testing is not yet ready, so this may regress again in the future.
1 parent 76652fa
History

back to top