Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LiveDebugVariables] Fix a DBG_VALUE reordering issue #111124

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

dstenb
Copy link
Collaborator

@dstenb dstenb commented Oct 4, 2024

LDV could reorder reinserted fragment and non-fragment debug values for
the same variable (compared to the input order), potentially resulting
in stale values being presented.

For example, before:

DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)
DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
DBG_VALUE %0, $noreg, !13, !DIExpression()

After (without this patch):

DBG_VALUE %stack.0, 0, !13, !DIExpression()
DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)

It would also reorder DBG_VALUEs for different variables. Although that
does not matter for the debug information output, it resulted in some
noise in pass before/after diffs.

This should hopefully align so that instruction referencing and
DBG_VALUE emit debug instructions in the same order (see the
sdag-salvage-add.ll change).

  • Add pre-commit test for LiveDebugVariables reorder issue
  • [LiveDebugVariables] Fix a DBG_VALUE reordering issue

@llvmbot
Copy link
Member

llvmbot commented Oct 4, 2024

@llvm/pr-subscribers-backend-amdgpu

Author: David Stenberg (dstenb)

Changes

LDV could reorder reinserted fragment and non-fragment debug values for
the same variable (compared to the input order), potentially resulting
in stale values being presented.

For example, before:

DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)
DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
DBG_VALUE %0, $noreg, !13, !DIExpression()

After (without this patch):

DBG_VALUE %stack.0, 0, !13, !DIExpression()
DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)

It would also reorder DBG_VALUEs for different variables. Although that
does not matter for the debug information output, it resulted in some
noise in pass before/after diffs.

This should hopefully align so that instruction referencing and
DBG_VALUE emit debug instructions in the same order (see the
sdag-salvage-add.ll change).

  • Add pre-commit test for LiveDebugVariables reorder issue
  • [LiveDebugVariables] Fix a DBG_VALUE reordering issue

Full diff: https://github.com/llvm/llvm-project/pull/111124.diff

6 Files Affected:

  • (modified) llvm/lib/CodeGen/LiveDebugVariables.cpp (+3-2)
  • (modified) llvm/test/CodeGen/AMDGPU/debug-value2.ll (+3-3)
  • (modified) llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir (+2)
  • (added) llvm/test/DebugInfo/Mips/livedebugvariables-reorder.mir (+96)
  • (modified) llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir (+2-2)
  • (modified) llvm/test/DebugInfo/X86/sdag-salvage-add.ll (+2-5)
diff --git a/llvm/lib/CodeGen/LiveDebugVariables.cpp b/llvm/lib/CodeGen/LiveDebugVariables.cpp
index 822a1beb489592..13b33facb42086 100644
--- a/llvm/lib/CodeGen/LiveDebugVariables.cpp
+++ b/llvm/lib/CodeGen/LiveDebugVariables.cpp
@@ -1625,8 +1625,9 @@ findInsertLocation(MachineBasicBlock *MBB, SlotIndex Idx, LiveIntervals &LIS,
   }
 
   // Don't insert anything after the first terminator, though.
-  return MI->isTerminator() ? MBB->getFirstTerminator() :
-                              std::next(MachineBasicBlock::iterator(MI));
+  auto It = MI->isTerminator() ? MBB->getFirstTerminator() :
+                                 std::next(MachineBasicBlock::iterator(MI));
+  return skipDebugInstructionsForward(It, MBB->end());
 }
 
 /// Find an iterator for inserting the next DBG_VALUE instruction
