KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.9k stars 815 forks source link

Fix for #3607 #3631

Open ravi688 opened 1 week ago

ravi688 commented 1 week ago

Convert 8/16-bit int (and their composite vector) types to their corresponding 32-bit types first and then convert the resulting 32-bit type to the target 8/16-bit type.

This change emits appropriate Op{S|U}Convert instructions instead of OpCompositeExtract followed by OpCompositeConstruct for 8/16-bit integer types.

What types of shaders are affected?

The following GLSL shader:

#version 460

#extension GL_EXT_shader_8bit_storage : require
#extension GL_EXT_shader_16bit_storage : require
#extension GL_EXT_shader_explicit_arithmetic_types_float16 : require

layout(binding = 1 ) uniform _16bit_storage
{
        i16vec4 i16v4;
};

// This is read back and checked on the CPU side to verify the converions
layout(binding = 2 ) writeonly buffer ConversionOutBuffer
{
        i8vec4 i16v4_to_i8v4;
} cob;

out vec4 fcolor;

void main()
{
        // Conversions
        {
                cob.i16v4_to_i8v4   = i8vec4(i16v4);
        }

        bool RED = true;
        bool GREEN = false;

        fcolor = vec4( (RED) ? 1.0f : 0.0f,
                                   (GREEN) ? 1.0f : 0.0f,
                                   0.0f, 1.0f);
}

Now compiles to the SPIR-V (i.e. with this patch applied):

...
    %19 = OpAccessChain %_ptr_Uniform_v4short %_ %int_0
    %20 = OpLoad %v4short %19
    %22 = OpSConvert %v4int %20
    %23 = OpSConvert %v4char %22
    %25 = OpAccessChain %_ptr_Uniform_v4char %cob %int_0
    OpStore %25 %23
...

Earlier it used to be compiled to the following SPIR-V instructions (i.e. without patch applied):

...
    %19 = OpAccessChain %_ptr_Uniform_v4short %_ %int_0
    %20 = OpLoad %v4short %19
    %22 = OpCompositeExtract %int %20 0 <-- incorrect instruction
    %23 = OpCompositeExtract %int %20 1 <-- incorrect instruction
    %24 = OpCompositeExtract %int %20 2 <-- incorrect instruction
    %25 = OpCompositeExtract %int %20 3 <-- incorrect instruction
    %26 = OpCompositeConstruct %v4int %22 %23 %24 %25
    %27 = OpSConvert %v4char %26
    %29 = OpAccessChain %_ptr_Uniform_v4char %cob %int_0
    OpStore %29 %27
...
ravi688 commented 1 week ago

@arcady-lunarg , could you please add the unit tests on your side again? I've ran out of my time this weekend. I see the tests are failing and I think it is expected for spv.8bit-16bit-construction.frag. You may inspect the SPIR-V with my patch applied for this shader and correct the reference SPIR-V file used for testing.

ravi688 commented 3 days ago

I think the following set of SPIR-V instructions are inefficient as what more the SPVKHR{16|8}bit_storage extensions allow.

...
    %19 = OpAccessChain %_ptr_Uniform_v4short %_ %int_0
    %20 = OpLoad %v4short %19
    %22 = OpSConvert %v4int %20
    %23 = OpSConvert %v4char %22
    %25 = OpAccessChain %_ptr_Uniform_v4char %cob %int_0
    OpStore %25 %23
...

Direct conversion from short to char is possible and allowed in SPVKHR{16|8}bit_storage extensions. However, it is not allowed in the corresponding GLSL extensions.

Any comments appreciated...