Repository: KhronosGroup/SPIRV-Tools (language: python)

Issue:
Don't convert struct members to half
An initial (and possibly overzealous) attempt at fixing #4814 

This currently just stops conversion to half when it's reading from a struct. This could lead to cases that could be optimised being missed - that would require checking the members of the struct to determine whether they're compatible with half. 


Discussion / hints:
[![CLA assistant check](https://cla-assistant.io/pull/badge/signed)](https://cla-assistant.io/KhronosGroup/SPIRV-Tools?pullRequest=5201) <br/>All committers have signed the CLA.
Why should struct members not be converted to half?
Currently the pass says that anything which produces a float and doesn't consume an explicit F32 value can be safely demoted to half (https://github.com/KhronosGroup/SPIRV-Tools/blob/6110f30a36775e4d452968407f12b16694df2cc8/source/opt/convert_to_half_pass.cpp#L297) . The way members of structs are accessed is through an OpCompositeExtract, which produces a float, and has operands which are OpTypeStruct and and integer (for indexing into the struct). This means you can end up with code which looks roughly like this (pseudocodeish)

%100 = struct {float a; float b;}
%some_half_variable = OpCompositeExtract %half %100 1
<op that uses %some_half_variable as an f16>

Which causes an error when you try to validate the code, because you're trying to put an f32 value into an f16 variable.

The more complicated, and probably most correct, fix is to insert conversions when you're accessing a potentially f32 member of a struct. Or to not do the demotion in that case. This also potentially affects other container-like types too.

This is intended as a lower-effort, more-correct-than-now, not as optimal intermediate fix so it won't do something explicitly wrong, but it does leave some performance on the table if the struct members weren't f32.


@cjo20 Can you a new test? This will provide an example that we can use to better understand the problem. It will also fill a hole in our testing, and help avoid regressions in the future. Here is a sample test that you can use to get started:

https://github.com/KhronosGroup/SPIRV-Tools/blob/01055c60cfc0cddd2724dc674ed73d09df992360/test/opt/convert_relaxed_to_half_test.cpp#L1426
Sorry for the delay.

Thank you for the explanation. This appears to be a bug. A convert should have been inserted.

If you could provide code as requested by @s-perron, that would be helpful. It will help us evaluate your fix.

I've spent some time trying to get a test working and I've not had much luck. I've tried to create a small case that reproduces the problem - the following shader in HLSL:

```
struct myStruct
{
	float f;
};

ConstantBuffer<myStruct> MyConstants : register(b2, space1);

half ExtractVal(myStruct a)
{
	return a.f;
}

float PSMain() : SV_TARGET
{
	return ExtractVal(MyConstants);
}
```

Which converts to the following SPIRV:
```
               OpCapability Shader
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %PSMain "PSMain" %out_var_SV_TARGET %MyConstants
               OpExecutionMode %PSMain OriginUpperLeft
               OpSource HLSL 600
               OpName %type_ConstantBuffer_myStruct "type.ConstantBuffer.myStruct"
               OpMemberName %type_ConstantBuffer_myStruct 0 "f"
               OpName %MyConstants "MyConstants"
               OpName %out_var_SV_TARGET "out.var.SV_TARGET"
               OpName %PSMain "PSMain"
               OpDecorate %out_var_SV_TARGET Location 0
               OpDecorate %MyConstants DescriptorSet 1
               OpDecorate %MyConstants Binding 2
               OpMemberDecorate %type_ConstantBuffer_myStruct 0 Offset 0
               OpDecorate %type_ConstantBuffer_myStruct Block
      %float = OpTypeFloat 32
%type_ConstantBuffer_myStruct = OpTypeStruct %float
%_ptr_Uniform_type_ConstantBuffer_myStruct = OpTypePointer Uniform %type_ConstantBuffer_myStruct
%_ptr_Output_float = OpTypePointer Output %float
       %void = OpTypeVoid
          %9 = OpTypeFunction %void
%MyConstants = OpVariable %_ptr_Uniform_type_ConstantBuffer_myStruct Uniform
%out_var_SV_TARGET = OpVariable %_ptr_Output_float Output
     %PSMain = OpFunction %void None %9
         %10 = OpLabel
         %11 = OpLoad %type_ConstantBuffer_myStruct %MyConstants
         %12 = OpCompositeExtract %float %11 0
               OpStore %out_var_SV_TARGET %12
               OpReturn
               OpFunctionEnd
```

Running with `--convert-relaxed-to-half` does reproduce the issue, as 
```        
        %12 = OpCompositeExtract %float %11 0
```
becomes
```
        %12 = OpCompositeExtract %half %11 0
        %14 = OpFConvert %float %12
```
Which then fails spirv-val with the message 
```
error: line 29: Result type (OpTypeFloat) does not match the type that results from indexing into the composite (OpTypeFloat).
  %12 = OpCompositeExtract %half %11 0
```

Running with my suggested change results in the OpCompositeExtract line not changing, and therefore not failing validation.

I've not used googletest before, and I've not got time to look in to generating the test itself right now.

I added the test. @cjo20 I should have pointed you to a different example because your test is suppose to be a "nopt".

Fix patch:
diff --git a/source/opt/convert_to_half_pass.cpp b/source/opt/convert_to_half_pass.cpp
index cb0065d2d3..e243bedf0c 100644
--- a/source/opt/convert_to_half_pass.cpp
+++ b/source/opt/convert_to_half_pass.cpp
@@ -171,6 +171,19 @@ bool ConvertToHalfPass::RemoveRelaxedDecoration(uint32_t id) {
 
 bool ConvertToHalfPass::GenHalfArith(Instruction* inst) {
   bool modified = false;
+  // If this is a OpCompositeExtract instruction and has a struct operand, we
+  // should not relax this instruction. Doing so could cause a mismatch between
+  // the result type and the struct member type.
+  bool hasStructOperand = false;
+  if (inst->opcode() == spv::Op::OpCompositeExtract) {
+    inst->ForEachInId([&hasStructOperand, this](uint32_t* idp) {
+      Instruction* op_inst = get_def_use_mgr()->GetDef(*idp);
+      if (IsStruct(op_inst)) hasStructOperand = true;
+    });
+    if (hasStructOperand) {
+      return false;
+    }
+  }
   // Convert all float32 based operands to float16 equivalent and change
   // instruction type to float16 equivalent.
   inst->ForEachInId([&inst, &modified, this](uint32_t* idp) {
@@ -303,12 +316,19 @@ bool ConvertToHalfPass::CloseRelaxInst(Instruction* inst) {
   if (closure_ops_.count(inst->opcode()) == 0) return false;
   // Can relax if all float operands are relaxed
   bool relax = true;
-  inst->ForEachInId([&relax, this](uint32_t* idp) {
+  bool hasStructOperand = false;
+  inst->ForEachInId([&relax, &hasStructOperand, this](uint32_t* idp) {
     Instruction* op_inst = get_def_use_mgr()->GetDef(*idp);
-    if (IsStruct(op_inst)) relax = false;
+    if (IsStruct(op_inst)) hasStructOperand = true;
     if (!IsFloat(op_inst, 32)) return;
     if (!IsRelaxed(*idp)) relax = false;
   });
+  // If the instruction has a struct operand, we should not relax it, even if
+  // all its uses are relaxed. Doing so could cause a mismatch between the
+  // result type and the struct member type.
+  if (hasStructOperand) {
+    return false;
+  }
   if (relax) {
     AddRelaxed(inst->result_id());
     return true;


Test patch:
diff --git a/test/opt/convert_relaxed_to_half_test.cpp b/test/opt/convert_relaxed_to_half_test.cpp
index 62b9ae4535..c5774045c3 100644
--- a/test/opt/convert_relaxed_to_half_test.cpp
+++ b/test/opt/convert_relaxed_to_half_test.cpp
@@ -1713,6 +1713,75 @@ TEST_F(ConvertToHalfTest, PreserveImageOperandPrecision) {
   SinglePassRunAndMatch<ConvertToHalfPass>(test, true);
 }
 
+TEST_F(ConvertToHalfTest, DontRelaxDecoratedOpCompositeExtract) {
+  // This test checks that a OpCompositeExtract with a Struct operand won't be
+  // relaxed, even if it is explicitly decorated with RelaxedPrecision.
+  const std::string test =
+      R"(OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %1 "main"
+OpExecutionMode %1 OriginUpperLeft
+OpDecorate %9 RelaxedPrecision
+%void = OpTypeVoid
+%3 = OpTypeFunction %void
+%float = OpTypeFloat 32
+%v4float = OpTypeVector %float 4
+%_struct_6 = OpTypeStruct %v4float
+%7 = OpUndef %_struct_6
+%1 = OpFunction %void None %3
+%8 = OpLabel
+%9 = OpCompositeExtract %float %7 0 3
+OpReturn
+OpFunctionEnd
+)";
+
+  const std::string expected =
+      R"(OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %1 "main"
+OpExecutionMode %1 OriginUpperLeft
+%void = OpTypeVoid
+%3 = OpTypeFunction %void
+%float = OpTypeFloat 32
+%v4float = OpTypeVector %float 4
+%_struct_6 = OpTypeStruct %v4float
+%7 = OpUndef %_struct_6
+%1 = OpFunction %void None %3
+%8 = OpLabel
+%9 = OpCompositeExtract %float %7 0 3
+OpReturn
+OpFunctionEnd
+)";
+
+  SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  SinglePassRunAndCheck<ConvertToHalfPass>(test, expected, true);
+}
+
+TEST_F(ConvertToHalfTest, DontRelaxOpCompositeExtract) {
+  // This test checks that a OpCompositeExtract with a Struct operand won't be
+  // relaxed, even if its result has no uses.
+  const std::string test =
+      R"(OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %1 "main"
+OpExecutionMode %1 OriginUpperLeft
+%void = OpTypeVoid
+%3 = OpTypeFunction %void
+%float = OpTypeFloat 32
+%v4float = OpTypeVector %float 4
+%_struct_6 = OpTypeStruct %v4float
+%7 = OpUndef %_struct_6
+%1 = OpFunction %void None %3
+%8 = OpLabel
+%9 = OpCompositeExtract %float %7 0 3
+OpReturn
+OpFunctionEnd
+)";
+
+  SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  SinglePassRunAndCheck<ConvertToHalfPass>(test, test, true);
+}
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools