Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for #3607 #3631

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix for #3607 #3631

wants to merge 1 commit into from

Conversation

ravi688
Copy link
Contributor

@ravi688 ravi688 commented Jun 23, 2024

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
...

…esponding 32-bit types first

 - this fixes KhronosGroup#3607
 - and this also fixes assertion failure in the PR KhronosGroup#3628

 - this change emits appropriate Op{S|U}Convert instructions instead of OpCompositeExtract followed by OpCompositeConstruct
   for 8/16-bit integer types.
@ravi688
Copy link
Contributor Author

ravi688 commented Jun 23, 2024

@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
Copy link
Contributor Author

ravi688 commented Jun 29, 2024

I think the following set of SPIR-V instructions are inefficient as what more the SPV_KHR_{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 SPV_KHR_{16|8}bit_storage extensions. However, it is not allowed in the corresponding GLSL extensions.

Any comments appreciated...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect SPIRV codegen for 8bit/16bit variables in buffers
1 participant