Incorrect SPIR-V can be generated when using samplers in structs with relaxed rules

[slime73]

If a sampler is in a struct and relaxed rules (-R) are used when generating SPIR-V code, it can have incorrect output.

Here's an example GLSL shader:

#version 450 core

in vec2 VarVertexCoord;

struct MyLightStruct {
    sampler2DShadow shadowMap;
};

uniform sampler2DShadow otherShadowMap;
uniform MyLightStruct[2] myLight;
out vec4 FragColor;

void main() {
    float accumulatedShadow = 0.0;

    for (int i = 0; i < 2; i++) {
        accumulatedShadow += texture(myLight[i].shadowMap, vec3(VarVertexCoord, 0.5));
    }

    accumulatedShadow += texture(otherShadowMap, vec3(VarVertexCoord, 0.5));

    FragColor = vec4(vec3(accumulatedShadow), 1.0);
}

If I run glslang -V -R --aml --amb myfile.frag I get this if I disassemble the result. It uses an extra myLighti_shadowMap uniform with no binding, which seems like it shouldn't happen.

bad SPIR-V disassembly

; SPIR-V
; Version: 1.0
; Generator: Khronos Glslang Reference Front End; 11
; Bound: 73
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main" %VarVertexCoord %FragColor
               OpExecutionMode %main OriginUpperLeft
               OpSource GLSL 450
               OpName %main "main"
               OpName %accumulatedShadow "accumulatedShadow"
               OpName %i "i"
               OpName %myLighti_shadowMap "myLighti.shadowMap"
               OpName %VarVertexCoord "VarVertexCoord"
               OpName %otherShadowMap "otherShadowMap"
               OpName %FragColor "FragColor"
               OpName %myLight_0__shadowMap "myLight[0].shadowMap"
               OpName %myLight_1__shadowMap "myLight[1].shadowMap"
               OpName %MyLightStruct "MyLightStruct"
               OpMemberName %MyLightStruct 0 "/*shadowMap*/"
               OpName %gl_DefaultUniformBlock "gl_DefaultUniformBlock"
               OpMemberName %gl_DefaultUniformBlock 0 "myLight"
               OpName %_ ""
               OpDecorate %VarVertexCoord Location 0
               OpDecorate %otherShadowMap DescriptorSet 0
               OpDecorate %otherShadowMap Binding 0
               OpDecorate %FragColor Location 0
               OpDecorate %myLight_0__shadowMap DescriptorSet 0
               OpDecorate %myLight_0__shadowMap Binding 0
               OpDecorate %myLight_1__shadowMap DescriptorSet 0
               OpDecorate %myLight_1__shadowMap Binding 0
               OpMemberDecorate %MyLightStruct 0 Offset 0
               OpDecorate %_arr_MyLightStruct_uint_2 ArrayStride 16
               OpMemberDecorate %gl_DefaultUniformBlock 0 Offset 0
               OpDecorate %gl_DefaultUniformBlock Block
               OpDecorate %_ DescriptorSet 0
               OpDecorate %_ Binding 0
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
      %float = OpTypeFloat 32
%_ptr_Function_float = OpTypePointer Function %float
    %float_0 = OpConstant %float 0
        %int = OpTypeInt 32 1
%_ptr_Function_int = OpTypePointer Function %int
      %int_0 = OpConstant %int 0
      %int_2 = OpConstant %int 2
       %bool = OpTypeBool
         %23 = OpTypeImage %float 2D 1 0 0 1 Unknown
         %24 = OpTypeSampledImage %23
%_ptr_UniformConstant_24 = OpTypePointer UniformConstant %24
%myLighti_shadowMap = OpVariable %_ptr_UniformConstant_24 UniformConstant
    %v2float = OpTypeVector %float 2
%_ptr_Input_v2float = OpTypePointer Input %v2float
%VarVertexCoord = OpVariable %_ptr_Input_v2float Input
  %float_0_5 = OpConstant %float 0.5
    %v3float = OpTypeVector %float 3
      %int_1 = OpConstant %int 1
%otherShadowMap = OpVariable %_ptr_UniformConstant_24 UniformConstant
    %v4float = OpTypeVector %float 4
%_ptr_Output_v4float = OpTypePointer Output %v4float
  %FragColor = OpVariable %_ptr_Output_v4float Output
    %float_1 = OpConstant %float 1
%myLight_0__shadowMap = OpVariable %_ptr_UniformConstant_24 UniformConstant
%myLight_1__shadowMap = OpVariable %_ptr_UniformConstant_24 UniformConstant
%MyLightStruct = OpTypeStruct %int
       %uint = OpTypeInt 32 0
     %uint_2 = OpConstant %uint 2
%_arr_MyLightStruct_uint_2 = OpTypeArray %MyLightStruct %uint_2
%gl_DefaultUniformBlock = OpTypeStruct %_arr_MyLightStruct_uint_2
%_ptr_Uniform_gl_DefaultUniformBlock = OpTypePointer Uniform %gl_DefaultUniformBlock
          %_ = OpVariable %_ptr_Uniform_gl_DefaultUniformBlock Uniform
       %main = OpFunction %void None %3
          %5 = OpLabel
