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.

Sign up for GitHub

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

Jump to bottom

Incorrect SPIRV codegen for 8bit/16bit variables in buffers #3607

Open
alelenv opened this issue May 21, 2024 · 6 comments · May be fixed by #3631
Open

Incorrect SPIRV codegen for 8bit/16bit variables in buffers #3607

alelenv opened this issue May 21, 2024 · 6 comments · May be fixed by #3631
Labels
bug sev:miscompile Given a valid input, glslang produces incorrect or invalid SPIR-V SPIR-V

Comments

@alelenv
Copy link
Contributor

alelenv commented May 21, 2024

For the following simple 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);
}

We see some weird codegen when loading members from UBO and doing a convert

         %19 = OpAccessChain %_ptr_Uniform_v4short %_ %int_0
         %20 = OpLoad %v4short %19
         %22 = OpCompositeExtract %int %20 0
         %23 = OpCompositeExtract %int %20 1
         %24 = OpCompositeExtract %int %20 2
         %25 = OpCompositeExtract %int %20 3
         %26 = OpCompositeConstruct %v4int %22 %23 %24 %25
         %27 = OpSConvert %v4char %26

OpCompositeExtract tried to extract as 'int' when %20 is a vector of i16/shorts
@arcady-lunarg arcady-lunarg added bug sev:miscompile Given a valid input, glslang produces incorrect or invalid SPIR-V SPIR-V labels May 21, 2024
@arcady-lunarg
Copy link
Contributor

Yeah that definitely isn't valid SPIR-V, I'll take a look. Interestingly this seems to be the one cast that isn't currently covered by our unit tests.

@ravi688
Copy link
Contributor

ravi688 commented Jun 16, 2024

@alelenv, how did you compiled your shader?
Trying to execute the following command reports an error for me:

$ glslang -G test.frag -o test.frag.spv
test.frag
ERROR: test.frag:20: 'location' : SPIR-V requires location for user input/output 
ERROR: 1 compilation errors.  No code generated.


ERROR: Linking fragment stage: Missing entry point: Each stage requires one entry point

SPIR-V is not generated for failed compile or link

The test.frag contains the shader you embedded into your first messaged in this issue.

@arcady-lunarg
Copy link
Contributor

@ravi688 You can use the --aml option to automatically assign locations when you get that error message.

@arcady-lunarg arcady-lunarg mentioned this issue Jun 17, 2024
Merged
@ravi688
Copy link
Contributor

ravi688 commented Jun 22, 2024 •
edited
Loading

Inspecting the code in ParseHelper.cpp, I think we are first trying to extract the components of i16v4 (i16vec4) and then convert each component, of type i16, separately to scalar type of i8, after that construct the target vector type of i8vec4 from the final scalar components of type i8.

Why can't we just use the following set of instructions?

%x = OpSConvert from i16vec4 to ivec4
OpSConvert from %x to i8vec4

I believe OpSConvert works on vector types also.

Also, the following (vulkan) GLSL code:

#version 450

layout(location = 0) in vec4 in_color;

layout(location = 0) out vec4 out_color;

void main()
{
	out_color = vec4(in_color);
}

translates to the following SPIR-V instructions:

...
 %12 = OpLoad %v4float %in_color
         %13 = OpCompositeExtract %float %12 0
         %14 = OpCompositeExtract %float %12 1
         %15 = OpCompositeExtract %float %12 2
         %16 = OpCompositeExtract %float %12 3
         %17 = OpCompositeConstruct %v4float %13 %14 %15 %16
 OpStore %out_color %17
...

Why are we using OpCompositeExtract for the case when number of components are the same for source and target both and no mixing of scalar or differently sized vectors is there? we could have directly used %12, i.e. ignored the identity conversion.

Is there anything which I'm not aware of just in case it is all right extracting and constructing again?

@ravi688 ravi688 mentioned this issue Jun 22, 2024
Merged
@arcady-lunarg
Copy link
Contributor

@ravi688 take a look at #3628 which does exactly what you suggest.

ravi688 added a commit to ravi688/glslang that referenced this issue Jun 23, 2024
@ravi688
Convert 8/16-bit int (and their composite vector) types to their corr…
fc075a1 
…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 ravi688 linked a pull request Jun 23, 2024 that will close this issue
Open
@ravi688
Copy link
Contributor

ravi688 commented Jun 23, 2024

Raise a PR #3631 which fixes this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug sev:miscompile Given a valid input, glslang produces incorrect or invalid SPIR-V SPIR-V
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix for #3607 ravi688/glslang
3 participants
@alelenv @ravi688 @arcady-lunarg