Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non uniform indices are not always properly handled by naga #6270

Open
hasenbanck opened this issue Sep 14, 2024 · 2 comments · May be fixed by #6276
Open

Non uniform indices are not always properly handled by naga #6270

hasenbanck opened this issue Sep 14, 2024 · 2 comments · May be fixed by #6276
Labels
naga Shader Translator type: bug Something isn't working

Comments

@hasenbanck
Copy link
Contributor

Description
We had an off error that only occured on AMD GPUs (Vega, RDNA1 and RDNA2), where under Vulkan and DX12 textures where rendered with the wrong texel information (see attached screenshot). We use the "SAMPLED_TEXTURE_AND_STORAGE_BUFFER_ARRAY_NON_UNIFORM_INDEXING" feature to select textures based on non-uniform vertex data inside the fragment shader. We found out, that the AMD devices sometimes used the wrong texture for some texels. We debugged the problem under Vulkan and saw, that sometimes in the SPIRV output the indexes used for indexing the binding array witht the texture where not properly markes as non-uniform indexes.

We traces the problem back to naga, which will not emmit the necessary marker for non-unified indexes when using fragment shader with no wrapper struct for the function parameter. We reproduces the problem using the texture_arrays example.

Repro steps
Use the current trunk branch (eb18854) and remove the wrapper struct for the fragment input:

@group(0) @binding(0)
var texture_array_top: binding_array<texture_2d<f32>>;
@group(0) @binding(1)
var texture_array_bottom: binding_array<texture_2d<f32>>;
@group(0) @binding(2)
var sampler_array: binding_array<sampler>;

@fragment
fn non_uniform_main(
    @location(0) tex_coord: vec2<f32>,
    @location(1) index: i32,
) -> @location(0) vec4<f32> {
    var outval: vec3<f32>;
    if tex_coord.y <= 0.5 {
        outval = textureSampleLevel(
            texture_array_top[index],
            sampler_array[index],
            tex_coord,
            0.0
        ).rgb;
    } else {
        outval = textureSampleLevel(
            texture_array_bottom[index],
            sampler_array[index],
            tex_coord,
            0.0
        ).rgb;
    }

    return vec4<f32>(outval.x, outval.y, outval.z, 1.0);
}

This will result in SPIRV code which does not have the expected non-uniform index marker in it. It looks like this in HLSL when de-compiled with SPIRV cross:

void frag_main()
{
    float3 outval = 0.0f.xxx;
    if (tex_coord.y <= 0.5f)
    {
        outval = texture_array_top[index].SampleLevel(sampler_array[index], tex_coord, 0.0f).xyz;
    }
    else
    {
        outval = texture_array_bottom[index].SampleLevel(sampler_array[index], tex_coord, 0.0f).xyz;
    }
    _29 = float4(outval.x, outval.y, outval.z, 1.0f);
}

Normally the indexes should be marked as non-uniform indexes like (using the original texture_arrays code):

void frag_main()
{
    float3 outval = 0.0f.xxx;
    FragmentInput _24 = { tex_coord, index };
    if (_24.tex_coord.y <= 0.5f)
    {
        outval = texture_array_top[NonUniformResourceIndex(_24.index)].SampleLevel(sampler_array[NonUniformResourceIndex(_24.index)], _24.tex_coord, 0.0f).xyz;
    }
    else
    {
        outval = texture_array_bottom[NonUniformResourceIndex(_24.index)].SampleLevel(sampler_array[NonUniformResourceIndex(_24.index)], _24.tex_coord, 0.0f).xyz;
    }
    _31 = float4(outval.x, outval.y, outval.z, 1.0f);
}

It is also possible to skip the wgpu part and directly convert both WGSL shader versions with the help of naga-cli:

naga non_uniform_indexing.wgsl non_uniform_indexing.hlsl

Expected vs observed behavior
Non-uniform indexes should properly marked as such in the resulting shader modules.

Extra materials
XInRsc6

Platform
WGPU stable (22.1) and WGPU trunk. Nvidia and Intel devices will accept the SPIRV modules and render them without any problem, but AMD devices on either Windows and Linux will render with artifacts (see screenshot).

@magcius
Copy link
Contributor

magcius commented Sep 15, 2024

The bug is that naga's analyzer considers flat interpolants (which integers are by default) to be uniform, which is just plain wrong. There is nothing that says that something flat across the triangle will be subgroup uniform.

// only flat inputs are uniform
Some(crate::Binding::Location {
interpolation: Some(crate::Interpolation::Flat),
..
}) => true,

@magcius magcius linked a pull request Sep 15, 2024 that will close this issue
6 tasks
magcius added a commit to magcius/wgpu that referenced this issue Sep 15, 2024
Implementations can absolutely pack multiple triangles per subgroup.

Fixes gfx-rs#6270.
magcius added a commit to magcius/wgpu that referenced this issue Sep 15, 2024
Implementations can absolutely pack multiple triangles per subgroup.

Fixes gfx-rs#6270.
@cwfitzgerald
Copy link
Member

That's a big oops!

@Wumpf Wumpf added type: bug Something isn't working naga Shader Translator labels Sep 15, 2024
@ErichDonGubler ErichDonGubler changed the title Non uniform indexes are not always properly handled by naga Non uniform indices are not always properly handled by naga Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
naga Shader Translator type: bug Something isn't working
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

4 participants