Skip to content

Commit

Permalink
Fix Slang->GLSL translation for entry point with multiple out param…
Browse files Browse the repository at this point in the history
…eters (#573)

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.
  • Loading branch information
Tim Foley authored May 23, 2018
1 parent 76652fa commit d7515c3
Showing 1 changed file with 10 additions and 2 deletions.
12 changes: 10 additions & 2 deletions source/slang/ir.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4515,9 +4515,17 @@ namespace Slang
break;
}

builder.setInsertBefore(terminatorInst);
// We dont' re-use `builder` here because we don't want to
// disrupt the source location it is using for inserting
// temporary variables at the top of the function.
//
IRBuilder terminatorBuilder;
terminatorBuilder.sharedBuilder = builder.sharedBuilder;
terminatorBuilder.setInsertBefore(terminatorInst);

assign(&builder, globalOutputVal, localVal);
// Assign from the local variabel to the global output
// variable before the actual `return` takes place.
assign(&terminatorBuilder, globalOutputVal, localVal);
}
}
else
Expand Down

0 comments on commit d7515c3

Please sign in to comment.