diff --git a/llvm/test/CodeGen/AMDGPU/debug-value2.ll b/llvm/test/CodeGen/AMDGPU/debug-value2.ll
index 1d4c11de4076cb..b09d540dd6d7b5 100644
--- a/llvm/test/CodeGen/AMDGPU/debug-value2.ll
+++ b/llvm/test/CodeGen/AMDGPU/debug-value2.ll
@@ -12,11 +12,11 @@ define <4 x float> @Scene_transformT(i32 %subshapeIdx, <4 x float> %v, float %ti
 entry:
   ; CHECK: v_mov_b32_e32 v[[COPIED_ARG_PIECE:[0-9]+]], v9
 
-  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gScene <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr6
+  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr[[COPIED_ARG_PIECE]]
+  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr8
   ; CHECK: ;DEBUG_VALUE: Scene_transformT:gScene <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr7
+  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gScene <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr6
   call void @llvm.dbg.value(metadata ptr addrspace(1) %gScene, metadata !120, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !154
-  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr8
-  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr[[COPIED_ARG_PIECE]]
   call void @llvm.dbg.value(metadata ptr addrspace(1) %gSceneOffsets, metadata !121, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !155
   %call = tail call ptr addrspace(1) @Scene_getSubShapeData(i32 %subshapeIdx, ptr addrspace(1) %gScene, ptr addrspace(1) %gSceneOffsets)
   %m_linearMotion = getelementptr inbounds %struct.ShapeData, ptr addrspace(1) %call, i64 0, i32 2
diff --git a/llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir b/llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir
index 5df70096e9305d..d6171ec30b1271 100644
--- a/llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir
+++ b/llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir
@@ -3,11 +3,13 @@
 ## This tests that LiveDebugVariables does not trim non-inlined variable
 ## location.
 
+# CHECK: ![[VARA:.*]] = !DILocalVariable(name: "a"
 # CHECK: ![[VARB:.*]] = !DILocalVariable(name: "b"
 # CHECK: ![[VARC:.*]] = !DILocalVariable(name: "c"
 # CHECK: $at = COPY $a2
 # CHECK-NEXT: DBG_VALUE $at, $noreg, ![[VARC]], !DIExpression(), debug-location
 # CHECK: $s0 = COPY $a1
+# CHECK-NEXT: DBG_VALUE $a0, $noreg, ![[VARA]], !DIExpression(), debug-location
 # CHECK-NEXT: DBG_VALUE $s0, $noreg, ![[VARB]], !DIExpression(), debug-location
 
 --- |
diff --git a/llvm/test/DebugInfo/Mips/livedebugvariables-reorder.mir b/llvm/test/DebugInfo/Mips/livedebugvariables-reorder.mir
new file mode 100644
index 00000000000000..e9392586c2afb1
--- /dev/null
+++ b/llvm/test/DebugInfo/Mips/livedebugvariables-reorder.mir
@@ -0,0 +1,96 @@
+# RUN: llc -emit-call-site-info %s -mtriple=mips -start-before=register-coalescer -stop-after=virtregrewriter -o - | FileCheck %s
+
+# Verify that LiveDebugVariables does not reorder the stack location DBG_VALUE
+# and the fragmented DBG_VALUEs for aaa, as the latter may represent a stale
+# value. It should also not reorder the DBG_VALUEs for the different variables,
+# as that results in noise in pass before/after diffs.
+
+# CHECK-DAG: ![[aaa:[0-9]+]] = !DILocalVariable(name: "aaa"
+# CHECK-DAG: ![[bbb:[0-9]+]] = !DILocalVariable(name: "bbb"
+# CHECK-DAG: ![[ccc:[0-9]+]] = !DILocalVariable(name: "ccc"
+# CHECK-DAG: ![[ddd:[0-9]+]] = !DILocalVariable(name: "ddd"
+
+# CHECK: DBG_VALUE 1001, $noreg, ![[aaa]], !DIExpression(DW_OP_LLVM_fragment, 0, 16)
+# CHECK-NEXT: DBG_VALUE 1002, $noreg, ![[aaa]], !DIExpression(DW_OP_LLVM_fragment, 16, 16)
+# CHECK-NEXT: DBG_VALUE 222, $noreg, ![[bbb]], !DIExpression()
+# CHECK-NEXT: DBG_VALUE 333, $noreg, ![[ccc]], !DIExpression()
+# CHECK-NEXT: DBG_VALUE 444, $noreg, ![[ddd]], !DIExpression()
+# CHECK-NEXT: DBG_VALUE %stack.0, 0, ![[aaa]], !DIExpression()
+
+--- |
+  target datalayout = "E-m:m-p:32:32-i8:8:32-i16:16:32-i64:64-n32-S64"
+  target triple = "mips"
+
+  ; Function Attrs: nounwind
+  define dso_local i32 @main() local_unnamed_addr #0 !dbg !8 {
+  entry:
+    ret i32 0, !dbg !21
+  }
+
+  declare !dbg !22 dso_local i32 @foo() local_unnamed_addr
+
+  ; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+  attributes #0 = { nounwind "frame-pointer"="all" }
+  attributes #1 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!2, !3, !4, !5, !6}
+  !llvm.ident = !{!7}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 19.0.0git", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "main.c", directory: "/", checksumkind: CSK_MD5, checksum: "9edfcd32ce51b21ab508a4a0755aa78b")
+  !2 = !{i32 7, !"Dwarf Version", i32 5}
+  !3 = !{i32 2, !"Debug Info Version", i32 3}
+  !4 = !{i32 1, !"wchar_size", i32 4}
+  !5 = !{i32 7, !"frame-pointer", i32 2}
+  !6 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
+  !7 = !{!"clang version 19.0.0git"}
+  !8 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 12, type: !9, scopeLine: 12, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+  !9 = !DISubroutineType(types: !10)
+  !10 = !{!11}
+  !11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !12 = !{!13, !14, !15, !16}
+  !13 = !DILocalVariable(name: "aaa", scope: !8, file: !1, line: 13, type: !11)
+  !14 = !DILocalVariable(name: "bbb", scope: !8, file: !1, line: 14, type: !11)
+  !15 = !DILocalVariable(name: "ccc", scope: !8, file: !1, line: 15, type: !11)
+  !16 = !DILocalVariable(name: "ddd", scope: !8, file: !1, line: 16, type: !11)
+  !17 = !DILocation(line: 13, scope: !8)
+  !18 = !DILocation(line: 0, scope: !8)
+  !19 = !DILocation(line: 17, scope: !8)
+  !20 = !{i64 2147496201}
+  !21 = !DILocation(line: 18, scope: !8)
+  !22 = !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !9, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+
+...
+---
+name:            main
+alignment:       4
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gpr32 }
+frameInfo:
+  maxAlignment:    1
+  adjustsStack:    true
+  hasCalls:        true
+callSites:
+  - { bb: 0, offset: 1 }
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    ADJCALLSTACKDOWN 16, 0, implicit-def dead $sp, implicit $sp, debug-location !17
+    JAL @foo, csr_o32_fpxx, implicit-def dead $ra, implicit-def $sp, implicit-def $v0, debug-location !17
+    ADJCALLSTACKUP 16, 0, implicit-def dead $sp, implicit $sp, debug-location !17
+    %0:gpr32 = COPY killed $v0, debug-location !17
+    DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !18
+    DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !18
+    DBG_VALUE 222, $noreg, !14, !DIExpression(), debug-location !18
+    DBG_VALUE 333, $noreg, !15, !DIExpression(), debug-location !18
+    DBG_VALUE 444, $noreg, !16, !DIExpression(), debug-location !18
+    DBG_VALUE %0, $noreg, !13, !DIExpression(), debug-location !18
+    INLINEASM &"", 1 /* sideeffect attdialect */, 12 /* clobber */, implicit-def dead early-clobber $v0, 12 /* clobber */, implicit-def dead early-clobber $v1, 12 /* clobber */, implicit-def dead early-clobber $a0, 12 /* clobber */, implicit-def dead early-clobber $a1, 12 /* clobber */, implicit-def dead early-clobber $a2, 12 /* clobber */, implicit-def dead early-clobber $a3, 12 /* clobber */, implicit-def dead early-clobber $t0, 12 /* clobber */, implicit-def dead early-clobber $t1, 12 /* clobber */, implicit-def dead early-clobber $t2, 12 /* clobber */, implicit-def dead early-clobber $t3, 12 /* clobber */, implicit-def dead early-clobber $t4, 12 /* clobber */, implicit-def dead early-clobber $t5, 12 /* clobber */, implicit-def dead early-clobber $t6, 12 /* clobber */, implicit-def dead early-clobber $t7, 12 /* clobber */, implicit-def dead early-clobber $t8, 12 /* clobber */, implicit-def dead early-clobber $t9, 12 /* clobber */, implicit-def dead early-clobber $gp, 12 /* clobber */, implicit-def dead early-clobber $ra, 12 /* clobber */, implicit-def dead early-clobber $s0, 12 /* clobber */, implicit-def dead early-clobber $s1, 12 /* clobber */, implicit-def dead early-clobber $s2, 12 /* clobber */, implicit-def dead early-clobber $s3, 12 /* clobber */, implicit-def dead early-clobber $s4, 12 /* clobber */, implicit-def dead early-clobber $s5, 12 /* clobber */, implicit-def dead early-clobber $s6, 12 /* clobber */, implicit-def dead early-clobber $s7, 12 /* clobber */, implicit-def dead early-clobber $at, !20, debug-location !19
+    $v0 = COPY killed %0, debug-location !21
+    RetRA implicit killed $v0, debug-location !21
+
+...
diff --git a/llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir b/llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir
index 08130b47b4c5c9..7c8aa966d3d643 100644
--- a/llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir
+++ b/llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir
@@ -118,15 +118,15 @@ body:             |
 
 # CHECK-LABEL: bb.3:
 # CHECK:        dead renamable $rcx = IMPLICIT_DEF
-# CHECK-NEXT:   DBG_VALUE 0, $noreg, !23, !DIExpression()
 # CHECK-NEXT:   DBG_VALUE $rcx, $noreg, !18, !DIExpression()
+# CHECK-NEXT:   DBG_VALUE 0, $noreg, !23, !DIExpression()
 
 # CHECK-LABEL: bb.4:
 # CHECK:        liveins: $rax
 # CHECK:        DBG_VALUE $rax, $noreg, !18, !DIExpression()
 # CHECK-NEXT:   renamable $rax = BTS64ri8 killed renamable $rax, 0, implicit-def $eflags
-# CHECK-NEXT:   DBG_VALUE 0, $noreg, !23, !DIExpression()
 # CHECK-NEXT:   DBG_VALUE $rax, $noreg, !18, !DIExpression()
+# CHECK-NEXT:   DBG_VALUE 0, $noreg, !23, !DIExpression()
 # CHECK-NEXT:   renamable $rax = BTS64ri8 killed renamable $rax, 0, implicit-def $eflags
 # CHECK-NEXT:   DBG_VALUE $rax, $noreg, !18, !DIExpression()
 # CHECK-NEXT:   dead renamable $rax = BTS64ri8 killed renamable $rax, 0, implicit-def $eflags
diff --git a/llvm/test/DebugInfo/X86/sdag-salvage-add.ll b/llvm/test/DebugInfo/X86/sdag-salvage-add.ll
index 5a77b9504206cb..bd3ff01f9a8266 100644
--- a/llvm/test/DebugInfo/X86/sdag-salvage-add.ll
+++ b/llvm/test/DebugInfo/X86/sdag-salvage-add.ll
@@ -27,21 +27,18 @@
 ; instruction selection as it is folded into the load. As a result, we should
 ; refer to s4 and myVar with complex expressions.
 ;
-; NB: instruction referencing and DBG_VALUE modes produce debug insts in a
-; different order.
-;
 ; CHECK:         ![[S4:.*]] = !DILocalVariable(name: "s4",
 ; CHECK:         ![[MYVAR:.*]] = !DILocalVariable(name: "myVar",
 ; CHECK:         $rax = MOV64rm
 ; INSTRREF-SAME: debug-instr-number 2,
 ; INSTRREF-NEXT: DBG_INSTR_REF ![[S4]],
-; DBGVALUE-NEXT: DBG_VALUE $rax, $noreg, ![[MYVAR]],
+; DBGVALUE-NEXT: DBG_VALUE $rax, $noreg, ![[S4]],
 ; DBGVALUE-SAME:       !DIExpression(DW_OP_plus_uconst, 4096, DW_OP_stack_value)
 ; INSTRREF-SAME:       !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_plus_uconst, 4096, DW_OP_stack_value)
 ; INSTRREF-SAME: dbg-instr-ref(2, 0)
 
 ; INSTRREF:      DBG_INSTR_REF ![[MYVAR]],
-; DBGVALUE:      DBG_VALUE $rax, $noreg, ![[S4]],
+; DBGVALUE:      DBG_VALUE $rax, $noreg, ![[MYVAR]],
 ; DBGVALUE-SAME:           !DIExpression(DW_OP_plus_uconst, 4096, DW_OP_stack_value)
 ; INSTRREF-SAME:           !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_plus_uconst, 4096, DW_OP_stack_value)
 ; INSTRREF-SAME: dbg-instr-ref(2, 0)

@llvmbot
Copy link
Member

llvmbot commented Oct 4, 2024

@llvm/pr-subscribers-debuginfo

Author: David Stenberg (dstenb)

Changes

LDV could reorder reinserted fragment and non-fragment debug values for
the same variable (compared to the input order), potentially resulting
in stale values being presented.

For example, before:

DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)
DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
DBG_VALUE %0, $noreg, !13, !DIExpression()

