Skip to content

[AMDGPU] Fix incorrect stepping in gdb for amdgcn.end.cf intrinsic. #83010

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 2, 2024
Merged

[AMDGPU] Fix incorrect stepping in gdb for amdgcn.end.cf intrinsic. #83010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea867d1
[AMDGPU] Clear dbg info on CFG intrinsic due to incorrect behaviour i…
vpykhtin Feb 26, 2024
63ec93f
minimize fix
vpykhtin Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions llvm/lib/Target/AMDGPU/SIAnnotateControlFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,12 @@ bool SIAnnotateControlFlow::closeControlFlow(BasicBlock *BB) {
// Split edge to make Def dominate Use
FirstInsertionPt = SplitEdge(DefBB, BB, DT, LI)->getFirstInsertionPt();
}
IRBuilder<>(FirstInsertionPt->getParent(), FirstInsertionPt)
.CreateCall(EndCf, {Exec});
IRBuilder<> IRB(FirstInsertionPt->getParent(), FirstInsertionPt);
// TODO: StructurizeCFG 'Flow' blocks have debug locations from the
// condition, for now just avoid copying these DebugLocs so that stepping
// out of the then/else block in a debugger doesn't step to the condition.
Comment on lines +340 to +342
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should StructurizeCFG have not preserved these locations then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but let's "revert" the change first and then what the structurizer should do there.

IRB.SetCurrentDebugLocation(DebugLoc());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only DebugLoc that is causing GDB test failures, where control flow joins in the 'Flow' block. Could you remove the other changes in this PR? I think the other debug locations should all be correct.

A comment might be nice here too, maybe something like: "StructurizeCFG 'Flow' blocks have debug locations from the condition, for now just avoid copying these DebugLocs so that stepping out of the then/else block in a debugger doesn't step to the condition."?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Emma, done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. Isn't the location we are now dropping with this patch associated with the return, not the condition?

The locations in the if_else test are:

[[DBG13]] = !DILocation(line: 1, column: 1, scope: [[DBG5]])
[[DBG14]] = !DILocation(line: 2, column: 1, scope: [[DBG5]])
[[DBG15]] = !DILocation(line: 3, column: 1, scope: [[DBG5]])
[[DBG16]] = !DILocation(line: 4, column: 1, scope: [[DBG5]])
[[DBG17]] = !DILocation(line: 5, column: 1, scope: [[DBG5]])
[[DBG18]] = !DILocation(line: 6, column: 1, scope: [[DBG5]])

Where DBG18 is associated with the return instruction.

This change drops , !dbg [[DBG18:![0-9]+]] from the end.cf call in the exit block. If the description in the comment applied wouldn't we have been emitting , !dbg [[DBG13]] or , !dbg [[DBG14]] before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slinder1 good point, probably this testcase doesn't reflect what's happening with the failing gdb test.

I'm not the right person to work on this particular issue, I just tried to set dbg location on end.cf intrinsic because it's the next instruction after asan_report call and the call uses return address (that is end.cf intrinsic) to report faulting location. I have a workaround for this - insert dummy instruction with proper dbg loc between asan_report call and end.cf intrinsic.

I suggest to submit this fix to restore original behaviour and continue to work on this later. Maybe it worth to edit the TODO comment.

IRB.CreateCall(EndCf, {Exec});
}

return true;
Expand Down
10 changes: 5 additions & 5 deletions llvm/test/CodeGen/AMDGPU/si-annotate-dbg-info.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ define amdgpu_ps i32 @if_else(i32 %0) !dbg !5 {
; OPT-NEXT: br label [[FLOW]], !dbg [[DBG16:![0-9]+]]
; OPT: exit:
; OPT-NEXT: [[RET:%.*]] = phi i32 [ [[TMP5]], [[FLOW]] ], [ 42, [[TRUE]] ], !dbg [[DBG17:![0-9]+]]
; OPT-NEXT: call void @llvm.amdgcn.end.cf.i64(i64 [[TMP8]]), !dbg [[DBG18:![0-9]+]]
; OPT-NEXT: call void @llvm.amdgcn.end.cf.i64(i64 [[TMP8]])
; OPT-NEXT: tail call void @llvm.dbg.value(metadata i32 [[RET]], metadata [[META11:![0-9]+]], metadata !DIExpression()), !dbg [[DBG17]]
; OPT-NEXT: ret i32 [[RET]], !dbg [[DBG18]]
; OPT-NEXT: ret i32 [[RET]], !dbg [[DBG18:![0-9]+]]
;
%c = icmp eq i32 %0, 0, !dbg !13
tail call void @llvm.dbg.value(metadata i1 %c, metadata !9, metadata !DIExpression()), !dbg !13
Expand Down Expand Up @@ -65,13 +65,13 @@ define amdgpu_ps void @loop_if_break(i32 %n) !dbg !19 {
; OPT: Flow:
; OPT-NEXT: [[TMP3]] = phi i32 [ [[I_NEXT]], [[LOOP_BODY]] ], [ undef, [[LOOP]] ]
; OPT-NEXT: [[TMP4:%.*]] = phi i1 [ false, [[LOOP_BODY]] ], [ true, [[LOOP]] ]
; OPT-NEXT: call void @llvm.amdgcn.end.cf.i64(i64 [[TMP2]]), !dbg [[DBG27]]
; OPT-NEXT: call void @llvm.amdgcn.end.cf.i64(i64 [[TMP2]])
; OPT-NEXT: [[TMP5]] = call i64 @llvm.amdgcn.if.break.i64(i1 [[TMP4]], i64 [[PHI_BROKEN]]), !dbg [[DBG27]]
; OPT-NEXT: [[TMP6:%.*]] = call i1 @llvm.amdgcn.loop.i64(i64 [[TMP5]]), !dbg [[DBG27]]
; OPT-NEXT: br i1 [[TMP6]], label [[EXIT:%.*]], label [[LOOP]], !dbg [[DBG27]]
; OPT: exit:
; OPT-NEXT: call void @llvm.amdgcn.end.cf.i64(i64 [[TMP5]]), !dbg [[DBG30:![0-9]+]]
; OPT-NEXT: ret void, !dbg [[DBG30]]
; OPT-NEXT: call void @llvm.amdgcn.end.cf.i64(i64 [[TMP5]])
; OPT-NEXT: ret void, !dbg [[DBG30:![0-9]+]]
;
entry:
br label %loop, !dbg !24
Expand Down