[LSR] fix a issue that LoopStrengthReduction drop debug location unnecessarily

[yuanboli233]

Summary

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.

[djtodoro]

Please add a test case.

[jmorse]

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.

[yuanboli233]

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?

[yuanboli233]

Do you think this example above can serve as a test case? If so, I can post the complete example here.

[jmorse]

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.

[yuanboli233]

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?

[yuanboli233]

add the test case requested.

If it looks OK, can you help me commit it, thanks!

[dblaikie]

llvm/test/Transforms/LoopStrengthReduce/optimizemax_debugloc.ll

10–27 ↗

(On Diff #336294)

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)

[yuanboli233]

Sorry, the original format was OK, I just copy-paste and didn't check the final format. I have just fixed the format.

[yuanboli233]

@jmorse @vsk ping

May I have some feedback for the current patch? Thanks a lot for your previous feedback. It seems that we have a positive test case for the bug. Please let me know if we need any other materials to improve the patch.

[jmorse]

I think this is good to go, with some revisions to the test.

llvm/test/Transforms/LoopStrengthReduce/optimizemax_debugloc.ll

2 ↗

(On Diff #336306)

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.

3 ↗

(On Diff #336306)

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.

33 ↗

(On Diff #336306)

We usually remove function attributes (unless they're necessary) from debug-info tests, just to reduce future maintenence

[yuanboli233]

Thanks for the feedback @jmorse, here I have the updated test.

[jmorse]

LGTM

[yuanboli233]

Can you help me to commit the changes as well. I don't have commit access. Thanks, @jmorse

[yuanboli233]

update comment format and remove function attribute #0 in the test case

[jmorse]

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.

[jmorse]

Pushed up; you might get buildbot emails about tests failing, as commits are tested in batches. I'll keep an eye on those.

[yuanboli233]

@jmorse Thanks for your help!

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

if (Cond->getPredicate() == CmpInst::ICMP_EQ)
     Pred = CmpInst::getInversePredicate(Pred);
 
   // Ok, everything looks ok to change the condition into an SLT or SGE and
   // delete the max calculation.
   ICmpInst *NewCond =
     new ICmpInst(Cond, Pred, Cond->getOperand(0), NewRHS, "scmp");
 
   // Delete the max calculation instructions.
+  NewCond->setDebugLoc(Cond->getDebugLoc());
   Cond->replaceAllUsesWith(NewCond);
   CondUse->setUser(NewCond);
   Instruction *Cmp = cast<Instruction>(Sel->getOperand(0));
   Cond->eraseFromParent();
   Sel->eraseFromParent();
   if (Cmp->use_empty())
     Cmp->eraseFromParent();
   return NewCond;