After (without this patch):

DBG_VALUE %stack.0, 0, !13, !DIExpression()
DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)

It would also reorder DBG_VALUEs for different variables. Although that
does not matter for the debug information output, it resulted in some
noise in pass before/after diffs.

This should hopefully align so that instruction referencing and
DBG_VALUE emit debug instructions in the same order (see the
sdag-salvage-add.ll change).

  • Add pre-commit test for LiveDebugVariables reorder issue
  • [LiveDebugVariables] Fix a DBG_VALUE reordering issue

Full diff: https://github.com/llvm/llvm-project/pull/111124.diff

6 Files Affected:

  • (modified) llvm/lib/CodeGen/LiveDebugVariables.cpp (+3-2)
  • (modified) llvm/test/CodeGen/AMDGPU/debug-value2.ll (+3-3)
  • (modified) llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir (+2)
  • (added) llvm/test/DebugInfo/Mips/livedebugvariables-reorder.mir (+96)
  • (modified) llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir (+2-2)
  • (modified) llvm/test/DebugInfo/X86/sdag-salvage-add.ll (+2-5)
diff --git a/llvm/lib/CodeGen/LiveDebugVariables.cpp b/llvm/lib/CodeGen/LiveDebugVariables.cpp
index 822a1beb489592..13b33facb42086 100644
--- a/llvm/lib/CodeGen/LiveDebugVariables.cpp
+++ b/llvm/lib/CodeGen/LiveDebugVariables.cpp
@@ -1625,8 +1625,9 @@ findInsertLocation(MachineBasicBlock *MBB, SlotIndex Idx, LiveIntervals &LIS,
   }
 
   // Don't insert anything after the first terminator, though.
