Skip to content

Commit 84f6e1d

Browse files
jmorsejmorse
committed
[DebugInfo] Clone dbg.values in SimplifyCFG like normal instructions (#72526)
The code in the CloneInstructionsIntoPredec... function modified by this patch has a long history that dates back to 2011, see d715ec8. There, when folding branches, all dbg.value intrinsics seen when folding would be saved and then re-inserted at the end of whatever was folded. Over the last 12 years this behaviour has been preserved. However, IMO it's bad behaviour. If we have: inst1 dbg.value1 inst2 dbg.value2 And we fold that sequence into a different block, then we would want the instructions and variable assignments to appear in the same order. However because of this old behaviour, the dbg.values are sunk, and we get: inst1 inst2 dbg.value1 dbg.value2 This clustering of dbg.values can make assignments to the same variable invisible, as well as reducing the coverage of other assignments. This patch relaxes the CloneInstructions... function and allows it to clone and update dbg.values in-place, causing them to appear in the original order in the destination block. I've added some extra dbg.values to the updated test: without the changes to the pass, the dbg.values sink into a blob ahead of the select. The RemoveDIs code can't cope with this right now so I've removed the "--try..." flag, restored in a commit to land in a couple of hours. (Metadata changes to make the LLVM-IR parser not drop the debug-info for it being out of date. The RemoveDIs related RUN line has been removed because it was spuriously passing due to the debug-info being dropped).
1 parent e8cd401 commit 84f6e1d

File tree

2 files changed

+29
-27
lines changed

2 files changed

+29
-27
lines changed

‎llvm/lib/Transforms/Utils/SimplifyCFG.cpp

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1101,12 +1101,13 @@ static void CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
11011101
// Note that there may be multiple predecessor blocks, so we cannot move
11021102
// bonus instructions to a predecessor block.
11031103
for (Instruction &BonusInst : *BB) {
1104-
if (isa<DbgInfoIntrinsic>(BonusInst) || BonusInst.isTerminator())
1104+
if (BonusInst.isTerminator())
11051105
continue;
11061106

11071107
Instruction *NewBonusInst = BonusInst.clone();
11081108

1109-
if (PTI->getDebugLoc() != NewBonusInst->getDebugLoc()) {
1109+
if (!isa<DbgInfoIntrinsic>(BonusInst) &&
1110+
PTI->getDebugLoc() != NewBonusInst->getDebugLoc()) {
11101111
// Unless the instruction has the same !dbg location as the original
11111112
// branch, drop it. When we fold the bonus instructions we want to make
11121113
// sure we reset their debug locations in order to avoid stepping on
@@ -1116,7 +1117,6 @@ static void CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
11161117

11171118
RemapInstruction(NewBonusInst, VMap,
11181119
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
1119-
VMap[&BonusInst] = NewBonusInst;
11201120

11211121
// If we speculated an instruction, we need to drop any metadata that may
11221122
// result in undefined behavior, as the metadata might have been valid
@@ -1126,8 +1126,13 @@ static void CloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
11261126
NewBonusInst->dropUBImplyingAttrsAndMetadata();
11271127

11281128
NewBonusInst->insertInto(PredBlock, PTI->getIterator());
1129+
1130+
if (isa<DbgInfoIntrinsic>(BonusInst))
1131+
continue;
1132+
11291133
NewBonusInst->takeName(&BonusInst);
11301134
BonusInst.setName(NewBonusInst->getName() + ".old");
1135+
VMap[&BonusInst] = NewBonusInst;
11311136

11321137
// Update (liveout) uses of bonus instructions,
11331138
// now that the bonus instruction has been cloned into predecessor.
@@ -3749,16 +3754,6 @@ static bool performBranchToCommonDestFolding(BranchInst *BI, BranchInst *PBI,
37493754
PBI->setCondition(
37503755
createLogicalOp(Builder, Opc, PBI->getCondition(), BICond, "or.cond"));
37513756

3752-
// Copy any debug value intrinsics into the end of PredBlock.
3753-
for (Instruction &I : *BB) {
3754-
if (isa<DbgInfoIntrinsic>(I)) {
3755-
Instruction *NewI = I.clone();
3756-
RemapInstruction(NewI, VMap,
3757-
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
3758-
NewI->insertBefore(PBI);
3759-
}
3760-
}
3761-
37623757
++NumFoldBranchToCommonDest;
37633758
return true;
37643759
}

‎llvm/test/Transforms/SimplifyCFG/branch-fold-dbg.ll

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
22
; RUN: opt -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S < %s | FileCheck %s
3-
; RUN: opt -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S < %s --try-experimental-debuginfo-iterators | FileCheck %s
43

54
%0 = type { ptr, ptr }
65

@@ -9,23 +8,26 @@
98
define i1 @foo(i32) nounwind ssp !dbg !0 {
109
; CHECK-LABEL: @foo(
1110
; CHECK-NEXT: Entry:
12-
; CHECK-NEXT: [[TMP1:%.*]] = icmp slt i32 [[TMP0:%.*]], 0
13-
; CHECK-NEXT: [[TMP2:%.*]] = icmp sgt i32 [[TMP0]], 4
14-
; CHECK-NEXT: [[OR_COND:%.*]] = or i1 [[TMP1]], [[TMP2]]
15-
; CHECK-NEXT: br i1 [[OR_COND]], label [[COMMON_RET:%.*]], label [[BB2:%.*]]
11+
; CHECK-NEXT: [[TMP1:%.*]] = icmp slt i32 [[TMP0:%.*]], 0, !dbg [[DBG7:![0-9]+]]
12+
; CHECK-NEXT: [[TMP2:%.*]] = icmp sgt i32 [[TMP0]], 4, !dbg [[DBG7]]
13+
; CHECK-NEXT: [[OR_COND:%.*]] = or i1 [[TMP1]], [[TMP2]], !dbg [[DBG7]]
14+
; CHECK-NEXT: br i1 [[OR_COND]], label [[COMMON_RET:%.*]], label [[BB2:%.*]], !dbg [[DBG7]]
1615
; CHECK: BB2:
17-
; CHECK-NEXT: [[TMP3:%.*]] = shl i32 1, [[TMP0]]
18-
; CHECK-NEXT: [[TMP4:%.*]] = and i32 [[TMP3]], 31
19-
; CHECK-NEXT: [[TMP5:%.*]] = icmp eq i32 [[TMP4]], 0
16+
; CHECK-NEXT: [[TMP3:%.*]] = shl i32 1, [[TMP0]], !dbg [[DBG7]]
17+
; CHECK-NEXT: [[TMP4:%.*]] = and i32 [[TMP3]], 31, !dbg [[DBG7]]
18+
; CHECK-NEXT: [[TMP5:%.*]] = icmp eq i32 [[TMP4]], 0, !dbg [[DBG7]]
19+
; CHECK-NEXT: call void @llvm.dbg.value(metadata ptr null, metadata [[META8:![0-9]+]], metadata !DIExpression()), !dbg [[DBG13:![0-9]+]]
2020
; CHECK-NEXT: [[TMP6:%.*]] = getelementptr inbounds [5 x %0], ptr @[[GLOB0:[0-9]+]], i32 0, i32 [[TMP0]]
21+
; CHECK-NEXT: call void @llvm.dbg.value(metadata ptr [[TMP6]], metadata [[META8]], metadata !DIExpression()), !dbg [[DBG13]]
2122
; CHECK-NEXT: [[TMP7:%.*]] = icmp eq ptr [[TMP6]], null
22-
; CHECK-NEXT: [[OR_COND2:%.*]] = select i1 [[TMP5]], i1 true, i1 [[TMP7]]
23+
; CHECK-NEXT: call void @llvm.dbg.value(metadata ptr [[TMP6]], metadata [[META8]], metadata !DIExpression()), !dbg [[DBG13]]
24+
; CHECK-NEXT: [[OR_COND2:%.*]] = select i1 [[TMP5]], i1 true, i1 [[TMP7]], !dbg [[DBG7]]
2325
; CHECK-NEXT: [[TMP8:%.*]] = icmp slt i32 [[TMP0]], 0
24-
; CHECK-NEXT: [[SPEC_SELECT:%.*]] = select i1 [[OR_COND2]], i1 false, i1 [[TMP8]]
25-
; CHECK-NEXT: br label [[COMMON_RET]]
26+
; CHECK-NEXT: [[SPEC_SELECT:%.*]] = select i1 [[OR_COND2]], i1 false, i1 [[TMP8]], !dbg [[DBG7]]
27+
; CHECK-NEXT: br label [[COMMON_RET]], !dbg [[DBG7]]
2628
; CHECK: common.ret:
2729
; CHECK-NEXT: [[COMMON_RET_OP:%.*]] = phi i1 [ false, [[ENTRY:%.*]] ], [ [[SPEC_SELECT]], [[BB2]] ]
28-
; CHECK-NEXT: ret i1 [[COMMON_RET_OP]]
30+
; CHECK-NEXT: ret i1 [[COMMON_RET_OP]], !dbg [[DBG14:![0-9]+]]
2931
;
3032
Entry:
3133
%1 = icmp slt i32 %0, 0, !dbg !5
@@ -43,9 +45,11 @@ BB2: ; preds = %BB1
4345

4446

4547
BB3: ; preds = %BB2
48+
call void @llvm.dbg.value(metadata ptr null, metadata !7, metadata !DIExpression()), !dbg !12
4649
%6 = getelementptr inbounds [5 x %0], ptr @0, i32 0, i32 %0, !dbg !6
47-
call void @llvm.dbg.value(metadata ptr %6, metadata !7, metadata !{}), !dbg !12
50+
call void @llvm.dbg.value(metadata ptr %6, metadata !7, metadata !DIExpression()), !dbg !12
4851
%7 = icmp eq ptr %6, null, !dbg !13
52+
call void @llvm.dbg.value(metadata ptr %6, metadata !7, metadata !DIExpression()), !dbg !12
4953
br i1 %7, label %BB5, label %BB4, !dbg !13
5054

5155
BB4: ; preds = %BB3
@@ -59,12 +63,13 @@ BB5: ; preds = %BB3, %BB2, %BB1, %E
5963
declare void @llvm.dbg.value(metadata, metadata, metadata) nounwind readnone
6064

6165
!llvm.dbg.cu = !{!2}
66+
!llvm.module.flags = !{!16, !17}
6267

6368
!0 = distinct !DISubprogram(name: "foo", line: 231, isLocal: false, isDefinition: true, virtualIndex: 6, flags: DIFlagPrototyped, isOptimized: false, unit: !2, file: !15, scope: !1, type: !3)
6469
!1 = !DIFile(filename: "a.c", directory: "/private/tmp")
6570
!2 = distinct !DICompileUnit(language: DW_LANG_C99, producer: "clang (trunk 129006)", isOptimized: true, emissionKind: FullDebug, file: !15, enums: !4, retainedTypes: !4)
6671
!3 = !DISubroutineType(types: !4)
67-
!4 = !{null}
72+
!4 = !{}
6873
!5 = !DILocation(line: 131, column: 2, scope: !0)
6974
!6 = !DILocation(line: 134, column: 2, scope: !0)
7075
!7 = !DILocalVariable(name: "bar", line: 232, scope: !8, file: !1, type: !9)
@@ -76,3 +81,5 @@ declare void @llvm.dbg.value(metadata, metadata, metadata) nounwind readnone
7681
!13 = !DILocation(line: 234, column: 2, scope: !8)
7782
!14 = !DILocation(line: 274, column: 1, scope: !8)
7883
!15 = !DIFile(filename: "a.c", directory: "/private/tmp")
84+
!16 = !{i32 1, !"Debug Info Version", i32 3}
85+
!17 = !{i32 2, !"Dwarf Version", i32 4}

0 commit comments

Comments
 (0)