-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
[DebugInfo] Handle dbg.assigns in FastISel #80734
Conversation
There are some rare circumstances where dbg.assign intrinsics can reach FastISel. They are a more specialised kind of dbg.value intrinsic with more information about the originating alloca. They only occur during optimisation, but might reach FastISel through always_inlining an optimised function into an optnone function. This is a slight problem as it's not safe (for debug-info accuracy) to ignore any intrinsics, and for RemoveDIs (the intrinsic-replacement project) it causes a crash through an unhandled switch case. To get around this, we can just treat the dbg.assign as a dbg.value (it's an actual subclass) and use the variable location information from the dbg.value fields. This loses a small amount of debug-info about stack locations, but is more accurate than just ignoring the intrinsic.
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesThere are some rare circumstances where dbg.assign intrinsics can reach FastISel. They are a more specialised kind of dbg.value intrinsic with more information about the originating alloca. They only occur during optimisation, but might reach FastISel through always_inlining an optimised function into an optnone function. This is a slight problem as it's not safe (for debug-info accuracy) to ignore any intrinsics, and for RemoveDIs (the intrinsic-replacement project) it causes a crash through an unhandled switch case. To get around this, we can just treat the dbg.assign as a dbg.value (it's an actual subclass) and use the variable location information from the dbg.value fields. This loses a small amount of debug-info about stack locations, but is more accurate than just ignoring the intrinsic. (This has popped up deep in an LTO build of a large codebase while testing RemoveDIs, I figured it'd be good to fix it for the intrinsic-form at the same time, just to demonstrate the correct behaviour). Full diff: https://github.com/llvm/llvm-project/pull/80734.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
index 4df79f474e8d2b..f8756527da87f6 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -1197,7 +1197,8 @@ void FastISel::handleDbgInfo(const Instruction *II) {
V = DPV.getVariableLocationOp(0);
bool Res = false;
- if (DPV.getType() == DPValue::LocationType::Value) {
+ if (DPV.getType() == DPValue::LocationType::Value ||
+ DPV.getType() == DPValue::LocationType::Assign) {
Res = lowerDbgValue(V, DPV.getExpression(), DPV.getVariable(),
DPV.getDebugLoc());
} else {
@@ -1393,6 +1394,13 @@ bool FastISel::selectIntrinsicCall(const IntrinsicInst *II) {
return true;
}
+ case Intrinsic::dbg_assign:
+ // A dbg.assign is a dbg.value with more information, typically produced
+ // during optimisation. If one reaches fastisel then something odd has
+ // happened (such as an optimised function being always-inlined into an
+ // optnone function). We will not be using the extra information in the
+ // dbg.assign in that case, just use its dbg.value fields.
+ LLVM_FALLTHROUGH;
case Intrinsic::dbg_value: {
// This form of DBG_VALUE is target-independent.
const DbgValueInst *DI = cast<DbgValueInst>(II);
diff --git a/llvm/test/DebugInfo/X86/dbg-assign-fastisel.ll b/llvm/test/DebugInfo/X86/dbg-assign-fastisel.ll
new file mode 100644
index 00000000000000..d90a8640cd29b2
--- /dev/null
+++ b/llvm/test/DebugInfo/X86/dbg-assign-fastisel.ll
@@ -0,0 +1,44 @@
+; RUN: llc %s -fast-isel=true -start-after=codegenprepare -stop-before=finalize-isel -o - | FileCheck %s
+; RUN: llc %s -fast-isel=true -start-after=codegenprepare -stop-before=finalize-isel -o - --try-experimental-debuginfo-iterators | FileCheck %s
+
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-unknown"
+
+; CHECK: DBG_VALUE
+
+declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata)
+
+define dso_local i32 @foo(i32 %a, i32 %b) local_unnamed_addr !dbg !8 {
+entry:
+ call void @llvm.dbg.assign(metadata !DIArgList(i32 %a, i32 %b), metadata !16, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus), metadata !21, metadata ptr undef, metadata !DIExpression()), !dbg !17
+ %mul = mul nsw i32 %b, %a, !dbg !18
+ ret i32 %mul, !dbg !18
+}
+
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !19, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 11.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "debug_value_list_selectiondag.cpp", directory: "/")
+!2 = !{}
+!3 = !{i32 2, !"CodeView", i32 1}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 2}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{!"clang version 11.0.0"}
+!8 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !9, file: !9, line: 1, type: !10, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
+!9 = !DIFile(filename: ".\\debug_value_list.cpp", directory: "/tmp")
+!10 = !DISubroutineType(types: !11)
+!11 = !{!12, !12, !12}
+!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!13 = !{!14, !15, !16}
+!14 = !DILocalVariable(name: "b", arg: 2, scope: !8, file: !9, line: 1, type: !12)
+!15 = !DILocalVariable(name: "a", arg: 1, scope: !8, file: !9, line: 1, type: !12)
+!16 = !DILocalVariable(name: "c", scope: !8, file: !9, line: 2, type: !12)
+!17 = !DILocation(line: 0, scope: !8)
+!18 = !DILocation(line: 3, scope: !8)
+!19 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
+!20 = !DILocalVariable(name: "d", scope: !8, file: !9, line: 2, type: !12)
+!21 = distinct !DIAssignID()
|
; RUN: llc %s -fast-isel=true -start-after=codegenprepare -stop-before=finalize-isel -o - | FileCheck %s | ||
; RUN: llc %s -fast-isel=true -start-after=codegenprepare -stop-before=finalize-isel -o - --try-experimental-debuginfo-iterators | FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't really need the =true?
Are similar changes needed for SelectionDAG and GlobalISel? |
For SelectionDAG it's already had dbg.assign/"assignment-tracking" plumbed into it. However it looks like GlobalISel could do with this too -- I'll add that to this review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless other reviewers have any more comments to add, this change LGTM.
In a slight rush as we can't keep-up with the main branch forever while waiting to turn RemoveDIs on; happy to revise and extend this patch in the future if it's needed. |
There are some rare circumstances where dbg.assign intrinsics can reach FastISel. They are a more specialised kind of dbg.value intrinsic with more information about the originating alloca. They only occur during optimisation, but might reach FastISel through always_inlining an optimised function into an optnone function.
This is a slight problem as it's not safe (for debug-info accuracy) to ignore any intrinsics, and for RemoveDIs (the intrinsic-replacement project) it causes a crash through an unhandled switch case. To get around this, we can just treat the dbg.assign as a dbg.value (it's an actual subclass) and use the variable location information from the dbg.value fields. This loses a small amount of debug-info about stack locations, but is more accurate than just ignoring the intrinsic.
(This has popped up deep in an LTO build of a large codebase while testing RemoveDIs, I figured it'd be good to fix it for the intrinsic-form at the same time, just to demonstrate the correct behaviour).