-  return MI->isTerminator() ? MBB->getFirstTerminator() :
-                              std::next(MachineBasicBlock::iterator(MI));
+  auto It = MI->isTerminator() ? MBB->getFirstTerminator() :
+                                 std::next(MachineBasicBlock::iterator(MI));
+  return skipDebugInstructionsForward(It, MBB->end());
 }
 
 /// Find an iterator for inserting the next DBG_VALUE instruction
diff --git a/llvm/test/CodeGen/AMDGPU/debug-value2.ll b/llvm/test/CodeGen/AMDGPU/debug-value2.ll
index 1d4c11de4076cb..b09d540dd6d7b5 100644
--- a/llvm/test/CodeGen/AMDGPU/debug-value2.ll
+++ b/llvm/test/CodeGen/AMDGPU/debug-value2.ll
@@ -12,11 +12,11 @@ define <4 x float> @Scene_transformT(i32 %subshapeIdx, <4 x float> %v, float %ti
 entry:
   ; CHECK: v_mov_b32_e32 v[[COPIED_ARG_PIECE:[0-9]+]], v9
 
-  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gScene <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr6
+  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr[[COPIED_ARG_PIECE]]
+  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr8
   ; CHECK: ;DEBUG_VALUE: Scene_transformT:gScene <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr7
+  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gScene <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr6
   call void @llvm.dbg.value(metadata ptr addrspace(1) %gScene, metadata !120, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !154
-  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr8
-  ; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr[[COPIED_ARG_PIECE]]
   call void @llvm.dbg.value(metadata ptr addrspace(1) %gSceneOffsets, metadata !121, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !155
   %call = tail call ptr addrspace(1) @Scene_getSubShapeData(i32 %subshapeIdx, ptr addrspace(1) %gScene, ptr addrspace(1) %gSceneOffsets)
   %m_linearMotion = getelementptr inbounds %struct.ShapeData, ptr addrspace(1) %call, i64 0, i32 2
diff --git a/llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir b/llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir
index 5df70096e9305d..d6171ec30b1271 100644
--- a/llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir
+++ b/llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir
@@ -3,11 +3,13 @@
 ## This tests that LiveDebugVariables does not trim non-inlined variable
 ## location.
 
+# CHECK: ![[VARA:.*]] = !DILocalVariable(name: "a"
 # CHECK: ![[VARB:.*]] = !DILocalVariable(name: "b"
 # CHECK: ![[VARC:.*]] = !DILocalVariable(name: "c"
 # CHECK: $at = COPY $a2
 # CHECK-NEXT: DBG_VALUE $at, $noreg, ![[VARC]], !DIExpression(), debug-location
 # CHECK: $s0 = COPY $a1
+# CHECK-NEXT: DBG_VALUE $a0, $noreg, ![[VARA]], !DIExpression(), debug-location
 # CHECK-NEXT: DBG_VALUE $s0, $noreg, ![[VARB]], !DIExpression(), debug-location
 
 --- |
diff --git a/llvm/test/DebugInfo/Mips/livedebugvariables-reorder.mir b/llvm/test/DebugInfo/Mips/livedebugvariables-reorder.mir
new file mode 100644
index 00000000000000..e9392586c2afb1
--- /dev/null
+++ b/llvm/test/DebugInfo/Mips/livedebugvariables-reorder.mir
@@ -0,0 +1,96 @@
+# RUN: llc -emit-call-site-info %s -mtriple=mips -start-before=register-coalescer -stop-after=virtregrewriter -o - | FileCheck %s
+
+# Verify that LiveDebugVariables does not reorder the stack location DBG_VALUE
+# and the fragmented DBG_VALUEs for aaa, as the latter may represent a stale
+# value. It should also not reorder the DBG_VALUEs for the different variables,
+# as that results in noise in pass before/after diffs.
+
+# CHECK-DAG: ![[aaa:[0-9]+]] = !DILocalVariable(name: "aaa"
+# CHECK-DAG: ![[bbb:[0-9]+]] = !DILocalVariable(name: "bbb"
+# CHECK-DAG: ![[ccc:[0-9]+]] = !DILocalVariable(name: "ccc"
+# CHECK-DAG: ![[ddd:[0-9]+]] = !DILocalVariable(name: "ddd"
+
+# CHECK: DBG_VALUE 1001, $noreg, ![[aaa]], !DIExpression(DW_OP_LLVM_fragment, 0, 16)
+# CHECK-NEXT: DBG_VALUE 1002, $noreg, ![[aaa]], !DIExpression(DW_OP_LLVM_fragment, 16, 16)
+# CHECK-NEXT: DBG_VALUE 222, $noreg, ![[bbb]], !DIExpression()
+# CHECK-NEXT: DBG_VALUE 333, $noreg, ![[ccc]], !DIExpression()
+# CHECK-NEXT: DBG_VALUE 444, $noreg, ![[ddd]], !DIExpression()
+# CHECK-NEXT: DBG_VALUE %stack.0, 0, ![[aaa]], !DIExpression()
+
+--- |
+  target datalayout = "E-m:m-p:32:32-i8:8:32-i16:16:32-i64:64-n32-S64"
+  target triple = "mips"
+
+  ; Function Attrs: nounwind
+  define dso_local i32 @main() local_unnamed_addr #0 !dbg !8 {
+  entry:
+    ret i32 0, !dbg !21
+  }
+
+  declare !dbg !22 dso_local i32 @foo() local_unnamed_addr
+
+  ; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+  attributes #0 = { nounwind "frame-pointer"="all" }
+  attributes #1 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!2, !3, !4, !5, !6}
+  !llvm.ident = !{!7}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 19.0.0git", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "main.c", directory: "/", checksumkind: CSK_MD5, checksum: "9edfcd32ce51b21ab508a4a0755aa78b")
+  !2 = !{i32 7, !"Dwarf Version", i32 5}
+  !3 = !{i32 2, !"Debug Info Version", i32 3}
+  !4 = !{i32 1, !"wchar_size", i32 4}
+  !5 = !{i32 7, !"frame-pointer", i32 2}
+  !6 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
+  !7 = !{!"clang version 19.0.0git"}
+  !8 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 12, type: !9, scopeLine: 12, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+  !9 = !DISubroutineType(types: !10)
+  !10 = !{!11}
+  !11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !12 = !{!13, !14, !15, !16}
+  !13 = !DILocalVariable(name: "aaa", scope: !8, file: !1, line: 13, type: !11)
+  !14 = !DILocalVariable(name: "bbb", scope: !8, file: !1, line: 14, type: !11)
+  !15 = !DILocalVariable(name: "ccc", scope: !8, file: !1, line: 15, type: !11)
+  !16 = !DILocalVariable(name: "ddd", scope: !8, file: !1, line: 16, type: !11)
+  !17 = !DILocation(line: 13, scope: !8)
+  !18 = !DILocation(line: 0, scope: !8)
+  !19 = !DILocation(line: 17, scope: !8)
+  !20 = !{i64 2147496201}
+  !21 = !DILocation(line: 18, scope: !8)
+  !22 = !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !9, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+
+...
+---
+name:            main
+alignment:       4
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gpr32 }
+frameInfo:
+  maxAlignment:    1
+  adjustsStack:    true
+  hasCalls:        true
+callSites:
+  - { bb: 0, offset: 1 }
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    ADJCALLSTACKDOWN 16, 0, implicit-def dead $sp, implicit $sp, debug-location !17
+    JAL @foo, csr_o32_fpxx, implicit-def dead $ra, implicit-def $sp, implicit-def $v0, debug-location !17
+    ADJCALLSTACKUP 16, 0, implicit-def dead $sp, implicit $sp, debug-location !17
+    %0:gpr32 = COPY killed $v0, debug-location !17
+    DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !18
+    DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !18
+    DBG_VALUE 222, $noreg, !14, !DIExpression(), debug-location !18
+    DBG_VALUE 333, $noreg, !15, !DIExpression(), debug-location !18
+    DBG_VALUE 444, $noreg, !16, !DIExpression(), debug-location !18
+    DBG_VALUE %0, $noreg, !13, !DIExpression(), debug-location !18
+    INLINEASM &"", 1 /* sideeffect attdialect */, 12 /* clobber */, implicit-def dead early-clobber $v0, 12 /* clobber */, implicit-def dead early-clobber $v1, 12 /* clobber */, implicit-def dead early-clobber $a0, 12 /* clobber */, implicit-def dead early-clobber $a1, 12 /* clobber */, implicit-def dead early-clobber $a2, 12 /* clobber */, implicit-def dead early-clobber $a3, 12 /* clobber */, implicit-def dead early-clobber $t0, 12 /* clobber */, implicit-def dead early-clobber $t1, 12 /* clobber */, implicit-def dead early-clobber $t2, 12 /* clobber */, implicit-def dead early-clobber $t3, 12 /* clobber */, implicit-def dead early-clobber $t4, 12 /* clobber */, implicit-def dead early-clobber $t5, 12 /* clobber */, implicit-def dead early-clobber $t6, 12 /* clobber */, implicit-def dead early-clobber $t7, 12 /* clobber */, implicit-def dead early-clobber $t8, 12 /* clobber */, implicit-def dead early-clobber $t9, 12 /* clobber */, implicit-def dead early-clobber $gp, 12 /* clobber */, implicit-def dead early-clobber $ra, 12 /* clobber */, implicit-def dead early-clobber $s0, 12 /* clobber */, implicit-def dead early-clobber $s1, 12 /* clobber */, implicit-def dead early-clobber $s2, 12 /* clobber */, implicit-def dead early-clobber $s3, 12 /* clobber */, implicit-def dead early-clobber $s4, 12 /* clobber */, implicit-def dead early-clobber $s5, 12 /* clobber */, implicit-def dead early-clobber $s6, 12 /* clobber */, implicit-def dead early-clobber $s7, 12 /* clobber */, implicit-def dead early-clobber $at, !20, debug-location !19
+    $v0 = COPY killed %0, debug-location !21
+    RetRA implicit killed $v0, debug-location !21
+
+...
diff --git a/llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir b/llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir
index 08130b47b4c5c9..7c8aa966d3d643 100644
--- a/llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir
+++ b/llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir
@@ -118,15 +118,15 @@ body:             |
 
 # CHECK-LABEL: bb.3:
 # CHECK:        dead renamable $rcx = IMPLICIT_DEF
-# CHECK-NEXT:   DBG_VALUE 0, $noreg, !23, !DIExpression()
 # CHECK-NEXT:   DBG_VALUE $rcx, $noreg, !18, !DIExpression()
+# CHECK-NEXT:   DBG_VALUE 0, $noreg, !23, !DIExpression()
 
 # CHECK-LABEL: bb.4:
 # CHECK:        liveins: $rax
 # CHECK:        DBG_VALUE $rax, $noreg, !18, !DIExpression()
 # CHECK-NEXT:   renamable $rax = BTS64ri8 killed renamable $rax, 0, implicit-def $eflags
-# CHECK-NEXT:   DBG_VALUE 0, $noreg, !23, !DIExpression()
 # CHECK-NEXT:   DBG_VALUE $rax, $noreg, !18, !DIExpression()
+# CHECK-NEXT:   DBG_VALUE 0, $noreg, !23, !DIExpression()
 # CHECK-NEXT:   renamable $rax = BTS64ri8 killed renamable $rax, 0, implicit-def $eflags
 # CHECK-NEXT:   DBG_VALUE $rax, $noreg, !18, !DIExpression()
 # CHECK-NEXT:   dead renamable $rax = BTS64ri8 killed renamable $rax, 0, implicit-def $eflags
diff --git a/llvm/test/DebugInfo/X86/sdag-salvage-add.ll b/llvm/test/DebugInfo/X86/sdag-salvage-add.ll
index 5a77b9504206cb..bd3ff01f9a8266 100644
--- a/llvm/test/DebugInfo/X86/sdag-salvage-add.ll
+++ b/llvm/test/DebugInfo/X86/sdag-salvage-add.ll
@@ -27,21 +27,18 @@
 ; instruction selection as it is folded into the load. As a result, we should
 ; refer to s4 and myVar with complex expressions.
 ;
-; NB: instruction referencing and DBG_VALUE modes produce debug insts in a
-; different order.
-;
 ; CHECK:         ![[S4:.*]] = !DILocalVariable(name: "s4",
 ; CHECK:         ![[MYVAR:.*]] = !DILocalVariable(name: "myVar",
 ; CHECK:         $rax = MOV64rm
 ; INSTRREF-SAME: debug-instr-number 2,
 ; INSTRREF-NEXT: DBG_INSTR_REF ![[S4]],
-; DBGVALUE-NEXT: DBG_VALUE $rax, $noreg, ![[MYVAR]],
+; DBGVALUE-NEXT: DBG_VALUE $rax, $noreg, ![[S4]],
 ; DBGVALUE-SAME:       !DIExpression(DW_OP_plus_uconst, 4096, DW_OP_stack_value)
 ; INSTRREF-SAME:       !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_plus_uconst, 4096, DW_OP_stack_value)
 ; INSTRREF-SAME: dbg-instr-ref(2, 0)
 
 ; INSTRREF:      DBG_INSTR_REF ![[MYVAR]],
-; DBGVALUE:      DBG_VALUE $rax, $noreg, ![[S4]],
+; DBGVALUE:      DBG_VALUE $rax, $noreg, ![[MYVAR]],
 ; DBGVALUE-SAME:           !DIExpression(DW_OP_plus_uconst, 4096, DW_OP_stack_value)
 ; INSTRREF-SAME:           !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_plus_uconst, 4096, DW_OP_stack_value)
 ; INSTRREF-SAME: dbg-instr-ref(2, 0)

@dstenb dstenb requested review from jmorse and SLTozer October 4, 2024 09:32
Copy link

github-actions bot commented Oct 4, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good with a question inline and nit, thanks for finding!

Technically this patch can have some quadratic behaviour, because if there are multiple DBG_VALUEs being inserted in the same place, we'll repeatedly skip over the ones we've already inserted. See the large comment above this hunk: I believe Facebook/Meta hit some serious compile-times because of that and installed a cache of insertion points at the start of blocks. I don't believe DBG_VALUEs cluster inside basic blocks to the same extent that they do at the start of blocks so this patch is /probably/ fine -- if you can see an obvious way to avoid skipping over previously-inserted DBG_VALUEs then that'd be great, but I don't think it should block the patch.

Comment on lines 35 to 36
attributes #0 = { nounwind "frame-pointer"="all" }
attributes #1 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these aren't necessary attributes please remove, they add needless future maintenance otherwise

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed unneeded attributes now.

I think that it might be possible to rewrite the test to not depend on "frame-pointer"="all" as well, but I did not do that now (but can do that if you prefer).

Comment on lines 14 to 21

; CHECK: ;DEBUG_VALUE: Scene_transformT:gScene <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr6
; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr[[COPIED_ARG_PIECE]]
; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr8
; CHECK: ;DEBUG_VALUE: Scene_transformT:gScene <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr7
; CHECK: ;DEBUG_VALUE: Scene_transformT:gScene <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr6
call void @llvm.dbg.value(metadata ptr addrspace(1) %gScene, metadata !120, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !154
; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr8
; CHECK: ;DEBUG_VALUE: Scene_transformT:gSceneOffsets <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr[[COPIED_ARG_PIECE]]
call void @llvm.dbg.value(metadata ptr addrspace(1) %gSceneOffsets, metadata !121, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !155
%call = tail call ptr addrspace(1) @Scene_getSubShapeData(i32 %subshapeIdx, ptr addrspace(1) %gScene, ptr addrspace(1) %gSceneOffsets)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard for me to analyse just from the github UI as this isn't a MIR test, but !120 = gScene, !121 = gSceneOffsets, which are ordered !120, !121 in the two pre-isel dbg.values. In the old check lines the DEBUG_VALUE lines come out in the right order, but now they're flipped?

Chances are I'm reading it wrong, or there's some other re-ordering happening somewhere in LLVM that LDVars was just accidentally undoing, seeing how the order in other test files changes is fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the debug values get reordered at isel due to the order of the lowered parameters. I should perhaps have added an XXX note about that in the test. I can do that.

define <4 x float> @Scene_transformT(i32 %subshapeIdx, <4 x float> %v, float %time, ptr addrspace(1) %gScene, ptr addrspace(1) %gSceneOffsets) local_unnamed_addr !dbg !110 {
entry:
    #dbg_value(ptr addrspace(1) %gScene, !120, !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef), !154)
    #dbg_value(ptr addrspace(1) %gSceneOffsets, !121, !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef), !155)
# *** IR Dump After AMDGPU DAG->DAG Pattern Instruction Selection (amdgpu-isel) ***:
# Machine code for function Scene_transformT: IsSSA, TracksLiveness
Function Live Ins: $sgpr4_sgpr5 in %55, $sgpr8_sgpr9 in %56, $sgpr10_sgpr11 in %57, $sgpr12 in %58, $sgpr13 in %59, $sgpr14 in %60, $sgpr15 in %61, $vgpr0 in %62, $vgpr1 in %63, $vgpr2 in %64, $vgpr3 in %65, $vgpr4 in %66, $vgpr5 in %67, $vgpr6 in %68, $vgpr7 in %69, $vgpr8 in %70, $vgpr9 in %71, $sgpr6_sgpr7 in %73, $vgpr31 in %74

bb.0.entry:
  successors: %bb.1(0x40000000), %bb.9(0x40000000); %bb.1(50.00%), %bb.9(50.00%)
  liveins: $sgpr4_sgpr5, $sgpr8_sgpr9, $sgpr10_sgpr11, $sgpr12, $sgpr13, $sgpr14, $sgpr15, $vgpr0, $vgpr1, $vgpr2, $vgpr3, $vgpr4, $vgpr5, $vgpr6, $vgpr7, $vgpr8, $vgpr9, $sgpr6_sgpr7, $vgpr31
  %74:vgpr_32 = COPY $vgpr31
  %73:sgpr_64 = COPY $sgpr6_sgpr7
  %71:vgpr_32 = COPY $vgpr9
  DBG_VALUE %71:vgpr_32, $noreg, !"gSceneOffsets", !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment, 32, 32), debug-location !155; GraphMaterialSystemKernels1.cl:2182:102 line no:2182
  %70:vgpr_32 = COPY $vgpr8
  DBG_VALUE %70:vgpr_32, $noreg, !"gSceneOffsets", !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment, 0, 32), debug-location !155; GraphMaterialSystemKernels1.cl:2182:102 line no:2182
  %69:vgpr_32 = COPY $vgpr7
  DBG_VALUE %69:vgpr_32, $noreg, !"gScene", !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment, 32, 32), debug-location !154; GraphMaterialSystemKernels1.cl:2182:80 line no:2182
  %68:vgpr_32 = COPY $vgpr6
  DBG_VALUE %68:vgpr_32, $noreg, !"gScene", !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment, 0, 32), debug-location !154; GraphMaterialSystemKernels1.cl:2182:80 line no:2182


