[Coroutines] [NFCI] Don't search the DILocalVariable for __promise when constructing the debug varaible for __coro_frame #105626
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-coroutines Author: Chuanqi Xu (ChuanqiXu9) ChangesAs the title mentioned, do not search for the DILocalVariable for __promise when constructing the debug variable for __coro_frame. This should make sense because the debug variable of And this patch makes the codes to construct the debug variable for This patch is not strictly NFC but it is intended to not affect any end users. Full diff: https://github.com/llvm/llvm-project/pull/105626.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index 73e30ea00a0e29..695c49dbaf2ac2 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -1113,7 +1113,8 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
// neither.
if (!DIS || !DIS->getUnit() ||
!dwarf::isCPlusPlus(
- (dwarf::SourceLanguage)DIS->getUnit()->getSourceLanguage()))
+ (dwarf::SourceLanguage)DIS->getUnit()->getSourceLanguage()) ||
+ DIS->getUnit()->getEmissionKind() != DebugEmissionKind::FullDebug)
return;
assert(Shape.ABI == coro::ABI::Switch &&
@@ -1121,30 +1122,11 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
DIBuilder DBuilder(*F.getParent(), /*AllowUnresolved*/ false);
- AllocaInst *PromiseAlloca = Shape.getPromiseAlloca();
- assert(PromiseAlloca &&
+ assert(Shape.getPromiseAlloca() &&
"Coroutine with switch ABI should own Promise alloca");
- TinyPtrVector<DbgDeclareInst *> DIs = findDbgDeclares(PromiseAlloca);
- TinyPtrVector<DbgVariableRecord *> DVRs = findDVRDeclares(PromiseAlloca);
-
- DILocalVariable *PromiseDIVariable = nullptr;
- DILocation *DILoc = nullptr;
- if (!DIs.empty()) {
- DbgDeclareInst *PromiseDDI = DIs.front();
- PromiseDIVariable = PromiseDDI->getVariable();
- DILoc = PromiseDDI->getDebugLoc().get();
- } else if (!DVRs.empty()) {
- DbgVariableRecord *PromiseDVR = DVRs.front();
- PromiseDIVariable = PromiseDVR->getVariable();
- DILoc = PromiseDVR->getDebugLoc().get();
- } else {
- return;
- }
-
- DILocalScope *PromiseDIScope = PromiseDIVariable->getScope();
- DIFile *DFile = PromiseDIScope->getFile();
- unsigned LineNum = PromiseDIVariable->getLine();
+ DIFile *DFile = DIS->getFile();
+ unsigned LineNum = DIS->getLine();
DICompositeType *FrameDITy = DBuilder.createStructType(
DIS->getUnit(), Twine(F.getName() + ".coro_frame_ty").str(),
@@ -1254,10 +1236,9 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
DBuilder.replaceArrays(FrameDITy, DBuilder.getOrCreateArray(Elements));
- auto *FrameDIVar = DBuilder.createAutoVariable(PromiseDIScope, "__coro_frame",
- DFile, LineNum, FrameDITy,
- true, DINode::FlagArtificial);
- assert(FrameDIVar->isValidLocationForIntrinsic(DILoc));
+ auto *FrameDIVar =
+ DBuilder.createAutoVariable(DIS, "__coro_frame", DFile, LineNum,
+ FrameDITy, true, DINode::FlagArtificial);
// Subprogram would have ContainedNodes field which records the debug
// variables it contained. So we need to add __coro_frame to the
@@ -1266,14 +1247,17 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
// If we don't add __coro_frame to the RetainedNodes, user may get
// `no symbol __coro_frame in context` rather than `__coro_frame`
// is optimized out, which is more precise.
- if (auto *SubProgram = dyn_cast<DISubprogram>(PromiseDIScope)) {
- auto RetainedNodes = SubProgram->getRetainedNodes();
- SmallVector<Metadata *, 32> RetainedNodesVec(RetainedNodes.begin(),
- RetainedNodes.end());
- RetainedNodesVec.push_back(FrameDIVar);
- SubProgram->replaceOperandWith(
- 7, (MDTuple::get(F.getContext(), RetainedNodesVec)));
- }
+ auto RetainedNodes = DIS->getRetainedNodes();
+ SmallVector<Metadata *, 32> RetainedNodesVec(RetainedNodes.begin(),
+ RetainedNodes.end());
+ RetainedNodesVec.push_back(FrameDIVar);
+ DIS->replaceOperandWith(7, (MDTuple::get(F.getContext(), RetainedNodesVec)));
+
+ // Construct the location for the frame debug variable. The column number
+ // is fake but it should be fine.
+ DILocation *DILoc =
+ DILocation::get(DIS->getContext(), LineNum, /*Column=*/1, DIS);
+ assert(FrameDIVar->isValidLocationForIntrinsic(DILoc));
if (UseNewDbgInfoFormat) {
DbgVariableRecord *NewDVR =
diff --git a/llvm/test/Transforms/Coroutines/coro-debug-coro-frame.ll b/llvm/test/Transforms/Coroutines/coro-debug-coro-frame.ll
index 8e5c4ab52e78eb..1d668fd0222f77 100644
--- a/llvm/test/Transforms/Coroutines/coro-debug-coro-frame.ll
+++ b/llvm/test/Transforms/Coroutines/coro-debug-coro-frame.ll
@@ -15,8 +15,7 @@
;
; CHECK-DAG: ![[FILE:[0-9]+]] = !DIFile(filename: "coro-debug.cpp"
; CHECK-DAG: ![[RAMP:[0-9]+]] = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov",
-; CHECK-DAG: ![[RAMP_SCOPE:[0-9]+]] = distinct !DILexicalBlock(scope: ![[RAMP]], file: ![[FILE]], line: 23
-; CHECK-DAG: ![[CORO_FRAME]] = !DILocalVariable(name: "__coro_frame", scope: ![[RAMP_SCOPE]], file: ![[FILE]], line: [[PROMISE_VAR_LINE:[0-9]+]], type: ![[FRAME_TYPE:[0-9]+]], flags: DIFlagArtificial)
+; CHECK-DAG: ![[CORO_FRAME]] = !DILocalVariable(name: "__coro_frame", scope: ![[RAMP]], file: ![[FILE]], line: [[CORO_FRAME_LINE:[0-9]+]], type: ![[FRAME_TYPE:[0-9]+]], flags: DIFlagArtificial)
; CHECK-DAG: ![[FRAME_TYPE]] = !DICompositeType(tag: DW_TAG_structure_type, name: "f.coro_frame_ty", {{.*}}elements: ![[ELEMENTS:[0-9]+]]
; CHECK-DAG: ![[ELEMENTS]] = !{![[RESUME_FN:[0-9]+]], ![[DESTROY_FN:[0-9]+]], ![[PROMISE:[0-9]+]], ![[VECTOR_TYPE:[0-9]+]], ![[INT64_0:[0-9]+]], ![[DOUBLE_1:[0-9]+]], ![[INT64_PTR:[0-9]+]], ![[INT32_2:[0-9]+]], ![[INT32_3:[0-9]+]], ![[UNALIGNED_UNKNOWN:[0-9]+]], ![[STRUCT:[0-9]+]], ![[CORO_INDEX:[0-9]+]], ![[SMALL_UNKNOWN:[0-9]+]]
; CHECK-DAG: ![[RESUME_FN]] = !DIDerivedType(tag: DW_TAG_member, name: "__resume_fn"{{.*}}, baseType: ![[RESUME_FN_TYPE:[0-9]+]]{{.*}}, flags: DIFlagArtificial
@@ -29,25 +28,26 @@
; CHECK-DAG: ![[UNKNOWN_TYPE_BASE]] = !DIBasicType(name: "UnknownType", size: 8, encoding: DW_ATE_unsigned_char, flags: DIFlagArtificial)
; CHECK-DAG: ![[VECTOR_TYPE_BASE_ELEMENTS]] = !{![[VECTOR_TYPE_BASE_SUBRANGE:[0-9]+]]}
; CHECK-DAG: ![[VECTOR_TYPE_BASE_SUBRANGE]] = !DISubrange(count: 16, lowerBound: 0)
-; CHECK-DAG: ![[INT64_0]] = !DIDerivedType(tag: DW_TAG_member, name: "__int_64_1", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[PROMISE_VAR_LINE]], baseType: ![[I64_BASE:[0-9]+]],{{.*}}, flags: DIFlagArtificial
+; CHECK-DAG: ![[INT64_0]] = !DIDerivedType(tag: DW_TAG_member, name: "__int_64_1", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[CORO_FRAME_LINE]], baseType: ![[I64_BASE:[0-9]+]],{{.*}}, flags: DIFlagArtificial
; CHECK-DAG: ![[I64_BASE]] = !DIBasicType(name: "__int_64", size: 64, encoding: DW_ATE_signed, flags: DIFlagArtificial)
-; CHECK-DAG: ![[DOUBLE_1]] = !DIDerivedType(tag: DW_TAG_member, name: "__double__2", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[PROMISE_VAR_LINE]], baseType: ![[DOUBLE_BASE:[0-9]+]]{{.*}}, flags: DIFlagArtificial
+; CHECK-DAG: ![[DOUBLE_1]] = !DIDerivedType(tag: DW_TAG_member, name: "__double__2", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[CORO_FRAME_LINE]], baseType: ![[DOUBLE_BASE:[0-9]+]]{{.*}}, flags: DIFlagArtificial
; CHECK-DAG: ![[DOUBLE_BASE]] = !DIBasicType(name: "__double_", size: 64, encoding: DW_ATE_float, flags: DIFlagArtificial)
-; CHECK-DAG: ![[INT32_2]] = !DIDerivedType(tag: DW_TAG_member, name: "__int_32_4", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[PROMISE_VAR_LINE]], baseType: ![[I32_BASE:[0-9]+]]{{.*}}, flags: DIFlagArtificial
+; CHECK-DAG: ![[INT32_2]] = !DIDerivedType(tag: DW_TAG_member, name: "__int_32_4", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[CORO_FRAME_LINE]], baseType: ![[I32_BASE:[0-9]+]]{{.*}}, flags: DIFlagArtificial
; CHECK-DAG: ![[I32_BASE]] = !DIBasicType(name: "__int_32", size: 32, encoding: DW_ATE_signed, flags: DIFlagArtificial)
-; CHECK-DAG: ![[INT32_3]] = !DIDerivedType(tag: DW_TAG_member, name: "__int_32_5", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[PROMISE_VAR_LINE]], baseType: ![[I32_BASE]]
+; CHECK-DAG: ![[INT32_3]] = !DIDerivedType(tag: DW_TAG_member, name: "__int_32_5", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[CORO_FRAME_LINE]], baseType: ![[I32_BASE]]
; CHECK-DAG: ![[UNALIGNED_UNKNOWN]] = !DIDerivedType(tag: DW_TAG_member, name: "_6",{{.*}}baseType: ![[UNALIGNED_UNKNOWN_BASE:[0-9]+]], size: 9
; CHECK-DAG: ![[UNALIGNED_UNKNOWN_BASE]] = !DICompositeType(tag: DW_TAG_array_type, baseType: ![[UNKNOWN_TYPE_BASE]], size: 16,{{.*}} elements: ![[UNALIGNED_UNKNOWN_ELEMENTS:[0-9]+]])
; CHECK-DAG: ![[UNALIGNED_UNKNOWN_ELEMENTS]] = !{![[UNALIGNED_UNKNOWN_SUBRANGE:[0-9]+]]}
; CHECk-DAG: ![[UNALIGNED_UNKNOWN_SUBRANGE]] = !DISubrange(count: 2, lowerBound: 0)
-; CHECK-DAG: ![[STRUCT]] = !DIDerivedType(tag: DW_TAG_member, name: "struct_big_structure_7", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[PROMISE_VAR_LINE]], baseType: ![[STRUCT_BASE:[0-9]+]]
+; CHECK-DAG: ![[STRUCT]] = !DIDerivedType(tag: DW_TAG_member, name: "struct_big_structure_7", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[CORO_FRAME_LINE]], baseType: ![[STRUCT_BASE:[0-9]+]]
; CHECK-DAG: ![[STRUCT_BASE]] = !DICompositeType(tag: DW_TAG_structure_type, name: "struct_big_structure"{{.*}}, align: 64, flags: DIFlagArtificial, elements: ![[STRUCT_ELEMENTS:[0-9]+]]
; CHECK-DAG: ![[STRUCT_ELEMENTS]] = !{![[MEM_TYPE:[0-9]+]]}
; CHECK-DAG: ![[MEM_TYPE]] = !DIDerivedType(tag: DW_TAG_member,{{.*}} baseType: ![[MEM_TYPE_BASE:[0-9]+]], size: 4000
; CHECK-DAG: ![[MEM_TYPE_BASE]] = !DICompositeType(tag: DW_TAG_array_type, baseType: ![[UNKNOWN_TYPE_BASE]], size: 4000,
; CHECK-DAG: ![[CORO_INDEX]] = !DIDerivedType(tag: DW_TAG_member, name: "__coro_index"
; CHECK-DAG: ![[SMALL_UNKNOWN]] = !DIDerivedType(tag: DW_TAG_member, name: "UnknownType_8",{{.*}} baseType: ![[UNKNOWN_TYPE_BASE]], size: 5
-; CHECK-DAG: ![[PROMISE_VAR:[0-9]+]] = !DILocalVariable(name: "__promise", scope: ![[RAMP_SCOPE]], file: ![[FILE]], line: [[PROMISE_VAR_LINE]]
+; CHECK-DAG: ![[PROMISE_VAR:[0-9]+]] = !DILocalVariable(name: "__promise", scope: ![[RAMP_SCOPE:[0-9]+]], file: ![[FILE]]
+; CHECK-DAG: ![[RAMP_SCOPE]] = distinct !DILexicalBlock(scope: ![[RAMP]], file: ![[FILE]], line: 23
; CHECK-DAG: ![[BAR_FUNC:[0-9]+]] = distinct !DISubprogram(name: "bar", linkageName: "_Z3barv",
; CHECK-DAG: ![[BAR_SCOPE:[0-9]+]] = distinct !DILexicalBlock(scope: ![[BAR_FUNC]], file: !1
; CHECK-DAG: ![[FRAME_TYPE_IN_BAR:[0-9]+]] = !DICompositeType(tag: DW_TAG_structure_type, name: "bar.coro_frame_ty", file: ![[FILE]], line: [[BAR_LINE:[0-9]+]]{{.*}}elements: ![[ELEMENTS_IN_BAR:[0-9]+]]
diff --git a/llvm/test/Transforms/Coroutines/coro-debug-dbg.values.ll b/llvm/test/Transforms/Coroutines/coro-debug-dbg.values.ll
index 0b3acc30a1eee0..28f5841bb20af7 100644
--- a/llvm/test/Transforms/Coroutines/coro-debug-dbg.values.ll
+++ b/llvm/test/Transforms/Coroutines/coro-debug-dbg.values.ll
@@ -25,6 +25,7 @@
; CHECK-SAME: ptr {{.*}} %[[frame:.*]])
; CHECK-SAME: !dbg ![[RESUME_FN_DBG_NUM:[0-9]+]]
; CHECK: %[[frame_alloca:.*]] = alloca ptr
+; CHECK-NEXT: #dbg_declare(ptr %begin.debug, ![[FRAME_DI_NUM:[0-9]+]],
; CHECK-NEXT: store ptr %[[frame]], ptr %[[frame_alloca]]
; CHECK: init.ready:
; CHECK: #dbg_value(ptr %[[frame_alloca]], ![[XVAR_RESUME:[0-9]+]],
@@ -38,6 +39,7 @@
; CHECK-SAME: !DIExpression(DW_OP_deref, DW_OP_plus_uconst, [[OffsetJ]], DW_OP_deref)
;
; CHECK: ![[RESUME_FN_DBG_NUM]] = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov"
+; CHECK: ![[FRAME_DI_NUM]] = !DILocalVariable(name: "__coro_frame"
; CHECK: ![[IVAR_RESUME]] = !DILocalVariable(name: "i"
; CHECK: ![[XVAR_RESUME]] = !DILocalVariable(name: "x"
; CHECK: ![[JVAR_RESUME]] = !DILocalVariable(name: "j"
diff --git a/llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll b/llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll
index 4f5cdcf15618c7..93b22081cf12f6 100644
--- a/llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll
+++ b/llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll
@@ -42,12 +42,14 @@
; CHECK-NEXT: %[[DBG_PTR:.*]] = alloca ptr
; CHECK-NEXT: #dbg_declare(ptr %[[DBG_PTR]], ![[XVAR_RESUME:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 32),
; CHECK-NEXT: #dbg_declare(ptr %[[DBG_PTR]], ![[IVAR_RESUME:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 20), ![[IDBGLOC_RESUME:[0-9]+]]
+; CHECK-NEXT: #dbg_declare(ptr %[[DBG_PTR]], ![[FRAME_RESUME:[0-9]+]], !DIExpression(DW_OP_deref),
; CHECK-NEXT: store ptr {{.*}}, ptr %[[DBG_PTR]]
; CHECK: %[[J:.*]] = alloca i32, align 4
; CHECK-NEXT: #dbg_declare(ptr %[[J]], ![[JVAR_RESUME:[0-9]+]], !DIExpression(), ![[JDBGLOC_RESUME:[0-9]+]]
; CHECK: init.ready:
; CHECK: await.ready:
;
+; CHECK-DAG: ![[FRAME_RESUME]] = !DILocalVariable(name: "__coro_frame"
; CHECK-DAG: ![[IVAR]] = !DILocalVariable(name: "i"
; CHECK-DAG: ![[PROG_SCOPE:[0-9]+]] = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov"
; CHECK-DAG: ![[BLK_SCOPE:[0-9]+]] = distinct !DILexicalBlock(scope: ![[PROG_SCOPE]], file: !1, line: 23, column: 12)
|
|
I realized why this received regression reports (08a0dec#commitcomment-145619095) is that, in However, after we optimize this, we tried to always to generate the debug info. But I didn't realize that it is possible we're going to generate debug info but not the debug info for local variables (the -glm case). @alinas @gribozavr Could you try to test this in your code bases to avoid breaking your repo again? |
…en constructing the debug varaible for __coro_frame As the title mentioned, do not search for the DILocalVariable for __promise when constructing the debug variable for __coro_frame. This should make sense because the debug variable of `__coro_frame` shouldn't dependent on the debug variable of `__promise`. And actually, it is not. Currently, we search the debug variable for `__promise` only because we want to get the debug location and the debug scope for the `__promise`. However, we can construct the debug location directly from the debug scope of the being compiled function. Then it is not necessary any more to search the `__promise` variable. And this patch makes the codes to construct the debug variable for `__coro_frame` to be more stable. Now we will always be able to construct the debug variable for the coroutine frame no matter if we found the debug variable for the __promise or not. This patch is not strictly NFC but it is intended to not affect any end users.
ChuanqiXu9
force-pushed
the
CoroDebugRefactor
branch
from
21cc26a to
2d9d74c
Compare
You can test this locally with the following command:git-clang-format --diff 65f66d2c605f0c9b0af26244f4d42ca93f552ec8 2d9d74cd686883dcd93826dfdca8482413d0990b --extensions cpp -- llvm/lib/Transforms/Coroutines/CoroFrame.cppView the diff from clang-format here.diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index 996cc142f3..df38d50e97 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -1114,7 +1114,8 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
if (!DIS || !DIS->getUnit() ||
!dwarf::isCPlusPlus(
(dwarf::SourceLanguage)DIS->getUnit()->getSourceLanguage()) ||
- DIS->getUnit()->getEmissionKind() != DICompileUnit::DebugEmissionKind::FullDebug)
+ DIS->getUnit()->getEmissionKind() !=
+ DICompileUnit::DebugEmissionKind::FullDebug)
return;
assert(Shape.ABI == coro::ABI::Switch &&
|
|
I am going to land this since I feel this change is innocent. |
qiaojbao
pushed a commit
to GPUOpen-Drivers/llvm-project
that referenced
this pull request
Sep 30, 2024
…160c3b024 Local branch amd-gfx c1c160c Merged main:c503758ab6a4eacd3ef671a4a5ccf813995d4456 into amd-gfx:17cdfa7e29b1 Remote branch main b103037 [Coroutines] [NFCI] Dont search the DILocalVariable for __promise when constructing the debug varaible for __coro_frame (llvm#105626)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As the title mentioned, do not search for the DILocalVariable for __promise when constructing the debug variable for __coro_frame.
This should make sense because the debug variable of
__coro_frameshouldn't dependent on the debug variable of__promise. And actually, it is not. Currently, we search the debug variable for__promiseonly because we want to get the debug location and the debug scope for the__promise. However, we can construct the debug location directly from the debug scope of the being compiled function. Then it is not necessary any more to search the__promisevariable.And this patch makes the codes to construct the debug variable for
__coro_frameto be more stable. Now we will always be able to construct the debug variable for the coroutine frame no matter if we found the debug variable for the __promise or not.This patch is not strictly NFC but it is intended to not affect any end users.