Fixes #2338. Added functionality to remove OpPhi instructions (replacing their uses) when merging blocks (#2339)
* Fixes #2338. Added check for phi node before merging blocks.
* Added functionality to merge blocks A and B even when B starts with OpPhi instructions, by replacing uses of the OpPhi results with the definitions coming from A. Added some tests for this.
* Fixed assertion.
diff --git a/source/opt/block_merge_util.cpp b/source/opt/block_merge_util.cpp
index 0a46ae6..aa97910 100644
--- a/source/opt/block_merge_util.cpp
+++ b/source/opt/block_merge_util.cpp
@@ -49,6 +49,20 @@
return IsMerge(context, block->id());
}
+// Removes any OpPhi instructions in |block|, which should have exactly one
+// predecessor, replacing uses of OpPhi ids with the ids associated with the
+// predecessor.
+void EliminateOpPhiInstructions(IRContext* context, BasicBlock* block) {
+ block->ForEachPhiInst([context, block](Instruction* phi) {
+ assert(2 == phi->NumInOperands() &&
+ "A block can only have one predecessor for block merging to make "
+ "sense.");
+ context->ReplaceAllUsesWith(phi->result_id(),
+ phi->GetSingleWordInOperand(0));
+ context->KillInst(phi);
+ });
+}
+
} // Anonymous namespace
bool CanMergeWithSuccessor(IRContext* context, BasicBlock* block) {
@@ -124,6 +138,8 @@
context->set_instr_block(&inst, &*bi);
}
+ EliminateOpPhiInstructions(context, &*sbi);
+
// Now actually move the instructions.
bi->AddInstructions(&*sbi);
diff --git a/test/opt/block_merge_test.cpp b/test/opt/block_merge_test.cpp
index 654e880..64ee952 100644
--- a/test/opt/block_merge_test.cpp
+++ b/test/opt/block_merge_test.cpp
@@ -741,6 +741,145 @@
SinglePassRunAndMatch<BlockMergePass>(text, true);
}
+TEST_F(BlockMergeTest, OpPhiInSuccessor) {
+ // Checks that when merging blocks A and B, the OpPhi at the start of B is
+ // removed and uses of its definition are replaced appropriately.
+ const std::string prefix =
+ R"(OpCapability Shader
+%1 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %main "main"
+OpExecutionMode %main OriginUpperLeft
+OpSource ESSL 310
+OpName %main "main"
+OpName %x "x"
+OpName %y "y"
+%void = OpTypeVoid
+%6 = OpTypeFunction %void
+%int = OpTypeInt 32 1
+%_ptr_Function_int = OpTypePointer Function %int
+%int_1 = OpConstant %int 1
+%main = OpFunction %void None %6
+%10 = OpLabel
+%x = OpVariable %_ptr_Function_int Function
+%y = OpVariable %_ptr_Function_int Function
+OpStore %x %int_1
+%11 = OpLoad %int %x
+)";
+
+ const std::string suffix_before =
+ R"(OpBranch %12
+%12 = OpLabel
+%13 = OpPhi %int %11 %10
+OpStore %y %13
+OpReturn
+OpFunctionEnd
+)";
+
+ const std::string suffix_after =
+ R"(OpStore %y %11
+OpReturn
+OpFunctionEnd
+)";
+ SinglePassRunAndCheck<BlockMergePass>(prefix + suffix_before,
+ prefix + suffix_after, true, true);
+}
+
+TEST_F(BlockMergeTest, MultipleOpPhisInSuccessor) {
+ // Checks that when merging blocks A and B, the OpPhis at the start of B are
+ // removed and uses of their definitions are replaced appropriately.
+ const std::string prefix =
+ R"(OpCapability Shader
+%1 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %main "main"
+OpExecutionMode %main OriginUpperLeft
+OpSource ESSL 310
+OpName %main "main"
+OpName %S "S"
+OpMemberName %S 0 "x"
+OpMemberName %S 1 "f"
+OpName %s "s"
+OpName %g "g"
+OpName %y "y"
+OpName %t "t"
+OpName %z "z"
+%void = OpTypeVoid
+%10 = OpTypeFunction %void
+%int = OpTypeInt 32 1
+%float = OpTypeFloat 32
+%S = OpTypeStruct %int %float
+%_ptr_Function_S = OpTypePointer Function %S
+%int_1 = OpConstant %int 1
+%float_2 = OpConstant %float 2
+%16 = OpConstantComposite %S %int_1 %float_2
+%_ptr_Function_float = OpTypePointer Function %float
+%_ptr_Function_int = OpTypePointer Function %int
+%int_3 = OpConstant %int 3
+%int_0 = OpConstant %int 0
+%main = OpFunction %void None %10
+%21 = OpLabel
+%s = OpVariable %_ptr_Function_S Function
+%g = OpVariable %_ptr_Function_float Function
+%y = OpVariable %_ptr_Function_int Function
+%t = OpVariable %_ptr_Function_S Function
+%z = OpVariable %_ptr_Function_float Function
+OpStore %s %16
+OpStore %g %float_2
+OpStore %y %int_3
+%22 = OpLoad %S %s
+OpStore %t %22
+%23 = OpAccessChain %_ptr_Function_float %s %int_1
+%24 = OpLoad %float %23
+%25 = OpLoad %float %g
+)";
+
+ const std::string suffix_before =
+ R"(OpBranch %26
+%26 = OpLabel
+%27 = OpPhi %float %24 %21
+%28 = OpPhi %float %25 %21
+%29 = OpFAdd %float %27 %28
+%30 = OpAccessChain %_ptr_Function_int %s %int_0
+%31 = OpLoad %int %30
+OpBranch %32
+%32 = OpLabel
+%33 = OpPhi %float %29 %26
+%34 = OpPhi %int %31 %26
+%35 = OpConvertSToF %float %34
+OpBranch %36
+%36 = OpLabel
+%37 = OpPhi %float %35 %32
+%38 = OpFSub %float %33 %37
+%39 = OpLoad %int %y
+OpBranch %40
+%40 = OpLabel
+%41 = OpPhi %float %38 %36
+%42 = OpPhi %int %39 %36
+%43 = OpConvertSToF %float %42
+%44 = OpFAdd %float %41 %43
+OpStore %z %44
+OpReturn
+OpFunctionEnd
+)";
+
+ const std::string suffix_after =
+ R"(%29 = OpFAdd %float %24 %25
+%30 = OpAccessChain %_ptr_Function_int %s %int_0
+%31 = OpLoad %int %30
+%35 = OpConvertSToF %float %31
+%38 = OpFSub %float %29 %35
+%39 = OpLoad %int %y
+%43 = OpConvertSToF %float %39
+%44 = OpFAdd %float %38 %43
+OpStore %z %44
+OpReturn
+OpFunctionEnd
+)";
+ SinglePassRunAndCheck<BlockMergePass>(prefix + suffix_before,
+ prefix + suffix_after, true, true);
+}
+
// TODO(greg-lunarg): Add tests to verify handling of these cases:
//
// More complex control flow