!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!2, !3, !4, !5, !6}
!llvm.ident = !{!7}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can lose ident and probably most of the module flags

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed that, as well as some unneeded line number metadata.

@dstenb
Copy link
Collaborator Author

dstenb commented Oct 4, 2024

This looks good with a question inline and nit, thanks for finding!

Technically this patch can have some quadratic behaviour, because if there are multiple DBG_VALUEs being inserted in the same place, we'll repeatedly skip over the ones we've already inserted. See the large comment above this hunk: I believe Facebook/Meta hit some serious compile-times because of that and installed a cache of insertion points at the start of blocks. I don't believe DBG_VALUEs cluster inside basic blocks to the same extent that they do at the start of blocks so this patch is /probably/ fine -- if you can see an obvious way to avoid skipping over previously-inserted DBG_VALUEs then that'd be great, but I don't think it should block the patch.

I'll look further into that. I think I still have access to http://llvm-compile-time-tracker.com, so I should be hopefully able to run some performance measurements on this.

@dstenb
Copy link
Collaborator Author

dstenb commented Oct 7, 2024

This looks good with a question inline and nit, thanks for finding!
Technically this patch can have some quadratic behaviour, because if there are multiple DBG_VALUEs being inserted in the same place, we'll repeatedly skip over the ones we've already inserted. See the large comment above this hunk: I believe Facebook/Meta hit some serious compile-times because of that and installed a cache of insertion points at the start of blocks. I don't believe DBG_VALUEs cluster inside basic blocks to the same extent that they do at the start of blocks so this patch is /probably/ fine -- if you can see an obvious way to avoid skipping over previously-inserted DBG_VALUEs then that'd be great, but I don't think it should block the patch.

I'll look further into that. I think I still have access to http://llvm-compile-time-tracker.com, so I should be hopefully able to run some performance measurements on this.

Results: http://llvm-compile-time-tracker.com/compare.php?from=736c163523d7c6281513c120e7d85a6337c4501b&to=142397c089bc9775cd04763c70634196cdf2a7cf&stat=instructions%3Au.

The parent commit is a patch which disables instruction referencing mode (as the profiling is done on x86-64). I did not see any outliers there. However, I don't know how those builds compare to Facebook/Meta builds.

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for running the profile -- I think we've made the best effort of checking the performance implications so we should be clear to go.

(Fascinating how tramp3d-v4 is always a major outlier when it comes to debug-info performance, as far as we can tell it just spends 100% of its time processing DBG_VALUEs).

@dstenb
Copy link
Collaborator Author

dstenb commented Oct 15, 2024

Thanks for the reviews, and sorry for the delay in landing this! Will land it shortly.

dstenb added a commit to dstenb/llvm-project that referenced this pull request Oct 15, 2024
LDV could reorder reinserted fragment and non-fragment debug values for
the same variable (compared to the input order), potentially resulting
in stale values being presented.

For example, before:

  DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)
  DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
  DBG_VALUE %0, $noreg, !13, !DIExpression()

