Fix handling of OpPhi in convert-relaxed-to-half (#4618)

Fixes #4452

source/opt/convert_to_half_pass.cpp

@@ -177,18 +177,21 @@ bool ConvertToHalfPass::GenHalfArith(Instruction* inst) {
   return modified;
 }
 
-bool ConvertToHalfPass::ProcessPhi(Instruction* inst) {
+bool ConvertToHalfPass::ProcessPhi(Instruction* inst, uint32_t from_width,
-  // Add float16 converts of any float32 operands and change type
+                                   uint32_t to_width) {
-  // of phi to float16 equivalent. Operand converts need to be added to
+  // Add converts of any float operands to to_width if they are of from_width.
-  // preceeding blocks.
+  // If converting to 16, change type of phi to float16 equivalent and remember
+  // result id. Converts need to be added to preceeding blocks.
   uint32_t ocnt = 0;
   uint32_t* prev_idp;
-  inst->ForEachInId([&ocnt, &prev_idp, this](uint32_t* idp) {
+  bool modified = false;
+  inst->ForEachInId([&ocnt, &prev_idp, &from_width, &to_width, &modified,
+                     this](uint32_t* idp) {
     if (ocnt % 2 == 0) {
       prev_idp = idp;
     } else {
       Instruction* val_inst = get_def_use_mgr()->GetDef(*prev_idp);
-      if (IsFloat(val_inst, 32)) {
+      if (IsFloat(val_inst, from_width)) {
         BasicBlock* bp = context()->get_instr_block(*idp);
         auto insert_before = bp->tail();
         if (insert_before != bp->begin()) {
@@ -197,15 +200,19 @@ bool ConvertToHalfPass::ProcessPhi(Instruction* inst) {
               insert_before->opcode() != SpvOpLoopMerge)
             ++insert_before;
         }
-        GenConvert(prev_idp, 16, &*insert_before);
+        GenConvert(prev_idp, to_width, &*insert_before);
+        modified = true;
       }
     }
     ++ocnt;
   });
-  inst->SetResultType(EquivFloatTypeId(inst->type_id(), 16));
+  if (to_width == 16u) {
-  get_def_use_mgr()->AnalyzeInstUse(inst);
+    inst->SetResultType(EquivFloatTypeId(inst->type_id(), 16u));
-  converted_ids_.insert(inst->result_id());
+    converted_ids_.insert(inst->result_id());
-  return true;
+    modified = true;
+  }
+  if (modified) get_def_use_mgr()->AnalyzeInstUse(inst);
+  return modified;
 }
 
 bool ConvertToHalfPass::ProcessConvert(Instruction* inst) {
@@ -242,9 +249,10 @@ bool ConvertToHalfPass::ProcessImageRef(Instruction* inst) {
 }
 
 bool ConvertToHalfPass::ProcessDefault(Instruction* inst) {
-  bool modified = false;
   // If non-relaxed instruction has changed operands, need to convert
   // them back to float32
+  if (inst->opcode() == SpvOpPhi) return ProcessPhi(inst, 16u, 32u);
+  bool modified = false;
   inst->ForEachInId([&inst, &modified, this](uint32_t* idp) {
     if (converted_ids_.count(*idp) == 0) return;
     uint32_t old_id = *idp;
@@ -262,7 +270,7 @@ bool ConvertToHalfPass::GenHalfInst(Instruction* inst) {
   if (IsArithmetic(inst) && inst_relaxed)
     modified = GenHalfArith(inst);
   else if (inst->opcode() == SpvOpPhi && inst_relaxed)
-    modified = ProcessPhi(inst);
+    modified = ProcessPhi(inst, 32u, 16u);
   else if (inst->opcode() == SpvOpFConvert)
     modified = ProcessConvert(inst);
   else if (image_ops_.count(inst->opcode()) != 0)

source/opt/convert_to_half_pass.h

@@ -93,7 +93,7 @@ class ConvertToHalfPass : public Pass {
   bool GenHalfArith(Instruction* inst);
 
   // Gen code for relaxed phi |inst|
-  bool ProcessPhi(Instruction* inst);
+  bool ProcessPhi(Instruction* inst, uint32_t from_width, uint32_t to_width);
 
   // Gen code for relaxed convert |inst|
   bool ProcessConvert(Instruction* inst);

test/opt/convert_relaxed_to_half_test.cpp

@@ -1489,6 +1489,87 @@ TEST_F(ConvertToHalfTest, RemoveRelaxDec) {
   EXPECT_EQ(Pass::Status::SuccessWithChange, std::get<1>(result));
 }
 
+TEST_F(ConvertToHalfTest, HandleNonRelaxedPhi) {
+  // See https://github.com/KhronosGroup/SPIRV-Tools/issues/4452
+
+  // 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"(
+; CHECK: [[fcvt:%\w+]] = OpFConvert %v3float {{%\w+}}
+; CHECK-NEXT: OpSelectionMerge {{%\w+}} None
+; CHECK: {{%\w+}} = OpPhi %v3float [[fcvt]] {{%\w+}} {{%\w+}} {{%\w+}}
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main" %output_color
+               OpExecutionMode %main OriginUpperLeft
+               OpSource GLSL 450
+               OpName %main "main"
+               OpName %MaterialParams "MaterialParams"
+               OpMemberName %MaterialParams 0 "foo"
+               OpName %materialParams "materialParams"
+               OpName %output_color "output_color"
+               OpMemberDecorate %MaterialParams 0 Offset 0
+               OpDecorate %MaterialParams Block
+               OpDecorate %materialParams DescriptorSet 0
+               OpDecorate %materialParams Binding 5
+               OpDecorate %output_color Location 0
+               OpDecorate %57 RelaxedPrecision
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+      %float = OpTypeFloat 32
+    %v3float = OpTypeVector %float 3
+%MaterialParams = OpTypeStruct %float
+%_ptr_Uniform_MaterialParams = OpTypePointer Uniform %MaterialParams
+%materialParams = OpVariable %_ptr_Uniform_MaterialParams Uniform
+        %int = OpTypeInt 32 1
+      %int_0 = OpConstant %int 0
+%_ptr_Uniform_float = OpTypePointer Uniform %float
+    %float_0 = OpConstant %float 0
+       %bool = OpTypeBool
+    %v4float = OpTypeVector %float 4
+%_ptr_Output_v4float = OpTypePointer Output %v4float
+%output_color = OpVariable %_ptr_Output_v4float Output
+       %uint = OpTypeInt 32 0
+     %uint_0 = OpConstant %uint 0
+%_ptr_Output_float = OpTypePointer Output %float
+     %uint_1 = OpConstant %uint 1
+     %uint_2 = OpConstant %uint 2
+  %float_0_5 = OpConstant %float 0.5
+         %61 = OpConstantComposite %v3float %float_0_5 %float_0_5 %float_0_5
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+         %55 = OpAccessChain %_ptr_Uniform_float %materialParams %int_0
+         %56 = OpLoad %float %55
+         %57 = OpCompositeConstruct %v3float %56 %56 %56
+         %31 = OpFOrdGreaterThan %bool %56 %float_0
+               OpSelectionMerge %33 None
+               OpBranchConditional %31 %32 %33
+         %32 = OpLabel
+         %37 = OpFMul %v3float %57 %61
+               OpBranch %33
+         %33 = OpLabel
+         %58 = OpPhi %v3float %57 %5 %37 %32
+         %45 = OpAccessChain %_ptr_Output_float %output_color %uint_0
+         %46 = OpCompositeExtract %float %58 0
+               OpStore %45 %46
+         %48 = OpAccessChain %_ptr_Output_float %output_color %uint_1
+         %49 = OpCompositeExtract %float %58 1
+               OpStore %48 %49
+         %51 = OpAccessChain %_ptr_Output_float %output_color %uint_2
+         %52 = OpCompositeExtract %float %58 2
+               OpStore %51 %52
+               OpReturn
+               OpFunctionEnd
+)";
+
+  SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  auto result = SinglePassRunAndMatch<ConvertToHalfPass>(test, true);
+  EXPECT_EQ(Pass::Status::SuccessWithChange, std::get<1>(result));
+}
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools