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

[expenses]

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

[aleino-nv]

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]

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

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

[expenses]

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]

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

[expenses]

@aleino-nv this might help, to quote #6005:

per spec https://www.w3.org/TR/WGSL/#operator-precedence-associativity + and ^ are in different associativity groups, hence one needs parenthesis.

[aleino-nv]

But what's an 'associativity group'? Does it correspond to a box in that section? (That section is non-normative, btw.)
That can't be, because * and + are in different boxes, and combining these operations doesn't require parentheses.

In any case, I guess the important fact is that we've now seen several examples of various WGSL compilers requiring parentheses that are not really required by the implementation.

Slang should produce WGSL that's compatible with as many browsers (or standalone WGSL compilers) as possible, and versions thereof.
To avoid having to revisit this topic multiple times in the future, I'm thinking we should just opt-in to parentheses by default.
In cases were that leads to egregious readability issues (like * and +), we can opt out in order to improve readability.

[expenses]

Perhaps @sagudev or someone else on the naga/tint/wgsl spec teams could comment?

[sagudev]

For your concrete example it this is relevant part:

the following groups do not associate with other groups:
...

• Binary XOR (can associate with self and unary weakly).

I think the best place would be to ask on spec repo.

[aleino-nv]

For your concrete example it this is relevant part:

the following groups do not associate with other groups:
...

• Binary XOR (can associate with self and unary weakly).

I think the best place would be to ask on spec repo.

@sagudev Thanks, but I think I understand how it works. I was just thinking about how to approach a fix in a way that's robust and will last basically no matter what the WGSL compilers do. I have a proposed patch for several similar issues now, and some pretty exhaustive tests.

[expenses]

Almost there! I found one last remaining issue with operations on bool3s:

// Computes `dir > 0 ? 3.0 - pos : pos`
// with special case to avoid rounding.
static float3 GetMirroredPos(float3 pos, float3 dir, bool rangeCheck) {
    float3 mirrored = asfloat(asuint(pos) ^ 0x7FFFFF);
    // XOR'ing will only work if the coords are in range [1.0, 2.0),
    // so just fallback to the original subtraction if that's not the case.
    if (rangeCheck && any(pos < 1.0 | pos >= 2.0)) mirrored = 3.0 - pos;
    return select(dir > 0, mirrored, pos);
}

generates:
https://shader-playground.timjones.io/4eaf2add29ee66a8d9ed1b0fb9d56643

[aleino-nv]

There were some cases missing that were not caught by the tests I added because a < b | c got translated to u32(a < b) | c so the u32 conversion added in the missing parentheses.

After another fix I'm now seeing https://shader-playground.timjones.io/f4fe1f09bd5ccb623c3b9b3476aabedd