From c9190a54da56e425d09e122368b18bcfe00dabf6 Mon Sep 17 00:00:00 2001
From: Steven Perron <stevenperron@google.com>
Date: Tue, 23 Jul 2019 17:59:30 -0400
Subject: [PATCH] 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
---
 source/opt/ssa_rewrite_pass.cpp  |  7 ++++
 source/opt/ssa_rewrite_pass.h    |  3 ++
 test/opt/local_ssa_elim_test.cpp | 57 ++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/source/opt/ssa_rewrite_pass.cpp b/source/opt/ssa_rewrite_pass.cpp
index f2cb2da28..7144ca0ca 100644
--- a/source/opt/ssa_rewrite_pass.cpp
+++ b/source/opt/ssa_rewrite_pass.cpp
@@ -102,6 +102,7 @@ void SSARewriter::ReplacePhiUsersWith(const PhiCandidate& phi_to_remove,
                                       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 @@ void SSARewriter::ReplacePhiUsersWith(const PhiCandidate& phi_to_remove,
           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 @@ uint32_t SSARewriter::GetReachingDef(uint32_t var_id, BasicBlock* bb) {
     // 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 c0373dc06..fddbdaf54 100644
--- a/source/opt/ssa_rewrite_pass.h
+++ b/source/opt/ssa_rewrite_pass.h
@@ -188,6 +188,9 @@ class SSARewriter {
   // 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 d2da1f9c1..a490d5290 100644
--- a/test/opt/local_ssa_elim_test.cpp
+++ b/test/opt/local_ssa_elim_test.cpp
@@ -1930,6 +1930,63 @@ TEST_F(LocalSSAElimTest, VariablePointerTest2) {
   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