[SimplifyCFG] Keep !dgb metadata of moved instruction, if they match.

[fhahn]

Summary

Currently SimplifyCFG drops the debug locations of 'bonus' instructions.
Such instructions are moved before the first branch. The reason for the
current behavior is that this could lead to surprising debug stepping,
if the block that's folded is dead.

In case the first branch and the instructions to be folded have the same
debug location, this shouldn't be an issue and we can keep the debug
location.

[vsk]

In its current form, the rule for preserving debug locs doesn't seem to apply here (https://llvm.org/docs/HowToUpdateDebugInfo.html#when-to-preserve-an-instruction-location). Imho we ought to revise the guidance there before proceeding with this patch (e.g. by adding some carveout for when the destination debug loc is the same). If you can share, it might also help to have a little context - unless there's some intervening optimization, the dwarf generator's location cascade should kick in in this situation?

[fhahn]

In D93662#2476793, @vsk wrote:

In its current form, the rule for preserving debug locs doesn't seem to apply here (https://llvm.org/docs/HowToUpdateDebugInfo.html#when-to-preserve-an-instruction-location). Imho we ought to revise the guidance there before proceeding with this patch (e.g. by adding some carveout for when the destination debug loc is the same). If you can share, it might also help to have a little context - unless there's some intervening optimization, the dwarf generator's location cascade should kick in in this situation?

The use case I am looking at is optimization remarks, which use !dbg to tie remarks back to the high-level source code. I am working on remarks that use !dbg to group together instructions that originated from the same place in the original source. For that, it is very convenient if all instructions have accurate !dbg metadata. With what you said earlier, it might be good to also scan forwards/backwards to the next instruction with !dbg and use that.

I don't know much about how !dbg is actually used by the debuggers, but it seems like a rule like if an instruction is moved to a new insert point and the instruction at the insert point has the same debug location as the moved instruction the location can be preserved on the moved instruction should not regress the debug experience. And in some cases, it might be beneficial, e.g. in case later optimization move the same instructions around again. What do you think?

[jmorse]

This improvement looks good and preserves more source locations. Awkwardly, as @vsk says it's quite different from the other documented rules, because it relies on the context of where the instructions are going to be placed.

In terms of wording, how about adding an additional clause:

A transformation should preserve the debug location of an instruction [...] if the destination block already contains an instruction with an identical debug location

Which is slightly broader than your wording @fhahn, but IMHO gets across the idea that the location shouldn't be preserved if it's "new" to a block. Opinions?

[vsk]

In D93662#2479383, @jmorse wrote:

This improvement looks good and preserves more source locations. Awkwardly, as @vsk says it's quite different from the other documented rules, because it relies on the context of where the instructions are going to be placed.

+1

In terms of wording, how about adding an additional clause:

A transformation should preserve the debug location of an instruction [...] if the destination block already contains an instruction with an identical debug location

Which is slightly broader than your wording @fhahn, but IMHO gets across the idea that the location shouldn't be preserved if it's "new" to a block. Opinions?

I think that sounds reasonable. My initial thought was that the broader language might invite folks to consider scanning the destination block for a matching location: there's nothing wrong with doing that in principle, but imho it ought to be discouraged for performance reasons.

[fhahn]

In D93662#2479778, @vsk wrote:

In D93662#2479383, @jmorse wrote:

In terms of wording, how about adding an additional clause:

A transformation should preserve the debug location of an instruction [...] if the destination block already contains an instruction with an identical debug location

Which is slightly broader than your wording @fhahn, but IMHO gets across the idea that the location shouldn't be preserved if it's "new" to a block. Opinions?

I think that sounds reasonable. My initial thought was that the broader language might invite folks to consider scanning the destination block for a matching location: there's nothing wrong with doing that in principle, but imho it ought to be discouraged for performance reasons.

I updated the doc with the suggested wording. Not sure if it is worth adding a clause saying to should only be done when it can be cheaply determined that the location is not new to the block?

[vsk]

Looks good to me as-is. Thanks!

[fhahn]

In D93662#2487498, @vsk wrote:

Looks good to me as-is. Thanks!

Great, thanks everyone!

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

for (Instruction &BonusInst : *BB) {
       if (isa<DbgInfoIntrinsic>(BonusInst) || isa<BranchInst>(BonusInst))
         continue;
 
       Instruction *NewBonusInst = BonusInst.clone();
 
       if (&BonusInst == Cond)
         CondInPred = NewBonusInst;
 
-      // When we fold the bonus instructions we want to make sure we
-      // reset their debug locations in order to avoid stepping on dead
-      // code caused by folding dead branches.
+      if (PBI->getDebugLoc() != NewBonusInst->getDebugLoc()) {
+        // Unless the instruction has the same !dbg location as the original
+        // branch, drop it. When we fold the bonus instructions we want to make
+        // sure we reset their debug locations in order to avoid stepping on
+        // dead code caused by folding dead branches.
         NewBonusInst->setDebugLoc(DebugLoc());
+      }
 
       RemapInstruction(NewBonusInst, VMap,
                        RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
       VMap[&BonusInst] = NewBonusInst;
 
       // If we moved a load, we cannot any longer claim any knowledge about
       // its potential value. The previous information might have been valid
       // only given the branch precondition.

llvm/test/Transforms/SimplifyCFG/fold-debug-location.ll

 ; RUN: opt -S -simplifycfg -simplifycfg-require-and-preserve-domtree=1 < %s | FileCheck %s --match-full-lines
 
 ; Make sure we reset the debug location when folding instructions.
+; CHECK-LABEL: define {{.+}} @patatino({{.+}}
 ; CHECK: [[VAL:%.*]] = and i32 %c2, %k
 ; CHECK-NEXT: [[VAL2:%.*]] icmp eq i32 [[VAL]], 0
 
 declare i32 @bar(...)
 
 define i32 @patatino(i32 %k, i32 %c1, i32 %c2) !dbg !6 {
   %1 = and i32 %c1, %k, !dbg !8
   %2 = icmp eq i32 %1, 0, !dbg !9
   br i1 %2, label %8, label %3, !dbg !10
 
 3:
   %4 = and i32 %c2, %k, !dbg !11
   %5 = icmp eq i32 %4, 0, !dbg !12
   br i1 %5, label %8, label %6, !dbg !13
 
 6:
   %7 = tail call i32 (...) @bar(), !dbg !14
   br label %8, !dbg !15
 
 8:
   ret i32 undef, !dbg !16
 }
 
+
+; All instructions involved in folding have the same !dbg location. Make sure
+; they are preserved.
+define void @dbg_all_equal(i32 %k, i32 %c1, i32 %c2) !dbg !17 {
+; CHECK-LABEL: define {{.+}} @dbg_all_equal({{.+}}
+; CHECK-NEXT:    [[A1:%[a-z0-9]+]] = and i32 %c1, %k, !dbg [[DBG:![0-9]+]]
+; CHECK-NEXT:    [[C1:%[a-z0-9]+]] = icmp eq i32 [[A1]], 0, !dbg [[DBG]]
+; CHECK-NEXT:    [[A2:%[a-z0-9]+]] = and i32 %c2, %k, !dbg [[DBG]]
+; CHECK-NEXT:    [[C2:%[a-z0-9]+]] = icmp eq i32 [[A2]], 0, !dbg [[DBG]]
+; CHECK-NEXT:    [[OR:%[.a-z0-9]+]] = or i1 [[C1]], [[C2]], !dbg [[DBG]]
+; CHECK-NEXT:    br i1 [[OR]], label {{.+}}, label {{.+}}, !dbg [[DBG]]
+;
+  %1 = and i32 %c1, %k, !dbg !18
+  %2 = icmp eq i32 %1, 0, !dbg !18
+  br i1 %2, label %8, label %3, !dbg !18
+
+3:
+  %4 = and i32 %c2, %k, !dbg !18
+  %5 = icmp eq i32 %4, 0, !dbg !18
+  br i1 %5, label %8, label %6, !dbg !18
+
+6:
+  %7 = tail call i32 (...) @bar(), !dbg !18
+  br label %8, !dbg !18
+
+8:
+  ret void
+}
+
+
+
 !llvm.dbg.cu = !{!0}
 !llvm.debugify = !{!3, !4}
 !llvm.module.flags = !{!5}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
 !1 = !DIFile(filename: "a.ll", directory: "/")
 !2 = !{}
 !3 = !{i32 9}
 !4 = !{i32 0}
 !5 = !{i32 2, !"Debug Info Version", i32 3}
 !6 = distinct !DISubprogram(name: "patatino", linkageName: "patatino", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
 !7 = !DISubroutineType(types: !2)
 !8 = !DILocation(line: 1, column: 1, scope: !6)
 !9 = !DILocation(line: 2, column: 1, scope: !6)
 !10 = !DILocation(line: 3, column: 1, scope: !6)
 !11 = !DILocation(line: 4, column: 1, scope: !6)
 !12 = !DILocation(line: 5, column: 1, scope: !6)
 !13 = !DILocation(line: 6, column: 1, scope: !6)
 !14 = !DILocation(line: 7, column: 1, scope: !6)
 !15 = !DILocation(line: 8, column: 1, scope: !6)
 !16 = !DILocation(line: 9, column: 1, scope: !6)
+!17 = distinct !DISubprogram(name: "dbg_all_equal", linkageName: "dbg_all_equal", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!18 = !DILocation(line: 10, column: 10, scope: !17)