%accumulatedShadow = OpVariable %_ptr_Function_float Function
          %i = OpVariable %_ptr_Function_int Function
               OpStore %accumulatedShadow %float_0
               OpStore %i %int_0
               OpBranch %14
         %14 = OpLabel
               OpLoopMerge %16 %17 None
               OpBranch %18
         %18 = OpLabel
         %19 = OpLoad %int %i
         %22 = OpSLessThan %bool %19 %int_2
               OpBranchConditional %22 %15 %16
         %15 = OpLabel
         %27 = OpLoad %24 %myLighti_shadowMap
         %31 = OpLoad %v2float %VarVertexCoord
         %34 = OpCompositeExtract %float %31 0
         %35 = OpCompositeExtract %float %31 1
         %36 = OpCompositeConstruct %v3float %34 %35 %float_0_5
         %37 = OpCompositeExtract %float %36 2
         %38 = OpImageSampleDrefImplicitLod %float %27 %36 %37
         %39 = OpLoad %float %accumulatedShadow
         %40 = OpFAdd %float %39 %38
               OpStore %accumulatedShadow %40
               OpBranch %17
         %17 = OpLabel
         %41 = OpLoad %int %i
         %43 = OpIAdd %int %41 %int_1
               OpStore %i %43
               OpBranch %14
         %16 = OpLabel
         %45 = OpLoad %24 %otherShadowMap
         %46 = OpLoad %v2float %VarVertexCoord
         %47 = OpCompositeExtract %float %46 0
         %48 = OpCompositeExtract %float %46 1
         %49 = OpCompositeConstruct %v3float %47 %48 %float_0_5
         %50 = OpCompositeExtract %float %49 2
         %51 = OpImageSampleDrefImplicitLod %float %45 %49 %50
         %52 = OpLoad %float %accumulatedShadow
         %53 = OpFAdd %float %52 %51
               OpStore %accumulatedShadow %53
         %57 = OpLoad %float %accumulatedShadow
         %58 = OpCompositeConstruct %v3float %57 %57 %57
         %60 = OpCompositeExtract %float %58 0
         %61 = OpCompositeExtract %float %58 1
         %62 = OpCompositeExtract %float %58 2
         %63 = OpCompositeConstruct %v4float %60 %61 %62 %float_1
               OpStore %FragColor %63
               OpReturn
               OpFunctionEnd

If I then run SPIRV-Cross to generate a Metal shader from the SPIR-V via spirv-cross --msl --msl-version 20100 frag.spv, this is the result which also seems broken - it's accessing a single myLighti_shadowMap uniform in the loop instead of accessing something different in each iteration.

bad MSL code

#include <metal_stdlib>
#include <simd/simd.h>

using namespace metal;

struct MyLightStruct
{
    int _shadowMap_;
};

struct _RESERVED_IDENTIFIER_FIXUP_gl_DefaultUniformBlock
{
    MyLightStruct myLight[2];
};

struct main0_out
{
    float4 FragColor [[color(0)]];
};

struct main0_in
{
    float2 VarVertexCoord [[user(locn0)]];
};

fragment main0_out main0(main0_in in [[stage_in]], depth2d<float> myLighti_shadowMap [[texture(0)]], depth2d<float> otherShadowMap [[texture(1)]], sampler myLighti_shadowMapSmplr [[sampler(0)]], sampler otherShadowMapSmplr [[sampler(1)]])
{
    main0_out out = {};
    float accumulatedShadow = 0.0;
    for (int i = 0; i < 2; i++)
    {
        float3 _36 = float3(in.VarVertexCoord, 0.5);
        accumulatedShadow += myLighti_shadowMap.sample_compare(myLighti_shadowMapSmplr, _36.xy, _36.z);
    }
    float3 _49 = float3(in.VarVertexCoord, 0.5);
    accumulatedShadow += otherShadowMap.sample_compare(otherShadowMapSmplr, _49.xy, _49.z);
    out.FragColor = float4(float3(accumulatedShadow), 1.0);
    return out;
}

FYI @sbourasse

Tested with the latest glslang release (14.2.0).

[sbourasse]

Thanks for another report, I'll look into this.

[sbourasse]

Turns out to be harder than expected.

Basically we know how to remap variables from arrays when they are indexed with an integer literal, because it is easy to append the index to the variable name, create a top level binding for it, and call it a day.
When indexing an array with a variable, this trick doesn't work anymore since appending the variable name won't properly route to a top-level variable.

The only option I see to keep this behaviour transparent is to collapse array stacks entirely for opaque types. This is a bit more involved than I would like, and I'm not sure it is actually achievable.

Is it something that used to work but doesn't anymore ?

[slime73]

It's something that OpenGL GLSL supports, so my users have written shaders that use it which won't work on other backends.

I don't really like that it exists though, I could detect it and put in a warning if it's totally necessary but I'm hoping to avoid that to allow any OpenGL GLSL shader to work on Vulkan, Metal, etc. with my tool.

[sbourasse]

I think I managed to make something work, but it might change the way opaque uniforms locations are found. Considering I don't know how you handle them, would you mind giving it a try, see if it works for you ?
Here is a link to the branch on my fork : https://github.com/sbourasse/glslang/tree/bourasse/array-collapse