[llvm] [DebugInfo][InstCombine] Do not overwrite prior DILocation for new Insts (PR #108565)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 13 07:08:51 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-debuginfo
Author: Stephen Tozer (SLTozer)
<details>
<summary>Changes</summary>
When InstCombine replaces an old instruction with a new instruction, it copies !dbg and !annotation metadata from old to new. For some InstCombine patterns we set a specific DILocation on the new instruction prior to insertion, however, which more accurately reflects the new instruction. This more specific DILocation may be overwritten on insertion by a less appropriate one, resulting in a less correct line mapping. This patch changes this behaviour to only copy the DILocation from old to new if the new instruction has no existing DILocation (which will always be the case for a new instruction unless InstCombine has specifically set one).
This isn't a perfect solution. In the test case given, we create a load instruction and give it the merged location of the two existing load instructions it replaces. Prior to this patch, if the PHI that is being RAUWd has a DILocation, it will overwrite the merged location on the new load, which is bad. However even with this patch, if either of the load instructions has no DILocation, then the new load won't have one either, and it will thus receive the DILocation from the PHI. This is probably bad - the PHI could have a valid DILocation corresponding to a different line, meaning that if the load results in an invalid memory access then we will attribute the resulting crash to the PHI's line, which could be worse than giving no attribution at all. I don't see any easy way to solve this problem however without fundamentally changing how InstCombine assigns DILocations; the most likely fix would be to exclusively assign DILocations within the InstCombine patterns, or to otherwise allow them to specify whether or not a DILocation ought to be copied from the original instruction or not.
---
Full diff: https://github.com/llvm/llvm-project/pull/108565.diff
2 Files Affected:
- (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+7-2)
- (added) llvm/test/Transforms/InstCombine/new-inst-dbgloc-overwrite.ll (+77)
``````````diff
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 8195e0539305cc..bd9af1ded431e9 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -5163,8 +5163,13 @@ bool InstCombinerImpl::run() {
LLVM_DEBUG(dbgs() << "IC: Old = " << *I << '\n'
<< " New = " << *Result << '\n');
- Result->copyMetadata(*I,
- {LLVMContext::MD_dbg, LLVMContext::MD_annotation});
+ // We copy the old instruction's DebugLoc to the new instruction, unless
+ // InstCombine already assigned a DebugLoc to it, in which case we
+ // should trust the more specifically selected DebugLoc.
+ if (!Result->getDebugLoc())
+ Result->setDebugLoc(I->getDebugLoc());
+ // We also copy annotation metadata to the new instruction.
+ Result->copyMetadata(*I, LLVMContext::MD_annotation);
// Everything uses the new instruction now.
I->replaceAllUsesWith(Result);
diff --git a/llvm/test/Transforms/InstCombine/new-inst-dbgloc-overwrite.ll b/llvm/test/Transforms/InstCombine/new-inst-dbgloc-overwrite.ll
new file mode 100644
index 00000000000000..c8ce8d3b433f4d
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/new-inst-dbgloc-overwrite.ll
@@ -0,0 +1,77 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals smart
+;; Tests that when InstCombine sets a DILocation on the new instruction when it
+;; is created, we do not try to overwrite that DILocation when we later insert
+;; the new instruction.
+;; In this test, InstCombine replaces two loads joined by a PHI into a PHI of
+;; the addresses followed by a load, and gives the new load a merge of the two
+;; incoming load DILocations. This test verifies that the new load keeps that
+;; DILocation, and doesn't have it overwritten by the DILocation of the original
+;; PHI. It will, however, receive the !annotation attached to the original PHI.
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
+define void @test(ptr %xfA, ptr %xfB, i1 %cmp5) {
+; CHECK-LABEL: @test(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br i1 [[CMP5:%.*]], label [[IF_ELSE:%.*]], label [[IF_THEN6:%.*]]
+; CHECK: if.then6:
+; CHECK-NEXT: br label [[IF_END11:%.*]]
+; CHECK: if.else:
+; CHECK-NEXT: br label [[IF_END11]]
+; CHECK: if.end11:
+; CHECK-NEXT: [[XFA_PN:%.*]] = phi ptr [ [[XFA:%.*]], [[IF_ELSE]] ], [ [[XFB:%.*]], [[IF_THEN6]] ]
+; CHECK-NEXT: [[XF1_SROA_8_0_IN:%.*]] = getelementptr i8, ptr [[XFA_PN]], i64 4
+; CHECK-NEXT: [[XF1_SROA_8_0:%.*]] = load float, ptr [[XF1_SROA_8_0_IN]], align 4, !dbg [[DBG3:![0-9]+]], !annotation [[META7:![0-9]+]]
+; CHECK-NEXT: [[CMP_I:%.*]] = fcmp ugt float [[XF1_SROA_8_0]], 0.000000e+00
+; CHECK-NEXT: br i1 [[CMP_I]], label [[IF_END_I:%.*]], label [[IF_THEN_I:%.*]]
+; CHECK: if.then.i:
+; CHECK-NEXT: br label [[IF_END_I]]
+; CHECK: if.end.i:
+; CHECK-NEXT: ret void
+;
+entry:
+ br i1 %cmp5, label %if.else, label %if.then6
+
+if.then6: ; preds = %entry
+ %xf1.sroa.8.0.xfB.sroa_idx = getelementptr i8, ptr %xfB, i64 4
+ %xf1.sroa.8.0.copyload = load float, ptr %xf1.sroa.8.0.xfB.sroa_idx, align 4, !dbg !3
+ br label %if.end11
+
+if.else: ; preds = %entry
+ %xf1.sroa.8.0.xfA.sroa_idx = getelementptr i8, ptr %xfA, i64 4
+ %xf1.sroa.8.0.copyload494 = load float, ptr %xf1.sroa.8.0.xfA.sroa_idx, align 4, !dbg !7
+ br label %if.end11
+
+if.end11: ; preds = %if.else, %if.then6
+ %xf1.sroa.8.0 = phi float [ %xf1.sroa.8.0.copyload494, %if.else ], [ %xf1.sroa.8.0.copyload, %if.then6 ], !dbg !8, !annotation !9
+ %cmp.i = fcmp ugt float %xf1.sroa.8.0, 0.000000e+00
+ br i1 %cmp.i, label %if.end.i, label %if.then.i
+
+if.then.i: ; preds = %if.end11
+ br label %if.end.i
+
+if.end.i: ; preds = %if.then.i, %if.end11
+ ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 20.0.0git", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "test.cpp", directory: "/tmp")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocation(line: 63, column: 12, scope: !4)
+!4 = distinct !DISubprogram(name: "operator=", linkageName: "_ZN11btMatrix3x3aSERKS_", scope: null, file: !1, line: 61, type: !5, scopeLine: 62, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !6)
+!5 = distinct !DISubroutineType(types: !6)
+!6 = !{}
+!7 = !DILocation(line: 63, column: 15, scope: !4)
+!8 = !DILocation(line: 64, scope: !4)
+!9 = !{!"Test String"}
+;.
+; CHECK: [[META0:![0-9]+]] = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: [[META1:![0-9]+]], producer: "{{.*}}clang version {{.*}}", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+; CHECK: [[META1]] = !DIFile(filename: "test.cpp", directory: {{.*}})
+; CHECK: [[DBG3]] = !DILocation(line: 64, scope: [[META4:![0-9]+]])
+; CHECK: [[META4]] = distinct !DISubprogram(name: "operator=", linkageName: "_ZN11btMatrix3x3aSERKS_", scope: null, file: [[META1]], line: 61, type: [[META5:![0-9]+]], scopeLine: 62, spFlags: DISPFlagDefinition, unit: [[META0]], retainedNodes: [[META6:![0-9]+]])
+; CHECK: [[META5]] = distinct !DISubroutineType(types: [[META6]])
+; CHECK: [[META6]] = !{}
+; CHECK: [[META7]] = !{!"Test String"}
+;.
``````````
</details>
https://github.com/llvm/llvm-project/pull/108565
More information about the llvm-commits
mailing list