After (without this patch):

  DBG_VALUE %stack.0, 0, !13, !DIExpression()
  DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
  DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)

It would also reorder DBG_VALUEs for different variables. Although that
does not matter for the debug information output, it resulted in some
noise in before/after pass diffs.

This should hopefully align so that instruction referencing and
DBG_VALUE emit debug instructions in the same order (see the
sdag-salvage-add.ll change).
LDV could reorder reinserted fragment and non-fragment debug values for
the same variable (compared to the input order), potentially resulting
in stale values being presented.

For example, before:

  DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)
  DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
  DBG_VALUE %0, $noreg, !13, !DIExpression()

After (without this patch):

  DBG_VALUE %stack.0, 0, !13, !DIExpression()
  DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
  DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)

It would also reorder DBG_VALUEs for different variables. Although that
does not matter for the debug information output, it resulted in some
noise in before/after pass diffs.

This should hopefully align so that instruction referencing and
DBG_VALUE emit debug instructions in the same order (see the
sdag-salvage-add.ll change).
@dstenb dstenb merged commit 9786198 into llvm:main Oct 15, 2024
3 of 5 checks passed
@dstenb dstenb deleted the ldv_reorder branch October 15, 2024 09:38
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
LDV could reorder reinserted fragment and non-fragment debug values for
the same variable (compared to the input order), potentially resulting
in stale values being presented.

