Address issues with GLSL style global in/out vars (#6669)

[sricker-nvidia]

Asserts and segfaults were observed trying to compile a simple vertex shader like:

in int2 inPos;

[shader("vertex")]
main(uniform int2 test1, int2 test2, out float4 pos: SV_Position) void main()
{
    // Bogus use of all input vars to prevent optimizing out.
    pos = float4(inPos.x, test1.x, test2.y, 0);
}

Further investigation found that while replacing "uniform int2 test1" with "int2 test1" allowed for successful compilation, the resulting output shader would have overlapping location qualifiers. For example, compiling the above with "int2 test1" to glsl might give:

...
layout(location = 0) in ivec2 test1_0;
layout(location = 1) in ivec2 test2_0;
layout(location = 0) in ivec2 translatedGlobalParams_inPos_0; ...

This was because Slang does not actually support mixing GLSL style global in/out vars and entry point params. However, this is never checked for or noted in documentation. Slang source also assumes input shaders do not mix these and these assumptions ultimately led to the observed asserts and seg faults when using uniform entry point params.

This change makes updates to throw an error when the compiler detects that it is trying to translate global in/out variables into entry point params when an entry point already contains parameters, allowing for compilation to fail gracefully. Change also makes minor updates to documentation to clarify allowed usage of GLSL-flavored global in/out variables.

[fairywreath]

It's better to use diagnostic sink here instead of SLANG_INVALID_OPERATION, you can add a new error to slang-diagnostics-defs.h

[sricker-nvidia]

Updated

[csyonghe]

I think this is likely too strict and will fail a lot of existing tests.

Can we just support this scenario instead of throwing an error?

[csyonghe]

At very least, we should only error when there are existing parameters that are varying in/out.

Existing uniform parameters, or builtin parameters are fine.

// should be allowed:
in float3 color;
void entry(float4 p: SV_Position, RWStructuredBuffer<float> b) {}

[sricker-nvidia]

I experimented with supporting mixing GLSL style global in/out vars and entry point params, but if we stick the global in/out vars into the entry point params, I think we need to recalculate all of the offsets for the existing entry point params and that seemed like it was getting a bit complicated. I can revisit that effort if it makes sense to spend time on it.

[sricker-nvidia]

The problem with allowing existing uniform parameters, or builtin parameters is that translateGlobalVaryingVar ends up replacing all of the entry point params before we try and collect entry point uniforms and make them global. The result is that collectEntryPointUniformParams ends up seg faulting because it gets a format for the entry point params that it doesn't expect after they have been transformed in translateGlobalVaryingVar. It might be possible to shift things around to avoid the seg fault, but it still would not solve the issue with the parameter offsets.

[sricker-nvidia]

I can use something like:

for (auto param : entryPointFunc->getParams())
{
    if (!param->findDecoration<IRSemanticDecoration>())
        // error

to allow something like

in float3 color;
void entry(float4 p: SV_Position, out float4 pos: SV_Position)

such that at least builtin parameters are fine, but working around uniform entry point parameters is more complicated to support.

[csyonghe]

OK, let’s just disallow all parameters then.

[sricker-nvidia]

Might make sense to update the label to "pr: breaking" as this does effect the tests/bugs/gh-5339.slang test without additional edits and could have an effect on existing code (although it might be surprising if that code worked as intended).

[jarcherNV]

Might make sense to update the label to "pr: breaking" as this does effect the tests/bugs/gh-5339.slang test without additional edits and could have an effect on existing code (although it might be surprising if that code worked as intended).

Would it be possible to fix those tests in your PR?

[sricker-nvidia]

Might make sense to update the label to "pr: breaking" as this does effect the tests/bugs/gh-5339.slang test without additional edits and could have an effect on existing code (although it might be surprising if that code worked as intended).

Would it be possible to fix those tests in your PR?

Yeah, I'm working on it now for the next patch, I just wanted to note things in case someone wiser than myself thought the non-breaking label was incorrect.

[csyonghe]

Yes, this will need to be labeled as breaking change.