SSA rewriter: Don't use trivial phis (#2757)
When a phi candidate is marked as trivial, we are suppose to update all
of its uses to the reference the value that it is being folded to.
However, the code updates the uses misses `defs_at_block_`. So at a
later time, the id for the trivial phi can reemerge.
Fixes #2744
diff --git a/source/opt/ssa_rewrite_pass.cpp b/source/opt/ssa_rewrite_pass.cpp
index f2cb2da..7144ca0 100644
--- a/source/opt/ssa_rewrite_pass.cpp
+++ b/source/opt/ssa_rewrite_pass.cpp
@@ -102,6 +102,7 @@
uint32_t repl_id) {
for (uint32_t user_id : phi_to_remove.users()) {
PhiCandidate* user_phi = GetPhiCandidate(user_id);
+ BasicBlock* bb = pass_->context()->get_instr_block(user_id);
if (user_phi) {
// If the user is a Phi candidate, replace all arguments that refer to
// |phi_to_remove.result_id()| with |repl_id|.
@@ -110,6 +111,10 @@
arg = repl_id;
}
}
+ } else if (bb->id() == user_id) {
+ // The phi candidate is the definition of the variable at basic block
+ // |bb|. We must change this to the replacement.
+ WriteVariable(phi_to_remove.var_id(), bb, repl_id);
} else {
// For regular loads, traverse the |load_replacement_| table looking for
// instances of |phi_to_remove|.
@@ -259,6 +264,8 @@
// require a Phi instruction. This will act as |var_id|'s current
// definition to break potential cycles.
PhiCandidate& phi_candidate = CreatePhiCandidate(var_id, bb);
+
+ // Set the value for |bb| to avoid an infinite recursion.
WriteVariable(var_id, bb, phi_candidate.result_id());
val_id = AddPhiOperands(&phi_candidate);
}
diff --git a/source/opt/ssa_rewrite_pass.h b/source/opt/ssa_rewrite_pass.h
index c0373dc..fddbdaf 100644
--- a/source/opt/ssa_rewrite_pass.h
+++ b/source/opt/ssa_rewrite_pass.h
@@ -188,6 +188,9 @@
// value |val_id|.
void WriteVariable(uint32_t var_id, BasicBlock* bb, uint32_t val_id) {
defs_at_block_[bb][var_id] = val_id;
+ if (auto* pc = GetPhiCandidate(val_id)) {
+ pc->AddUser(bb->id());
+ }
}
// Processes the store operation |inst| in basic block |bb|. This extracts
diff --git a/test/opt/local_ssa_elim_test.cpp b/test/opt/local_ssa_elim_test.cpp
index d2da1f9..a490d52 100644
--- a/test/opt/local_ssa_elim_test.cpp
+++ b/test/opt/local_ssa_elim_test.cpp
@@ -1930,6 +1930,63 @@
SinglePassRunAndMatch<LocalMultiStoreElimPass>(text, false);
}
+TEST_F(LocalSSAElimTest, ChainedTrivialPhis) {
+ // Check that the copy object get the undef value implicitly assigned in the
+ // entry block.
+ const std::string text = R"(
+; CHECK: [[undef:%\w+]] = OpUndef %v4float
+; CHECK: OpCopyObject %v4float [[undef]]
+ OpCapability Shader
+ %1 = OpExtInstImport "GLSL.std.450"
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint GLCompute %2 "main"
+ OpExecutionMode %2 LocalSize 1 18 6
+ OpSource ESSL 310
+ %void = OpTypeVoid
+ %4 = OpTypeFunction %void
+ %bool = OpTypeBool
+ %float = OpTypeFloat 32
+ %v4float = OpTypeVector %float 4
+%_ptr_Function_v4float = OpTypePointer Function %v4float
+ %2 = OpFunction %void None %4
+ %9 = OpLabel
+ %10 = OpVariable %_ptr_Function_v4float Function
+ OpBranch %11
+ %11 = OpLabel
+ OpLoopMerge %12 %13 None
+ OpBranch %14
+ %14 = OpLabel
+ %15 = OpUndef %bool
+ OpBranchConditional %15 %16 %12
+ %16 = OpLabel
+ %17 = OpUndef %bool
+ OpSelectionMerge %18 None
+ OpBranchConditional %17 %19 %18
+ %19 = OpLabel
+ %20 = OpUndef %bool
+ OpLoopMerge %21 %22 None
+ OpBranchConditional %20 %23 %21
+ %23 = OpLabel
+ %24 = OpLoad %v4float %10
+ %25 = OpCopyObject %v4float %24
+ %26 = OpUndef %bool
+ OpBranch %22
+ %22 = OpLabel
+ OpBranch %19
+ %21 = OpLabel
+ OpBranch %12
+ %18 = OpLabel
+ OpBranch %13
+ %13 = OpLabel
+ OpBranch %11
+ %12 = OpLabel
+ %27 = OpLoad %v4float %10
+ OpReturn
+ OpFunctionEnd
+ )";
+ SinglePassRunAndMatch<SSARewritePass>(text, false);
+}
+
// TODO(greg-lunarg): Add tests to verify handling of these cases:
//
// No optimization in the presence of
