Set correct scope and line info for DebugValue (#4125)

The existing spirv-opt `DebugInfoManager::AddDebugValueForDecl()` sets
the scope and line info of the new added DebugValue using the scope and
line of DebugDeclare. This is wrong because only a single DebugDeclare
must exist under a scope while we have to add DebugValue for all the
places where the variable's value is updated. Therefore, we have to set
the scope and line of DebugValue based on the places of the variable
updates.

This bug makes
https://github.com/google/amber/blob/main/tests/cases/debugger_hlsl_shadowed_vars.amber
fail. This commit fixes the bug.

source/opt/debug_info_manager.cpp

@@ -500,14 +500,15 @@ bool DebugInfoManager::AddDebugValueIfVarDeclIsVisible(
            insert_before->opcode() == SpvOpVariable) {
       insert_before = insert_before->NextNode();
     }
-    modified |= AddDebugValueForDecl(dbg_decl_or_val, value_id,
+    modified |= AddDebugValueForDecl(dbg_decl_or_val, value_id, insert_before,
-                                     insert_before) != nullptr;
+                                     scope_and_line) != nullptr;
   }
   return modified;
 }
 
 Instruction* DebugInfoManager::AddDebugValueForDecl(
-    Instruction* dbg_decl, uint32_t value_id, Instruction* insert_before) {
+    Instruction* dbg_decl, uint32_t value_id, Instruction* insert_before,
+    Instruction* scope_and_line) {
   if (dbg_decl == nullptr || !IsDebugDeclare(dbg_decl)) return nullptr;
 
   std::unique_ptr<Instruction> dbg_val(dbg_decl->Clone(context()));
@@ -517,6 +518,7 @@ Instruction* DebugInfoManager::AddDebugValueForDecl(
   dbg_val->SetOperand(kDebugDeclareOperandVariableIndex, {value_id});
   dbg_val->SetOperand(kDebugValueOperandExpressionIndex,
                       {GetEmptyDebugExpression()->result_id()});
+  dbg_val->UpdateDebugInfoFrom(scope_and_line);
 
   auto* added_dbg_val = insert_before->InsertBefore(std::move(dbg_val));
   AnalyzeDebugInst(added_dbg_val);

source/opt/debug_info_manager.h

@@ -152,11 +152,14 @@ class DebugInfoManager {
       std::unordered_set<Instruction*>* invisible_decls);
 
   // Creates a DebugValue for DebugDeclare |dbg_decl| and inserts it before
-  // |insert_before|. The new DebugValue has the same line, scope, and
+  // |insert_before|. The new DebugValue has the same line and scope as
-  // operands with DebugDeclare but it uses |value_id| for value. Returns
+  // |scope_and_line|, or no scope and line information if |scope_and_line|
-  // the added DebugValue, or nullptr if it does not add a DebugValue.
+  // is nullptr. The new DebugValue has the same operands as DebugDeclare
+  // but it uses |value_id| for the value. Returns the created DebugValue,
+  // or nullptr if fails to create one.
   Instruction* AddDebugValueForDecl(Instruction* dbg_decl, uint32_t value_id,
-                                    Instruction* insert_before);
+                                    Instruction* insert_before,
+                                    Instruction* scope_and_line);
 
   // Erases |instr| from data structures of this class.
   void ClearDebugInfo(Instruction* instr);

source/opt/local_single_store_elim_pass.cpp

@@ -175,7 +175,7 @@ bool LocalSingleStoreElimPass::RewriteDebugDeclares(Instruction* store_inst,
     for (auto* decl : invisible_decls) {
       if (dominator_analysis->Dominates(store_inst, decl)) {
         context()->get_debug_info_mgr()->AddDebugValueForDecl(decl, value_id,
-                                                              decl);
+                                                              decl, store_inst);
         modified = true;
       }
     }

source/opt/scalar_replacement_pass.cpp

@@ -175,7 +175,8 @@ bool ScalarReplacementPass::ReplaceWholeDebugDeclare(
     Instruction* added_dbg_value =
         context()->get_debug_info_mgr()->AddDebugValueForDecl(
             dbg_decl, /*value_id=*/var->result_id(),
-            /*insert_before=*/var->NextNode());
+            /*insert_before=*/var->NextNode(), /*scope_and_line=*/dbg_decl);
+
     if (added_dbg_value == nullptr) return false;
     added_dbg_value->AddOperand(
         {SPV_OPERAND_TYPE_ID,

source/opt/ssa_rewrite_pass.cpp

@@ -658,8 +658,8 @@ Pass::Status SSARewriter::AddDebugValuesForInvisibleDebugDecls(Function* fp) {
   //   a = 3;
   //   foo(3);
   // After inlining:
-  //   a = 3; // we want to specify "DebugValue: %x = %int_3"
+  //   a = 3;
-  //   foo and x disappeared!
+  //   foo and x disappeared but we want to specify "DebugValue: %x = %int_3".
   //
   // We want to specify the value for the variable using |defs_at_block_[bb]|,
   // where |bb| is the basic block contains the decl.
@@ -681,16 +681,17 @@ Pass::Status SSARewriter::AddDebugValuesForInvisibleDebugDecls(Function* fp) {
     if (value && (pass_->context()->get_instr_block(value) == nullptr ||
                   dom_tree->Dominates(value, decl))) {
       if (pass_->context()->get_debug_info_mgr()->AddDebugValueForDecl(
-              decl, value->result_id(), decl) == nullptr) {
+              decl, value->result_id(), decl, value) == nullptr) {
         return Pass::Status::Failure;
       }
     } else {
       // If |value| in the same basic block does not dominate |decl|, we can
       // assign the value in the immediate dominator.
       value_id = GetValueAtBlock(var_id, dom_tree->ImmediateDominator(bb));
+      if (value_id) value = pass_->get_def_use_mgr()->GetDef(value_id);
       if (value_id &&
           pass_->context()->get_debug_info_mgr()->AddDebugValueForDecl(
-              decl, value_id, decl) == nullptr) {
+              decl, value_id, decl, value) == nullptr) {
         return Pass::Status::Failure;
       }
     }

test/opt/debug_info_manager_test.cpp

@@ -31,6 +31,9 @@ static const uint32_t kDebugFunctionOperandFunctionIndex = 13;
 static const uint32_t kDebugInlinedAtOperandLineIndex = 4;
 static const uint32_t kDebugInlinedAtOperandScopeIndex = 5;
 static const uint32_t kDebugInlinedAtOperandInlinedIndex = 6;
+static const uint32_t kOpLineInOperandFileIndex = 0;
+static const uint32_t kOpLineInOperandLineIndex = 1;
+static const uint32_t kOpLineInOperandColumnIndex = 2;
 
 namespace spvtools {
 namespace opt {
@@ -609,6 +612,80 @@ void main(float in_var_color : COLOR) {
   EXPECT_FALSE(dbg_info_mgr->IsVariableDebugDeclared(100));
 }
 
+TEST(DebugInfoManager, AddDebugValueForDecl) {
+  const std::string text = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "OpenCL.DebugInfo.100"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main" %in_var_COLOR
+               OpExecutionMode %main OriginUpperLeft
+          %5 = OpString "ps.hlsl"
+         %14 = OpString "#line 1 \"ps.hlsl\"
+void main(float in_var_color : COLOR) {
+  float color = in_var_color;
+}
+"
+         %17 = OpString "float"
+         %21 = OpString "main"
+         %24 = OpString "color"
+               OpName %in_var_COLOR "in.var.COLOR"
+               OpName %main "main"
+               OpDecorate %in_var_COLOR Location 0
+       %uint = OpTypeInt 32 0
+    %uint_32 = OpConstant %uint 32
+      %float = OpTypeFloat 32
+%_ptr_Input_float = OpTypePointer Input %float
+       %void = OpTypeVoid
+         %27 = OpTypeFunction %void
+%_ptr_Function_float = OpTypePointer Function %float
+%in_var_COLOR = OpVariable %_ptr_Input_float Input
+         %13 = OpExtInst %void %1 DebugExpression
+         %15 = OpExtInst %void %1 DebugSource %5 %14
+         %16 = OpExtInst %void %1 DebugCompilationUnit 1 4 %15 HLSL
+         %18 = OpExtInst %void %1 DebugTypeBasic %17 %uint_32 Float
+         %20 = OpExtInst %void %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %18 %18
+         %22 = OpExtInst %void %1 DebugFunction %21 %20 %15 1 1 %16 %21 FlagIsProtected|FlagIsPrivate 1 %main
+         %12 = OpExtInst %void %1 DebugInfoNone
+         %25 = OpExtInst %void %1 DebugLocalVariable %24 %18 %15 1 20 %22 FlagIsLocal 0
+       %main = OpFunction %void None %27
+         %28 = OpLabel
+        %100 = OpVariable %_ptr_Function_float Function
+         %31 = OpLoad %float %in_var_COLOR
+        %101 = OpExtInst %void %1 DebugScope %22
+               OpLine %5 13 7
+               OpStore %100 %31
+               OpNoLine
+        %102 = OpExtInst %void %1 DebugNoScope
+         %36 = OpExtInst %void %1 DebugDeclare %25 %100 %13
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  std::unique_ptr<IRContext> context =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  auto* def_use_mgr = context->get_def_use_mgr();
+  auto* dbg_decl = def_use_mgr->GetDef(36);
+  EXPECT_EQ(dbg_decl->GetOpenCL100DebugOpcode(),
+            OpenCLDebugInfo100DebugDeclare);
+
+  auto* dbg_info_mgr = context->get_debug_info_mgr();
+  Instruction* store = dbg_decl->PreviousNode();
+  auto* dbg_value =
+      dbg_info_mgr->AddDebugValueForDecl(dbg_decl, 100, dbg_decl, store);
+
+  EXPECT_EQ(dbg_value->GetOpenCL100DebugOpcode(), OpenCLDebugInfo100DebugValue);
+  EXPECT_EQ(dbg_value->dbg_line_inst()->GetSingleWordInOperand(
+                kOpLineInOperandFileIndex),
+            5);
+  EXPECT_EQ(dbg_value->dbg_line_inst()->GetSingleWordInOperand(
+                kOpLineInOperandLineIndex),
+            13);
+  EXPECT_EQ(dbg_value->dbg_line_inst()->GetSingleWordInOperand(
+                kOpLineInOperandColumnIndex),
+            7);
+}
+
 }  // namespace
 }  // namespace analysis
 }  // namespace opt

test/opt/local_single_store_elim_test.cpp

@@ -1403,18 +1403,20 @@ TEST_F(LocalSingleStoreElimTest, AddDebugValueforStoreOutOfDebugDeclareScope) {
 %param_var_pos = OpVariable %_ptr_Function_v4float Function
 %param_var_color = OpVariable %_ptr_Function_v4float Function
          %55 = OpLoad %v4float %in_var_POSITION
+               OpLine %7 6 23
                OpStore %param_var_pos %55
+               OpNoLine
          %56 = OpLoad %v4float %in_var_COLOR
 ;CHECK:      DebugNoScope
 ;CHECK-NOT:  OpLine
 ;CHECK:      [[pos:%\w+]] = OpLoad %v4float %in_var_POSITION
 ;CHECK:      [[color:%\w+]] = OpLoad %v4float %in_var_COLOR
 
-               OpStore %param_var_color %56
-         %93 = OpExtInst %void %1 DebugScope %48
-               OpLine %7 6 23
-         %73 = OpExtInst %void %1 DebugDeclare %53 %param_var_pos %52
                OpLine %7 7 23
+               OpStore %param_var_color %56
+               OpNoLine
+         %93 = OpExtInst %void %1 DebugScope %48
+         %73 = OpExtInst %void %1 DebugDeclare %53 %param_var_pos %52
          %74 = OpExtInst %void %1 DebugDeclare %51 %param_var_color %52
 ;CHECK:      OpLine [[file:%\w+]] 6 23
 ;CHECK-NEXT: {{%\w+}} = OpExtInst %void {{%\w+}} DebugValue [[dbg_pos]] [[pos]] [[empty_expr:%\w+]]

test/opt/local_ssa_elim_test.cpp

@@ -2261,9 +2261,13 @@ TEST_F(LocalSSAElimTest, AddDebugValueForFunctionParameterWithPhi) {
          %69 = OpExtInst %void %1 DebugLexicalBlock %60 7 21 %68
          %70 = OpExtInst %void %1 DebugLocalVariable %11 %59 %60 6 26 %67 FlagIsLocal 2
 
+; CHECK: [[color_name:%\w+]] = OpString "color"
+; CHECK: [[pos_name:%\w+]] = OpString "pos"
 ; CHECK: [[i_name:%\w+]] = OpString "i"
 ; CHECK: [[null_expr:%\w+]] = OpExtInst %void [[ext:%\w+]] DebugExpression
 ; CHECK: [[dbg_i:%\w+]] = OpExtInst %void [[ext]] DebugLocalVariable [[i_name]] {{%\w+}} {{%\w+}} 6 16 {{%\w+}} FlagIsLocal 1
+; CHECK: [[dbg_color:%\w+]] = OpExtInst %void [[ext]] DebugLocalVariable [[color_name]] {{%\w+}} {{%\w+}} 15 23
+; CHECK: [[dbg_pos:%\w+]] = OpExtInst %void [[ext]] DebugLocalVariable [[pos_name]] {{%\w+}} {{%\w+}} 14 23
          %71 = OpExtInst %void %1 DebugLocalVariable %15 %65 %60 6 16 %67 FlagIsLocal 1
          %72 = OpExtInst %void %1 DebugTypeFunction FlagIsProtected|FlagIsPrivate %62 %59 %59
          %73 = OpExtInst %void %1 DebugFunction %16 %72 %60 14 1 %61 %14 FlagIsProtected|FlagIsPrivate 15 %156
@@ -2282,13 +2286,23 @@ TEST_F(LocalSSAElimTest, AddDebugValueForFunctionParameterWithPhi) {
         %169 = OpExtInst %void %1 DebugNoScope
 %param_var_pos = OpVariable %_ptr_Function_v4float Function
 %param_var_color = OpVariable %_ptr_Function_v4float Function
+               OpLine %7 100 105
          %80 = OpLoad %v4float %in_var_POSITION
                OpStore %param_var_pos %80
+               OpNoLine
+               OpLine %7 200 205
          %81 = OpLoad %v4float %in_var_COLOR
                OpStore %param_var_color %81
+               OpNoLine
         %170 = OpExtInst %void %1 DebugScope %73
+
+; CHECK: OpLine {{%\w+}} 100 105
+; CHECK: DebugValue [[dbg_pos]]
         %124 = OpExtInst %void %1 DebugDeclare %78 %param_var_pos %77
+; CHECK: OpLine {{%\w+}} 200 205
+; CHECK: DebugValue [[dbg_color]]
         %125 = OpExtInst %void %1 DebugDeclare %76 %param_var_color %77
+
         %171 = OpExtInst %void %1 DebugScope %74
                OpLine %7 17 18