For example, before:

  DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)
  DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
  DBG_VALUE %0, $noreg, !13, !DIExpression()

After (without this patch):

  DBG_VALUE %stack.0, 0, !13, !DIExpression()
  DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
  DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)

It would also reorder DBG_VALUEs for different variables. Although that
does not matter for the debug information output, it resulted in some
noise in before/after pass diffs.

This should hopefully align so that instruction referencing and
DBG_VALUE emit debug instructions in the same order (see the
sdag-salvage-add.ll change).
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
LDV could reorder reinserted fragment and non-fragment debug values for
the same variable (compared to the input order), potentially resulting
in stale values being presented.

For example, before:

  DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)
  DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
  DBG_VALUE %0, $noreg, !13, !DIExpression()

After (without this patch):

  DBG_VALUE %stack.0, 0, !13, !DIExpression()
  DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
  DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)

It would also reorder DBG_VALUEs for different variables. Although that
does not matter for the debug information output, it resulted in some
noise in before/after pass diffs.

This should hopefully align so that instruction referencing and
DBG_VALUE emit debug instructions in the same order (see the
sdag-salvage-add.ll change).
EricWF pushed a commit to efcs/llvm-project that referenced this pull request Oct 22, 2024
LDV could reorder reinserted fragment and non-fragment debug values for
the same variable (compared to the input order), potentially resulting
in stale values being presented.

For example, before:

  DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)
  DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
  DBG_VALUE %0, $noreg, !13, !DIExpression()

After (without this patch):

  DBG_VALUE %stack.0, 0, !13, !DIExpression()
  DBG_VALUE 1002, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16)
  DBG_VALUE 1001, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16)

It would also reorder DBG_VALUEs for different variables. Although that
does not matter for the debug information output, it resulted in some
noise in before/after pass diffs.

This should hopefully align so that instruction referencing and
DBG_VALUE emit debug instructions in the same order (see the
sdag-salvage-add.ll change).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants