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

Issue:
spirv-opt: Invalid SPIR-V generated when optimizing for performance without merge-return
Optimizing for performance without merge-return, then running `--convert-relaxed-to-half` passes generates invalid SPIR-V. When merge-return is enabled, everything compiles correctly.

```
error: line 59: Result type (OpTypeVector) does not match the type that results from indexing into the composite (OpTypeVector).
  %181 = OpCompositeExtract %v3half %81 0
```

To reproduce:

[repro.zip](https://github.com/KhronosGroup/SPIRV-Tools/files/8856565/repro.zip)

```
# SPIRV-Tools v2022.3-dev v2022.2-28-gc9450135
# Glslang Version: 10:11.10.0

# Compile.
glslangValidator -V -o unoptimized.spv test.frag

# Optimize for performance (without merge-return).
spirv-opt -Oconfig=optimizations.cfg unoptimized.spv -o optimized.spv

# Check that it's good so far.
spirv-val optimized.spv

# Convert relaxed to half.
spirv-opt \
    --convert-relaxed-to-half \
    --simplify-instructions \
    --redundancy-elimination \
    --eliminate-dead-code-aggressive \
    optimized.spv \
    -o half.spv

# Validate the SPIR-V (this fails)
# error: line 59: Result type (OpTypeVector) does not match the type that results from indexing into the composite (OpTypeVector).
#   %181 = OpCompositeExtract %v3half %81 0
spirv-val half.spv
```



Discussion / hints:
@greg-lunarg Looks like something going on in a pass that you wrote?  Can you take a look at it or find someone else to look into it?
I believe this issue occurs here: https://github.com/KhronosGroup/SPIRV-Tools/blob/main/source/opt/convert_to_half_pass.cpp#L297

It looks like it checks all sources to float operations and, regardless of whether the operation is decorated as relaxed or not, says it's ok to make it relaxed if none of the sources are f32. I think this fails in the case of extracting items from a struct, as the source OpType is OpTypeStruct (so the check passes). The actual item accessed by OpCompositeExtract is one of the members of the struct though, which can be f32. This can lead to mismatched types.

If demoting all float instructions that don't have an f32 source is a correct thing to do (I'm not sure it is), then the check potentially needs to check whether members of structs are non-relaxable-f32.
@greg-lunarg Another `convert-relaxed-to-half` issue.

Fix patch:
diff --git a/source/opt/convert_to_half_pass.cpp b/source/opt/convert_to_half_pass.cpp
index 7a4c1f4097..2c4a631e17 100644
--- a/source/opt/convert_to_half_pass.cpp
+++ b/source/opt/convert_to_half_pass.cpp
@@ -39,6 +39,13 @@ bool ConvertToHalfPass::IsFloat(Instruction* inst, uint32_t width) {
   return Pass::IsFloat(ty_id, width);
 }
 
+bool ConvertToHalfPass::IsStruct(Instruction* inst) {
+  uint32_t ty_id = inst->type_id();
+  if (ty_id == 0) return false;
+  Instruction* ty_inst = Pass::GetBaseType(ty_id);
+  return (ty_inst->opcode() == spv::Op::OpTypeStruct);
+}
+
 bool ConvertToHalfPass::IsDecoratedRelaxed(Instruction* inst) {
   uint32_t r_id = inst->result_id();
   for (auto r_inst : get_decoration_mgr()->GetDecorationsFor(r_id, false))
@@ -294,6 +301,7 @@ bool ConvertToHalfPass::CloseRelaxInst(Instruction* inst) {
   bool relax = true;
   inst->ForEachInId([&relax, this](uint32_t* idp) {
     Instruction* op_inst = get_def_use_mgr()->GetDef(*idp);
+    if (IsStruct(op_inst)) relax = false;
     if (!IsFloat(op_inst, 32)) return;
     if (!IsRelaxed(*idp)) relax = false;
   });
diff --git a/source/opt/convert_to_half_pass.h b/source/opt/convert_to_half_pass.h
index feabfba3e1..24a478ffc6 100644
--- a/source/opt/convert_to_half_pass.h
+++ b/source/opt/convert_to_half_pass.h
@@ -45,6 +45,7 @@ class ConvertToHalfPass : public Pass {
   // Return true if |inst| returns scalar, vector or matrix type with base
   // float and |width|
   bool IsFloat(Instruction* inst, uint32_t width);
+  bool IsStruct(Instruction* inst);
 
   // Return true if |inst| is decorated with RelaxedPrecision
   bool IsDecoratedRelaxed(Instruction* inst);


Test patch:
diff --git a/test/opt/convert_relaxed_to_half_test.cpp b/test/opt/convert_relaxed_to_half_test.cpp
index 6a06de84f7..27330e109a 100644
--- a/test/opt/convert_relaxed_to_half_test.cpp
+++ b/test/opt/convert_relaxed_to_half_test.cpp
@@ -1570,6 +1570,49 @@ TEST_F(ConvertToHalfTest, HandleNonRelaxedPhi) {
   EXPECT_EQ(Pass::Status::SuccessWithChange, std::get<1>(result));
 }
 
+TEST_F(ConvertToHalfTest, DoNotReplaceStructMember) {
+  // See https://github.com/KhronosGroup/SPIRV-Tools/issues/4814
+
+  // This test is a case with a non-relaxed phi with a relaxed operand.
+  // A convert must be inserted at the end of the block associated with
+  // the operand.
+  const std::string test =
+      R"(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
+)";
+
+  SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  SinglePassRunAndCheck<ConvertToHalfPass>(test, test, true);
+}
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools