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

Index value issue in GLSL front nested loop #5246

Closed
patriciogonzalezvivo opened this issue Feb 12, 2024 · 4 comments · Fixed by #5311
Closed

Index value issue in GLSL front nested loop #5246

patriciogonzalezvivo opened this issue Feb 12, 2024 · 4 comments · Fixed by #5311
Labels
area: naga front-end lang: GLSL OpenGL Shading Language naga Shader Translator type: bug Something isn't working

Comments

@patriciogonzalezvivo
Copy link

patriciogonzalezvivo commented Feb 12, 2024

Description

I'm successfully converting a shader from GLSL to WGSL, with no errors.
The shader contain a nested loop and I notice there is an issue with the index value.

Repro steps

Where:

layout(set = 0, binding = 0) uniform sampler     linearSampler;
layout(set = 1, binding = 0) uniform texture2D   inputTex;

out vec4 fragColor;

float gaussian( vec2 d, float s) { return exp(-( d.x*d.x + d.y*d.y) / (2.0 * s*s)); }

void main() {
    int kernelSize = 16;
    vec2 res = vec2(1024.);
    vec2 pixel = 1.0/res;
    vec2 pos = gl_FragCoord.xy * pixel;

    float kernelSizef = float(kernelSize);
    float halfKernelSizef = kernelSizef * 0.5;

    vec4 accumColor = vec4(0.0);
    float accumWeight = 0.0;

    for (int x = 0; x < kernelSize; x++) {
        for (int y = 0; y < kernelSize; y++) {
            vec2 offset = vec2(x,y) - halfKernelSizef;
            float weight = gaussian(offset, kernelSizef);
            accumColor += weight * texture(sampler2D(inputTex, linearSampler), pos + offset * pixel * 10.0);
            accumWeight += weight;
        }
    }
    fragColor = accumColor / accumWeight;
}

Note: I'm exaggerating the kernel radius to make the results more obvious

The above code produce this results:

Untitled Project

And by changing the order of the loops to

...
    for (int y = 0; y < kernelSize; y++) {
        for (int x = 0; x < kernelSize; x++) {
...

I get:

Untitled Project (1)

Expected vs observed behavior

I notice that I can solve the issue by creating the index values outside the scope of the loops and that solve the problem:

...
    int x = 0;
    int y = 0;
    for ( x = 0; x < kernelSize; x++) {
        for ( y = 0; y < kernelSize; y++) {
...

Untitled Project (2)

Extra materials

Here is a conversion of the above GLSL code to WGLS

struct FragmentOutput {
    @location(0) fragColor: vec4<f32>,
}

@group(0) @binding(0) 
var linearSampler: sampler;
@group(1) @binding(0) 
var inputTex: texture_2d<f32>;
var<private> fragColor: vec4<f32>;
var<private> gl_FragCoord: vec4<f32>;

fn gaussian(d: vec2<f32>, s: f32) -> f32 {
    var d_1: vec2<f32>;
    var s_1: f32;

    d_1 = d;
    s_1 = s;
    let _e7 = d_1;
    let _e9 = d_1;
    let _e12 = d_1;
    let _e14 = d_1;
    let _e20 = s_1;
    let _e22 = s_1;
    let _e25 = d_1;
    let _e27 = d_1;
    let _e30 = d_1;
    let _e32 = d_1;
    let _e38 = s_1;
    let _e40 = s_1;
    return exp((-(((_e25.x * _e27.x) + (_e30.y * _e32.y))) / ((2f * _e38) * _e40)));
}

fn main_1() {
    var kernelSize: i32 = 16i;
    var res: vec2<f32> = vec2(1024f);
    var pixel: vec2<f32>;
    var pos: vec2<f32>;
    var kernelSizef: f32;
    var halfKernelSizef: f32;
    var accumColor: vec4<f32> = vec4(0f);
    var accumWeight: f32 = 0f;
    var x: i32 = 0i;
    var y: i32 = 0i;
    var offset: vec2<f32>;
    var weight: f32;

    let _e9 = res;
    pixel = (vec2(1f) / _e9);
    let _e14 = gl_FragCoord;
    let _e16 = pixel;
    pos = (_e14.xy * _e16);
    let _e19 = kernelSize;
    kernelSizef = f32(_e19);
    let _e22 = kernelSizef;
    halfKernelSizef = (_e22 * 0.5f);
    loop {
        let _e33 = x;
        let _e34 = kernelSize;
        if !((_e33 < _e34)) {
            break;
        }
        {
            loop {
                let _e42 = y;
                let _e43 = kernelSize;
                if !((_e42 < _e43)) {
                    break;
                }
                {
                    let _e49 = x;
                    let _e50 = y;
                    let _e54 = halfKernelSizef;
                    offset = (vec2<f32>(f32(_e49), f32(_e50)) - vec2(_e54));
                    let _e60 = offset;
                    let _e61 = kernelSizef;
                    let _e62 = gaussian(_e60, _e61);
                    weight = _e62;
                    let _e64 = accumColor;
                    let _e65 = weight;
                    let _e66 = pos;
                    let _e67 = offset;
                    let _e68 = pixel;
                    let _e73 = pos;
                    let _e74 = offset;
                    let _e75 = pixel;
                    let _e80 = textureSample(inputTex, linearSampler, (_e73 + ((_e74 * _e75) * 10f)));
                    accumColor = (_e64 + (_e65 * _e80));
                    let _e83 = accumWeight;
                    let _e84 = weight;
                    accumWeight = (_e83 + _e84);
                }
                continuing {
                    let _e46 = y;
                    y = (_e46 + 1i);
                }
            }
        }
        continuing {
            let _e37 = x;
            x = (_e37 + 1i);
        }
    }
    let _e86 = accumColor;
    let _e87 = accumWeight;
    fragColor = (_e86 / vec4(_e87));
    return;
}

@fragment 
fn main(@builtin(position) param: vec4<f32>) -> FragmentOutput {
    gl_FragCoord = param;
    main_1();
    let _e9 = fragColor;
    return FragmentOutput(_e9);
}

and id.txt using naga-cli

ir.txt

Platform
Information about your OS, version of wgpu, your tech stack, etc.

@Vipitis
Copy link

Vipitis commented Feb 18, 2024

I run into this as well, trying to build a minimal example to figure out what exactly is going wrong. One thing I noticed is that the index variable might be declared as an int, but ends up behaving like a float so could this be part of this issue?

@Vipitis
Copy link

Vipitis commented Feb 19, 2024

I built and illustrative example on Shadertoy

porting this to wgpu via our wgpu-shadertoy implementation you get the following behaviour:

working by declaring the inner index outside the loop: inner loop gets run 18 times:
red is 18/25

broken when declared inside the index, the inner loop runs 3 times. But only on the first loop of the outer loop.
red is 3/25

So my hypothesis seems to be: If you declare the inner index variable inside the inner loop: it only runs the inner on the first loop out the outer loop.

hope this helps.

@teoxoy teoxoy added the type: bug Something isn't working label Feb 27, 2024
@teoxoy
Copy link
Member

teoxoy commented Feb 27, 2024

I think the issue is here:

self.parse_declaration(frontend, ctx, false, false)?;

the last arg should be is_inside_loop.

Would you be open to start a PR for this?

@cwfitzgerald
Copy link
Member

Thanks for the hint, PR submitted!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: naga front-end lang: GLSL OpenGL Shading Language naga Shader Translator type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants