shader-slang / slang

Making it easier to work with shaders
MIT License
1.78k stars 159 forks source link

Function param input modifiers `out` and `inout` are unexpectedly strict #4430

Closed chaoticbob closed 1 week ago

chaoticbob commented 1 week ago

For the shader below, Slang errors out during compilation saying that the parameters need to be l-value when passed to the function:

shader.hlsl(27): error 30047: argument passed to parameter '0' must be l-value.
    return run(r0, r1, r2, r3, r4, r5, r6, r7, r8);
               ^~
shader.hlsl(27): error 30047: argument passed to parameter '1' must be l-value.
    return run(r0, r1, r2, r3, r4, r5, r6, r7, r8);
                   ^~
shader.hlsl(27): error 30047: argument passed to parameter '2' must be l-value.
    return run(r0, r1, r2, r3, r4, r5, r6, r7, r8);
                       ^~
shader.hlsl(27): error 30047: argument passed to parameter '3' must be l-value.
    return run(r0, r1, r2, r3, r4, r5, r6, r7, r8);
                           ^~
shader.hlsl(27): error 30047: argument passed to parameter '4' must be l-value.
    return run(r0, r1, r2, r3, r4, r5, r6, r7, r8);
                               ^~
shader.hlsl(27): error 30047: argument passed to parameter '5' must be l-value.
    return run(r0, r1, r2, r3, r4, r5, r6, r7, r8);
                                   ^~
shader.hlsl(27): error 30047: argument passed to parameter '6' must be l-value.
    return run(r0, r1, r2, r3, r4, r5, r6, r7, r8);
                                       ^~
shader.hlsl(27): error 30047: argument passed to parameter '7' must be l-value.
    return run(r0, r1, r2, r3, r4, r5, r6, r7, r8);
                                           ^~
shader.hlsl(27): error 30047: argument passed to parameter '8' must be l-value.
    return run(r0, r1, r2, r3, r4, r5, r6, r7, r8);

The shader below uses inout but the same thing happens with out. This seems unnecessarily strict and the behavior is unexpected when compared to compiling HLSL with DXC. I couldn't find in any of the specifications out input modifiers are handled for resources arguments. Every doc I could find only talked about input modifiers for declarable variable types.

According to a comment by llvm-beaz in hlsl-specs#28: HLSL's out and inout parameters aren't references (passed by address) they're actually copied in and out of the function.

However, that doesn't seem to line up how DXC treats parameters that are resource types. The behavior for resource arguments seems, very strong emphasis on seems here, to be passed by reference. For instance DXC doesn't complain if a0 is decorated with out or inout even though resource is read-only. However attempting to write to a0 results in this error message:

shader.hlsl:22:21: error: cannot assign to return value because function 'operator[]<const vector<float, 4> &>' returns a const value
    a0[uint2(0, 0)] = float4(0, 0, 0, 0);
    ~~~~~~~~~~~~~~~ ^
note: function 'operator[]<const vector<float, 4> &>' which returns const-qualified type 'const vector<float, 4> &' declared here

I checked DXIL and SPIR-V for when all the parameters are decorated with in, out, and inout in separate cases, the DXIL and SPIR-V for each case appears to be the same.

From the user's perspective, it looks like Slang is expecting the arguments to be l-value to agree with the input modifiers for the declared parameter. I don't now if DXC is making making special cases for resource arguments vs variable arguments, but there's probably a large number of shaders in existence that rely on a more relaxed behavior for resource arguments. At least from the user it has the appearance of pass-by-reference and error checking is done based on resource not access and not on parameter input modifiers.

Shader

Texture2D<float4>               r0;
RWTexture3D<float4>             r1;
SamplerState                    r2;
RaytracingAccelerationStructure r3;
RWBuffer<float4>                r4;
ByteAddressBuffer               r5;
RWByteAddressBuffer             r6;
RWStructuredBuffer<float4>      r7;
AppendStructuredBuffer<float4>  r8;

float4 run(inout    Texture2D<float4>               a0,
           inout    RWTexture3D<float4>             a1,
           inout    SamplerState                    a2,
           inout    RaytracingAccelerationStructure a3,
           inout    RWBuffer<float4>                a4,
           inout    ByteAddressBuffer               a5,
           inout    RWByteAddressBuffer             a6,
           inout    RWStructuredBuffer<float4>      a7,
           inout    AppendStructuredBuffer<float4>  a8)
{
    float4 pos = a4.Load(0);
    return a0.Sample(a2, float2(a6.Load(pos.x), a5.Load(pos.y)));
}

float4 main(): SV_Target
{
    return run(r0, r1, r2, r3, r4, r5, r6, r7, r8);
}
csyonghe commented 1 week ago

This will be one of the cases we will NOT follow dxc's behavior because that behavior is fundamentally flawed.

In HLSL, resource types are just handles.

If a function has an inout Texture2D, it doesn't mean the texture itself is modifiable, instead it just means the function may return another texture handle through that function parameter. This means that the caller needs to pass in a l-valued texture handle. Since global shader parameters are not l-values, passing them to an out parameter is incorrect from the type system's point of view, and this behavior is consistent and desired.

The HLSL language has made various mistakes confusing the mutability of a handle vs. the things pointed to by the handle. An example is the OutputIndices type of mesh shaders. They should have been treated as an handle through which modifications are made, therefore the handle itself should just be "in", instead of "out" in the entry point signature.

In slang we care a lot about type system consistency and we try to avoid confusing mistakes in HLSL.

csyonghe commented 1 week ago

I will close this issue because I feel strong enough that this isn't something we should "fix" in Slang.