Fix missing and incorrect DebugValues (#4929)

Specificially, fixes DebugValues coming out of
eliminate-local-single-store and eliminate-local-multi-store AKA SSA
rewrite.

5 files changed, 37 insertions, 14 deletions

diff --git a/source/opt/debug_info_manager.cpp b/source/opt/debug_info_manager.cpp
index f0a78a5ff..98d98d4ff 100644
--- a/source/opt/debug_info_manager.cpp
+++ b/source/opt/debug_info_manager.cpp
@@ -564,8 +564,8 @@ bool DebugInfoManager::IsDeclareVisibleToInstr(Instruction* dbg_declare,
 
 bool DebugInfoManager::AddDebugValueIfVarDeclIsVisible(
     Instruction* scope_and_line, uint32_t variable_id, uint32_t value_id,
-    Instruction* insert_pos,
-    std::unordered_set<Instruction*>* invisible_decls) {
+    Instruction* insert_pos, std::unordered_set<Instruction*>* invisible_decls,
+    bool force) {
   assert(scope_and_line != nullptr);
 
   auto dbg_decl_itr = var_id_to_dbg_decl_.find(variable_id);
@@ -573,7 +573,7 @@ bool DebugInfoManager::AddDebugValueIfVarDeclIsVisible(
 
   bool modified = false;
   for (auto* dbg_decl_or_val : dbg_decl_itr->second) {
-    if (!IsDeclareVisibleToInstr(dbg_decl_or_val, scope_and_line)) {
+    if (!IsDeclareVisibleToInstr(dbg_decl_or_val, scope_and_line) && !force) {
       if (invisible_decls) invisible_decls->insert(dbg_decl_or_val);
       continue;
     }
diff --git a/source/opt/debug_info_manager.h b/source/opt/debug_info_manager.h
index df34b30f4..7dfb7c7f9 100644
--- a/source/opt/debug_info_manager.h
+++ b/source/opt/debug_info_manager.h
@@ -15,6 +15,7 @@
 #ifndef SOURCE_OPT_DEBUG_INFO_MANAGER_H_
 #define SOURCE_OPT_DEBUG_INFO_MANAGER_H_
 
+#include <set>
 #include <unordered_map>
 #include <unordered_set>
 
@@ -145,11 +146,12 @@ class DebugInfoManager {
   // variable that is in the scope of |scope_and_line| and whose memory is
   // |variable_id| and inserts it after the instruction |insert_pos|.
   // Returns whether a DebugValue is added or not. |invisible_decls| returns
-  // DebugDeclares invisible to |scope_and_line|.
+  // DebugDeclares invisible to |scope_and_line|. Assume visible if |force|
+  // is true.
   bool AddDebugValueIfVarDeclIsVisible(
       Instruction* scope_and_line, uint32_t variable_id, uint32_t value_id,
       Instruction* insert_pos,
-      std::unordered_set<Instruction*>* invisible_decls);
+      std::unordered_set<Instruction*>* invisible_decls, bool force = false);
 
   // Creates a DebugValue for DebugDeclare |dbg_decl| and inserts it before
   // |insert_before|. The new DebugValue has the same line and scope as
@@ -244,9 +246,18 @@ class DebugInfoManager {
   // operand is the function.
   std::unordered_map<uint32_t, Instruction*> fn_id_to_dbg_fn_;
 
+  // Orders Instruction* for use in associative containers (i.e. less than
+  // ordering). Unique Id is used.
+  typedef Instruction* InstPtr;
+  struct InstPtrLess {
+    bool operator()(const InstPtr& lhs, const InstPtr& rhs) const {
+      return lhs->unique_id() < rhs->unique_id();
+    }
+  };
+
   // Mapping from variable or value ids to DebugDeclare or DebugValue
   // instructions whose operand is the variable or value.
-  std::unordered_map<uint32_t, std::unordered_set<Instruction*>>
+  std::unordered_map<uint32_t, std::set<InstPtr, InstPtrLess>>
       var_id_to_dbg_decl_;
 
   // Mapping from DebugScope ids to users.
diff --git a/source/opt/local_single_store_elim_pass.cpp b/source/opt/local_single_store_elim_pass.cpp
index 8cdd0abdc..2efb1e2c8 100644
--- a/source/opt/local_single_store_elim_pass.cpp
+++ b/source/opt/local_single_store_elim_pass.cpp
@@ -179,7 +179,7 @@ bool LocalSingleStoreElimPass::RewriteDebugDeclares(Instruction* store_inst,
   uint32_t value_id = store_inst->GetSingleWordInOperand(1);
   bool modified =
       context()->get_debug_info_mgr()->AddDebugValueIfVarDeclIsVisible(
-          store_inst, var_id, value_id, store_inst, &invisible_decls);
+          store_inst, var_id, value_id, store_inst, &invisible_decls, true);
 
   // For cases like the argument passing for an inlined function, the value
   // assignment is out of DebugDeclare's scope, but we have to preserve the
diff --git a/source/opt/ssa_rewrite_pass.cpp b/source/opt/ssa_rewrite_pass.cpp
index 29ab6123f..23cd64135 100644
--- a/source/opt/ssa_rewrite_pass.cpp
+++ b/source/opt/ssa_rewrite_pass.cpp
@@ -316,7 +316,8 @@ void SSARewriter::ProcessStore(Instruction* inst, BasicBlock* bb) {
   if (pass_->IsTargetVar(var_id)) {
     WriteVariable(var_id, bb, val_id);
     pass_->context()->get_debug_info_mgr()->AddDebugValueIfVarDeclIsVisible(
-        inst, var_id, val_id, inst, &decls_invisible_to_value_assignment_);
+        inst, var_id, val_id, inst, &decls_invisible_to_value_assignment_,
+        true);
 
 #if SSA_REWRITE_DEBUGGING_LEVEL > 1
     std::cerr << "\tFound store '%" << var_id << " = %" << val_id << "': "
@@ -497,7 +498,7 @@ uint32_t SSARewriter::GetPhiArgument(const PhiCandidate* phi_candidate,
   return 0;
 }
 
-bool SSARewriter::ApplyReplacements() {
+bool SSARewriter::ApplyReplacements(Function* fp) {
   bool modified = false;
 
 #if SSA_REWRITE_DEBUGGING_LEVEL > 2
@@ -507,9 +508,20 @@ bool SSARewriter::ApplyReplacements() {
   std::cerr << "\n\n";
 #endif
 
+  // Sort phi candiates by reverse postorder. Operand of a Phi may be
+  // a phi itself so make sure all operand phis are generated first.
+  std::vector<const PhiCandidate*> ordered_phis_to_generate;
+  pass_->context()->cfg()->ForEachBlockInReversePostOrder(
+      &*fp->begin(), [&ordered_phis_to_generate, this](BasicBlock* bb) {
+        for (const PhiCandidate* phi_candidate : phis_to_generate_) {
+          if (phi_candidate->bb() == bb)
+            ordered_phis_to_generate.push_back(phi_candidate);
+        }
+      });
+
   // Add Phi instructions from completed Phi candidates.
   std::vector<Instruction*> generated_phis;
-  for (const PhiCandidate* phi_candidate : phis_to_generate_) {
+  for (const PhiCandidate* phi_candidate : ordered_phis_to_generate) {
 #if SSA_REWRITE_DEBUGGING_LEVEL > 2
     std::cerr << "Phi candidate: " << phi_candidate->PrettyPrint(pass_->cfg())
               << "\n";
@@ -557,11 +569,11 @@ bool SSARewriter::ApplyReplacements() {
         phi_candidate->var_id(), phi_candidate->result_id(),
         {SpvDecorationRelaxedPrecision});
 
-    // Add DebugValue for the new OpPhi instruction.
+    // Add DebugValue for the new OpPhi instruction. Assume OpPhi is visible.
     insert_it->SetDebugScope(local_var->GetDebugScope());
     pass_->context()->get_debug_info_mgr()->AddDebugValueIfVarDeclIsVisible(
         &*insert_it, phi_candidate->var_id(), phi_candidate->result_id(),
-        &*insert_it, &decls_invisible_to_value_assignment_);
+        &*insert_it, &decls_invisible_to_value_assignment_, true);
 
     modified = true;
   }
@@ -733,7 +745,7 @@ Pass::Status SSARewriter::RewriteFunctionIntoSSA(Function* fp) {
   FinalizePhiCandidates();
 
   // Finally, apply all the replacements in the IR.
-  bool modified = ApplyReplacements();
+  bool modified = ApplyReplacements(fp);
 
   auto status = AddDebugValuesForInvisibleDebugDecls(fp);
   if (status == Pass::Status::SuccessWithChange ||
diff --git a/source/opt/ssa_rewrite_pass.h b/source/opt/ssa_rewrite_pass.h
index 1f4cd24ca..44cdd360a 100644
--- a/source/opt/ssa_rewrite_pass.h
+++ b/source/opt/ssa_rewrite_pass.h
@@ -181,7 +181,7 @@ class SSARewriter {
   // Applies all the SSA replacement decisions.  This replaces loads/stores to
   // SSA target variables with their corresponding SSA IDs, and inserts Phi
   // instructions for them.
-  bool ApplyReplacements();
+  bool ApplyReplacements(Function* fp);
 
   // Registers a definition for variable |var_id| in basic block |bb| with
   // value |val_id|.