A proposed bug fix for a bug report: https://bugs.llvm.org/show_bug.cgi?id=48067
Preserve the original debug location for the newly created comparison instruction.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
In principle this looks good, but as @djtodoro points out it needs a test.
Note also the concern I had in PR48067, OptimizeLoopTermCond might move the newly created instruction around, which needs to obey the source-location-update rules. If the test case could cover this scenario too, that'd be great coverage to gain, and show that it's doing-the-right-thing.
Comment Actions
As @jmorse pointed out, OptimizeLoopTermCond might move the newly created instruction around. I was working on an example to trigger an instruction move and the replacement of the conditional. Here I have an example, I show part of the code here:
define void @foobar(i32 %n) !dbg !6 {
bb.nph:
%cond = icmp eq i32 %n, 0, !dbg !16
call void @llvm.dbg.value(metadata i1 %cond, metadata !9, metadata !DIExpression()), !dbg !16
%umax = select i1 %cond, i32 1, i32 %n, !dbg !17
call void @llvm.dbg.value(metadata i32 %umax, metadata !11, metadata !DIExpression()), !dbg !17
br label %bb, !dbg !18
bb: ; preds = %bb, %bb.nph
%i.03 = phi i32 [ 0, %bb.nph ], [ %indvar.next, %bb ], !dbg !19
call void @llvm.dbg.value(metadata i32 %i.03, metadata !13, metadata !DIExpression()), !dbg !19
%indvar.next = add nuw nsw i32 %i.03, 1, !dbg !20
call void @llvm.dbg.value(metadata i32 %indvar.next, metadata !14, metadata !DIExpression()), !dbg !20
%exitcond = icmp eq i32 %indvar.next, %umax, !dbg !21
call void @llvm.dbg.value(metadata i1 %exitcond, metadata !15, metadata !DIExpression()), !dbg !21
br i1 %exitcond, label %return, label %bb, !dbg !22
return: ; preds = %bb
ret void, !dbg !23
}is optimized to
define void @foobar(i32 %n) !dbg !6 {
bb.nph:
call void @llvm.dbg.value(metadata i1 undef, metadata !9, metadata !DIExpression()), !dbg !16
call void @llvm.dbg.value(metadata i32 undef, metadata !11, metadata !DIExpression()), !dbg !17
br label %bb, !dbg !18
bb: ; preds = %bb, %bb.nph
%i.03 = phi i32 [ 0, %bb.nph ], [ %indvar.next, %bb ], !dbg !19
call void @llvm.dbg.value(metadata i32 %i.03, metadata !13, metadata !DIExpression()), !dbg !19
%indvar.next = add nuw nsw i32 %i.03, 1, !dbg !20
call void @llvm.dbg.value(metadata i32 %indvar.next, metadata !14, metadata !DIExpression()), !dbg !20
call void @llvm.dbg.value(metadata i1 %scmp, metadata !15, metadata !DIExpression()), !dbg !21
%scmp = icmp uge i32 %indvar.next, %n, !dbg !21
br i1 %scmp, label %return, label %bb, !dbg !22
return: ; preds = %bb
ret void, !dbg !23
}The %exitcond is replaced by %scmp, and %scmp is moved to the second last instruction of the block. Maybe the debug location should probably still be kept, but I am not very sure about it. Even the debug location should not be kept, it probably should be dropped during the instruction move in the OptimizeLoopTermCond function. May I know your thoughts about this specific case?
Comment Actions
Do you think this example above can serve as a test case? If so, I can post the complete example here.
Comment Actions
Happily we can consult https://llvm.org/docs/HowToUpdateDebugInfo.html -- the example that you've given looks like's a good test for the "positive" case, where we should keep the source location attached to the compare instruction. The comparison doesn't get moved between blocks, so we match "When to preserve an instruction location".
Ideally we should have a test for the negative case, i.e. a scenario where the source location _should_ be dropped. That would happen if we moved the comparison between blocks, across a conditional branch. That's what I was originally concerned about earlier... however after some study I'm less certain now that this can ever occur with loop strength reduction. I put some assertions into LoopStrengthReduce.cpp to look for those scenarios happening and did a stage2 build of clang, and it never detected any. Plus, LoopStrengthReduction only operates on loops that have been transformed to only have one backedge. Thus, I reckon we don't need a negative test here.
Comment Actions
Thanks a lot for the feedback!
I think the test case I mentioned could serve as a test. Here is the complete test case:
; RUN: opt < %s -loop-reduce -S 2>&1 | FileCheck %s
; ModuleID = 'simplified-dbg.bc'
source_filename = "abc.ll"
target datalayout = "n8:16:32:64"
target triple = "x86_64-unknown-linux-gnu"
define void @foobar(i32 %n) !dbg !6 {
bb.nph:
%cond = icmp eq i32 %n, 0, !dbg !16
call void @llvm.dbg.value(metadata i1 %cond, metadata !9, metadata !DIExpression()), !dbg !16
%umax = select i1 %cond, i32 1, i32 %n, !dbg !17
call void @llvm.dbg.value(metadata i32 %umax, metadata !11, metadata !DIExpression()), !dbg !17
br label %bb, !dbg !18
bb: ; preds = %bb, %bb.nph
%i.03 = phi i32 [ 0, %bb.nph ], [ %indvar.next, %bb ], !dbg !19
call void @llvm.dbg.value(metadata i32 %i.03, metadata !13, metadata !DIExpression()), !dbg !19
%indvar.next = add nuw nsw i32 %i.03, 1, !dbg !20
call void @llvm.dbg.value(metadata i32 %indvar.next, metadata !14, metadata !DIExpression()), !dbg !20
; CHECK: scmp =
; CHECK-SAME: 21
%exitcond = icmp eq i32 %indvar.next, %umax, !dbg !21
call void @llvm.dbg.value(metadata i1 %exitcond, metadata !15, metadata !DIExpression()), !dbg !21
br i1 %exitcond, label %return, label %bb, !dbg !22
return: ; preds = %bb
ret void, !dbg !23
}
; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
declare void @llvm.dbg.value(metadata, metadata, metadata) #0
attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
!llvm.dbg.cu = !{!0}
!llvm.debugify = !{!3, !4}
!llvm.module.flags = !{!5}
!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
!1 = !DIFile(filename: "simplified.ll", directory: "/")
!2 = !{}
!3 = !{i32 8}
!4 = !{i32 5}
!5 = !{i32 2, !"Debug Info Version", i32 3}
!6 = distinct !DISubprogram(name: "foobar", linkageName: "foobar", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
!7 = !DISubroutineType(types: !2)
!8 = !{!9, !11, !13, !14, !15}
!9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 1, type: !10)
!10 = !DIBasicType(name: "ty8", size: 8, encoding: DW_ATE_unsigned)
!11 = !DILocalVariable(name: "2", scope: !6, file: !1, line: 2, type: !12)
!12 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
!13 = !DILocalVariable(name: "3", scope: !6, file: !1, line: 4, type: !12)
!14 = !DILocalVariable(name: "4", scope: !6, file: !1, line: 5, type: !12)
!15 = !DILocalVariable(name: "5", scope: !6, file: !1, line: 6, type: !10)
!16 = !DILocation(line: 1, column: 1, scope: !6)
!17 = !DILocation(line: 2, column: 1, scope: !6)
!18 = !DILocation(line: 3, column: 1, scope: !6)
!19 = !DILocation(line: 4, column: 1, scope: !6)
!20 = !DILocation(line: 5, column: 1, scope: !6)
!21 = !DILocation(line: 6, column: 1, scope: !6)
!22 = !DILocation(line: 7, column: 1, scope: !6)
!23 = !DILocation(line: 8, column: 1, scope: !6)I am not sure how I can add the test case, should I update the diff here?
| llvm/test/Transforms/LoopStrengthReduce/optimizemax_debugloc.ll | ||
|---|---|---|
| 11–28 | Could use some reformatting (if you pasted this into vim, for instance, you can "set paste" so when you paste the code it doesn't try to indent) | |
Comment Actions
Sorry, the original format was OK, I just copy-paste and didn't check the final format. I have just fixed the format.
Comment Actions
I think this is good to go, with some revisions to the test.
| llvm/test/Transforms/LoopStrengthReduce/optimizemax_debugloc.ll | ||
|---|---|---|
| 3 | The CHECK line should try to closely match the correct conditions for the test to pass in -- you don't need to regex the SSA-Value names as they're very stable, and don't often change in small tests like these. Instead, you should check that the correct metadata is attached ("!dbg !21"). The metadata numbers can often change when LLVM alters how it uses metadata, so you shouldn't use literal numbers in the CHECK line. Instead, you should capture the number (see the FileCheck documentation, or other debug-info tests) and then check that there's a DILocation metadata node with the same metadata number. | |
| 4 | Could you include a top level comment explaining what this is testing for -- it makes it easier for people in the future to understand what the intention of the test was. | |
| 34 | We usually remove function attributes (unless they're necessary) from debug-info tests, just to reduce future maintenence | |
Comment Actions
Can you help me to commit the changes as well. I don't have commit access. Thanks, @jmorse
Comment Actions
Can you help me to commit the changes as well. I don't have commit access. Thanks, @jmorse
Sure, although I'll do that on Monday so that there's enough time in the day to revert it if there are difficulties.
Comment Actions
Pushed up; you might get buildbot emails about tests failing, as commits are tested in batches. I'll keep an eye on those.
The CHECK line should try to closely match the correct conditions for the test to pass in -- you don't need to regex the SSA-Value names as they're very stable, and don't often change in small tests like these.
Instead, you should check that the correct metadata is attached ("!dbg !21"). The metadata numbers can often change when LLVM alters how it uses metadata, so you shouldn't use literal numbers in the CHECK line. Instead, you should capture the number (see the FileCheck documentation, or other debug-info tests) and then check that there's a DILocation metadata node with the same metadata number.