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

wgsl output doesn't use parenthesis for mixed operations, generating invalid code #6005

Open
expenses opened this issue Jan 5, 2025 · 4 comments · Fixed by #6030
Open

wgsl output doesn't use parenthesis for mixed operations, generating invalid code #6005

expenses opened this issue Jan 5, 2025 · 4 comments · Fixed by #6030
Assignees

Comments

@expenses
Copy link

expenses commented Jan 5, 2025

See gfx-rs/wgpu#6857

I got this error in Chromium

Compilation log for [Invalid ShaderModule (unlabeled)]:
6 error(s) generated while compiling the shader:
:67:54 error: mixing '+' and '^' requires parenthesis
        var _S4 : u32 = _S3 + (((((_S2 << (u32(4)))) + u32(2738958700) ^ (_S2 + sum_1)) ^ (((_S2 >> (u32(5)))) + u32(3355524772))));

slang code: https://github.com/NVIDIAGameWorks/Falcor/blob/9dc819c162b2070335c65060436041690b7937f8/Source/Falcor/Utils/Math/HashUtils.slang#L62-L74

@expenses expenses changed the title wgsk output doesn't use parenthesis for mixed operations, generating invalid code wgsl output doesn't use parenthesis for mixed operations, generating invalid code Jan 5, 2025
@aleino-nv aleino-nv self-assigned this Jan 7, 2025
@aleino-nv
Copy link
Collaborator

I have a fix ready for this, but I'll spend some more time tomorrow creating more exhaustive tests for this type of issue.

aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 8, 2025
Add parentheses for a few cases that Dawn/Tint (WGSL compiler) complains about.
Closes shader-slang#6005.
@aleino-nv
Copy link
Collaborator

With the above fix, the following WGSL code is now generated for the mentioned blockCipherTEA function:

https://shader-playground.timjones.io/a6fbc241c06921e0e3b71c5dfd1ad03d

@expenses
Copy link
Author

expenses commented Jan 9, 2025

Hey @aleino-nv, great work! != still needs parensthesis though. For example this function generates invalid code: https://shader-playground.timjones.io/e873e27794407bee99368aa55b342abd

bool isValidHemisphereReflection(const ShadingData sd, const ShadingFrame sf, const float3 wiLocal, const float3 woLocal, const float3 wo)
{
    // Check that wi/wo are in the upper hemisphere around the shading normal.
    if (min(wiLocal.z, woLocal.z) < kMinCosTheta) return false;

    // Check that wi/wo are on the same geometric side.
    bool wiTop = sd.frontFacing; // The flag is computed dot(wi, faceN) >= 0.f.
    bool woTop = dot(wo, sd.faceN) >= 0.f;
    if (wiTop != woTop) return false;

#if FALCOR_BACKFACE_BLACK
    // Additionally check that we're on the same geometric side as the shading normal.
    bool shadingTop = dot(sf.N, sd.faceN) >= 0.f;
    if (wiTop != shadingTop) return false;
#endif

    return true;
}

https://github.com/NVIDIAGameWorks/Falcor/blob/eb540f6748774680ce0039aaf3ac9279266ec521/Source/Falcor/Scene/Material/ShadingUtils.slang#L155-L172

Same with >= and &:

localhost/:1 Error while parsing WGSL: :652:26 error: mixing '>=' and '&' requires parenthesis
    if(any((ray_1.mDir_0 >= _S52 & (ray_1.mOrigin_0 >= vec3<f32>((f32(_S54) - 0.5f))))))
                         ^^^^^^^^^

@aleino-nv
Copy link
Collaborator

@expenses Thanks for reporting this! I will re-open this issue and look at some more cases.

@aleino-nv aleino-nv reopened this Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants