Detect and deduplicate system semantic input/output.

[wijiler]

using this fragment shader

static const int scaleFactor = 10;

struct Light
{
    float3 position;
    float4 color;
    float radius;
}
///
struct Transform
{
    float3 position;
    float2 scale;
    float rotation;
}
static const float4 vpositions[6] =
    {
        float4(1.0f, 1.0f, 0.0f, 1.0f),
        float4(1.0f, -1.0f, 0.0f, 1.0f),
        float4(-1.0f, -1.0f, 0.0f, 1.0f),
        float4(-1.0f, -1.0f, 0.0f, 1.0f),
        float4(-1.0f, 1.0f, 0.0f, 1.0f),
        float4(1.0f, 1.0f, 0.0f, 1.0f),
    };

float3 rot(float3 p, float3 o, float a)
{
    return float3((p.x - o.x) * cos(a) - (p.y - o.y) * sin(a) + o.x, (p.x - o.x) * sin(a) + (p.y - o.y) * cos(a) + o.y, p.z);
}

float3 scale(float3 p, float3 o, float sf)
{
    return float3(o.x + (p.x / sf), o.y + (p.y / sf), p.z);
}

float3 transform(Transform vert, uint32_t id)
{
    float3 rPos = (float3(vpositions[id].xyz) + vert.position) / scaleFactor;
    rPos = rot(rPos, vert.position, vert.rotation);
    rPos.x *= vert.scale.x;
    rPos.y *= vert.scale.y;

    return rPos;
}
///
struct pushConstants
{
    uint lightCount;
    Light *lights;
    Transform *instances;
}

struct VertOutput
{
    float4 pos : SV_Position;
}

[vk_binding(0, 0)]
Texture2D<float4> gBuffer[];

static const Texture2D<float4> albedo = gBuffer[0];

[vk_push_constant]
pushConstants pc;
[shader("vertex")]
VertOutput vertMain(uint vertID: SV_VertexID, uint instID: SV_InstanceID)
{
    VertOutput output;
    output.pos = float4(transform(pc.instances[instID], vertID), 1.0f);
    return output;
}

[shader("fragment")]
float4 fragMain(VertOutput input, uint2 fragPos: SV_Position)
    : SV_Target
{
    float4 color = albedo[fragPos];
    float3 diffuse = float3(0, 0, 0);
    for (uint i = 0; i < pc.lightCount; i++)
    {
        Light light = pc.lights[i];
        float3 ambient = light.color.xyz * 0.1f;

        float3 lightDir = light.position - input.pos.xyz;
        float atten = 1.0 / dot(lightDir, lightDir);
        float3 lightColor = light.color.xyz * (light.color.w * atten);
        diffuse = lightColor;
    }
    return float4(diffuse, 1.0f) * color;
}

We are met with these fun validation errors

VUID-VkShaderCreateInfoEXT-pCode-08737(ERROR / SPEC): msgNum: -1600984977 - Validation Error: [ VUID-VkShaderCreateInfoEXT-pCode-08737 ] | MessageID = 0xa092e86f | vkCreateShadersEXT(): pCreateInfos[0].pCode (spirv-val produced an error):
Non-unique OpEntryPoint interface '150[%150]' is disallowed
  %gl_FragCoord = OpVariable %_ptr_Input_v4float Input
. The Vulkan spec states: If codeType is VK_SHADER_CODE_TYPE_SPIRV_EXT, pCode must adhere to the validation rules described by the Validation Rules within a Module section of the SPIR-V Environment appendix (https://vulkan.lunarg.com/doc/view/1.3.290.0/windows/1.3-extensions/vkspec.html#VUID-VkShaderCreateInfoEXT-pCode-08737)
    Objects: 0
VUID-VkShaderCreateInfoEXT-pCode-08737(ERROR / SPEC): msgNum: -1600984977 - Validation Error: [ VUID-VkShaderCreateInfoEXT-pCode-08737 ] | MessageID = 0xa092e86f | vkCreateShadersEXT(): pCreateInfos[0].pCode (spirv-val produced an error):
Non-unique OpEntryPoint interface '150[%150]' is disallowed
  %gl_FragCoord = OpVariable %_ptr_Input_v4float Input
. The Vulkan spec states: If codeType is VK_SHADER_CODE_TYPE_SPIRV_EXT, pCode must adhere to the validation rules described by the Validation Rules within a Module section of the SPIR-V Environment appendix (https://vulkan.lunarg.com/doc/view/1.3.290.0/windows/1.3-extensions/vkspec.html#VUID-VkShaderCreateInfoEXT-pCode-08737)
    Objects: 0

But if we just return color or just return diffuse, there is no validation errors, hence why this confuses me, on latest release btw

[wijiler]

https://github.com/gfx-rs/naga/issues/2036

[csyonghe]

The problem is that SV_Position is declared twice in the fragment shader parameter list, once in VertOutput::pos and once in fragMain::fragPos, and Slang doesn't current deduplicate these things. A workaround is to remove one declaration.

[wijiler]

The problem is that SV_Position is declared twice in the fragment shader parameter list, once in VertOutput::pos and once in fragMain::fragPos, and Slang doesn't current deduplicate these things. A workaround is to remove one declaration.

Ah, it might be nice to have some warning for this, thank you for the information

[wijiler]

@csyonghe It might be nice to have some syntax like

struct VertOutput
{
    float4 pos : SV_Position, REAL_POS;
}

so that if I need to pass the vertex position into the fragment shader I dont have to do this

struct VertOutput
{
    float4 pos : SV_Position;
    float3 realPos : REAL_POS;
}

although im not sure its really that big of an issue

[pdjonov]

I hit this validation error with SV_ViewID because I linked together two different modules that bound to it (one a using private file-scope variable, the other in shader params). I worked around it, but modules being incompatible (even though the shader compile succeeds) because of something that's not even in the modules' public interface is not great.

[vblanco20-1]

Just wanted to add here, that this error makes gl_FragPos impossible to use. On a normal shader you have the SV_Position on vertex struct declaration, and then you have the fragPos on the fragment shader so its going to conflict no matter what. If you remove the SV_Position declaration on vertex struct when compiling fragment shader, its going to mismatch and error the linking.

[aleino-nv]

The problem is that SV_Position is declared twice in the fragment shader parameter list, once in VertOutput::pos and once in fragMain::fragPos, and Slang doesn't current deduplicate these things. A workaround is to remove one declaration.

@csyonghe The title suggests to me that the fix to Slang would be to automatically remove one of the SV_Position semantics. Is that the intended fix?

That seems strange to me. How would Slang know which one to remove? This seems more like it should be an error.

[csyonghe]

@vblanco20-1 I don't understand your comment. Can you elaborate why you think it is impossible to use gl_FragCoord? I don't see why it is not possible.

[csyonghe]

@aleino-nv I think the right way out is to error if the semantic is declared twice.

[pdjonov]

Can duplicate SV_-bound variables not be merged (at least in contexts where they are readonly)? It's really useful to bind to things like SV_InstanceID as a private implementation detail in a module. But if two modules do that, it'd be nice for it to just work rather than to have an invisible "you can't import these two things together" hazard that could only be fixed by putting the module's implementation details in its public API (forcing all users of the module, including those that wouldn't otherwise hit the hazard, to do extra work binding and passing through the SV_ value).

[csyonghe]

We don’t recommend any modules to declare global variables with SV_ semantics attached on them. This feature is really only supposed to be used from the builtin core module and not user code. The system values should be declared on the entry point function and passed down to other libraries/functions.

[pdjonov]

That's really unfortunate. Using modules to hide the rendering engine's interface details from higher-level material code (and switching the import paths to compile for different rendering backends) seemed like a really good way to make the code much cleaner and get rid of a lot of "copy-paste these declarations in every per-material, per-stage entry point" boilerplate.

So, I guess I'll ask, is this the "it'll probably work but we're not gonna spend time helping you if it doesn't" kind of "we don't recommend" or the "we're planning to break such code ourselves" kind? Asking because being limited to the interface module being a monolith is a lot less bad than loads of boilerplate.

[csyonghe]

We don’t plan to break any existing support for global variable declarations, and we likely will want to implement some kind of deduplication logic to make this better. However, global builtins are not the only source of problems when moving towards a fully modular approach.

In the future we may be supporting compiling a slang module directly to dynamically linkable machine code to reduce compile time. In that world any form of global variables will be a huge problem, and any code that is written today using global variables in any form will likely not benefit from dynamic linking when we are able support it. For this reason we don’t recommend global variables if it can be avoided.

[pdjonov]

Oh, that's fine then. If it isn't going to blow up in some future slang release then I'll stick with what I'm doing (and when you get around to deduping, I can break the monolithic module up a bit). Way better than trying to figure out some other way (like macros or something...?) to avoid touching every material shader file any time I want a different SV_ bindings for something. If I ever need slang code compiled to a dynamic lib then that's a totally different environment with different constraints and it'll need rethinking anyway.

Thanks for the quick reply!

[csyonghe]

Another solution is to declare all the builtins in a shared module and import it from whatever place that needs them. This way you won’t have any duplicates.

import will take care of deduplication. A symbol is never defined twice no matter how many times the module is imported.

[vblanco20-1]

@csyonghe With the glsl module, using gl_FragCoord uses the SV_Position intrinsic automatically (see https://github.com/shader-slang/slang/blob/master/source/slang/glsl.meta.slang#L99 ), And you need to use SV_Position from your vertex shader output. This is what conflicts. This little dummy shader here will replicate the issue.

import glsl; //defines gl_fragcoord using SV_Position

struct VertexStageOutput
{ 
    float4 position : SV_Position;  //second usage of SV_Position
};

[shader("vertex")]
VertexStageOutput vertexMain(    
    uint vertexID: SV_VertexID)
{
    VertexStageOutput output;

    output.position = vec4(1, 0, 0, 0);

    return output;
};

[shader("fragment")]
float4 pxMain(
    VertexStageOutput verts) {
    
    return float4(gl_FragCoord.xy,0,0); //errors
};

[csyonghe]

@vblanco20-1 in your example, you can just use verts.position in the fragment shader, right?

The glsl module is intended for supporting glsl input, and we are not testing for when mixing glsl and HLSL.

[vblanco20-1]

@vblanco20-1 in your example, you can just use verts.position in the fragment shader, right?

The glsl module is intended for supporting glsl input, and we are not testing for when mixing glsl and HLSL.

gl_FragCoord is for accessing what pixel coordinate is being executed by the pixel shader, its applying viewport transform from the normal position stuff of a vertex.

[csyonghe]

I believe that verts.position from fragment shader is exactly that: coordinate in viewport space.

[csyonghe]

I think the original issue is being addressed in the discussion.

For @vblanco20-1's issue, I don't believe there is any actual conflict, since gl_FragCoord is exactly equivalent to verts.position in the example code, according to the spec of HLSL and GLSL. Closing this issue since there is no further actionable item.