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

spirv-opt: Fix OpCompositeExtract relaxation with struct operands #5536

Merged

Conversation

bejado
Copy link
Contributor

@bejado bejado commented Jan 18, 2024

When running the ConvertToHalf pass, an OpCompositeExtract instruction with a OpTypeStruct operand should not be demoted to half. This was (partially) fixed in #5201. However, that PR did not address a couple of scenarios:

  1. The OpCompositeExtract instruction is explicitly decorated with RelaxedPrecision.
               OpDecorate %9 RelaxedPrecision
    %v4float = OpTypeVector %float 4
  %_struct_6 = OpTypeStruct %v4float
          %7 = OpUndef %_struct_6
               ...
          %9 = OpCompositeExtract %float %7 0 3
               OpReturn
  1. The OpCompositeExtract instruction's usages are also relaxed (or it has no uses), so it is automatically decorated with RelaxedPrecision. See this block in convert_to_half_pass.cpp.
    %v4float = OpTypeVector %float 4
  %_struct_6 = OpTypeStruct %v4float
          %7 = OpUndef %_struct_6
               ...
          %9 = OpCompositeExtract %float %7 0 3
; %9 is not used, or its uses are also relaxed
               OpReturn

Otherwise, spirv-opt ends up generating SPIR-V which does not pass validation:

error: line 15: Result type (OpTypeFloat) does not match the type that results from indexing into the composite (OpTypeFloat).
  %9 = OpCompositeExtract %half %7 0 3

This PR is an attempt to address these issues by more strictly disallowing an OpCompositeExtract instruction to be demoted to half precision if it has a OpTypeStruct operand.

@CLAassistant
Copy link

CLAassistant commented Jan 18, 2024

CLA assistant check
All committers have signed the CLA.

@greg-lunarg
Copy link
Contributor

Please change reviewer to @jeremy-lunarg

@s-perron s-perron requested review from jeremy-lunarg and removed request for ncesario-lunarg January 26, 2024 21:11
Copy link
Contributor

@jeremy-lunarg jeremy-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jeremy-lunarg jeremy-lunarg merged commit 8d3ee2e into KhronosGroup:main Feb 1, 2024
24 checks passed
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.

6 participants