[Transforms][Debugify] Fix "Missing line" false alarm on PHI nodes

[Nuullll]

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=49959

The "Missing line" false alarm was introduced in https://reviews.llvm.org/D75242

[xiangzhangllvm]

llvm/lib/Transforms/Utils/Debugify.cpp

140

Or we don't set debugloc for PHI.

495

Here also ship PHI

[xiangzhangllvm]

@vks, do you know why we set PHI with debug_loc but not check it before?

[vsk]

Context: In D59417 (see also llvm.org/PR37964), there was some debate about whether it's worthwhile to assign a line 0 location to a phi instruction which would otherwise not have a location. The consensus seemed to be: no, it's not worth it. So D75242 was spun off to suppress the false positive in debugify. It looks like D75242 had the unexpected/unfortunate side-effect of introducing a new FP: any unique, preserved DebugLoc attached to a phi instruction because "missing".

My two cents is that a total ban on attaching debug location/intrinsic info to phi instructions isn't the best way to fix this. That would lead to a loss in test coverage (we do see cases where passes copy debug locations from phi instructions, or where debug intrinsics refer to phi instructions). I think the current version of this patch (skip the warning on phi instructions) does the right thing. That said I'm no longer very active in debug info reviews so I'll defer to other reviewers.

[xiangzhangllvm]

Thanks for your detailed explanation, In my eyes, D75242 eliminated uncommon "warnings" (only pass generate PHI node), but generate common "warnings" (all pass with PHI node ).
Let me first +1 for this patch if no other reviewers object.
LGTM

[djtodoro]

So this patch will remove reporting warnings of missing debug loc to PHIs completely, right?
Can we at least introduce an option to keep checking the PHIs, because there are cases it can be a real issue detected by debugify.

llvm/test/DebugInfo/debugify-ignore-phi.ll

19

I think we can remove the attributes.

[xiangzhangllvm]

In D100446#2712464, @djtodoro wrote:

So this patch will remove reporting warnings of missing debug loc to PHIs completely, right?

Yes

Can we at least introduce an option to keep checking the PHIs, because there are cases it can be a real issue detected by debugify.

Of course we can. But this warning just for pass generate PHI, we usually ignore warnings if they are make sense. I think the reason @Nuullll write this patch is just that "PHI existed in too many passes, but very few pass generate PHI"

[djtodoro]

In D100446#2715753, @xiangzhangllvm wrote:

In D100446#2712464, @djtodoro wrote:

So this patch will remove reporting warnings of missing debug loc to PHIs completely, right?

Yes

Can we at least introduce an option to keep checking the PHIs, because there are cases it can be a real issue detected by debugify.

Of course we can. But this warning just for pass generate PHI, we usually ignore warnings if they are make sense. I think the reason @Nuullll write this patch is just that "PHI existed in too many passes, but very few pass generate PHI"

got it, thanks. This lgtm.

[Nuullll]

Removed useless attributes in the test.

[djtodoro]

Thanks!

[Nuullll]

Ah, this is my first time trying to make a llvm contribution.
What shall I do next to get my patch landed?

[djtodoro]

In D100446#2756413, @Nuullll wrote:

Ah, this is my first time trying to make a llvm contribution.
What shall I do next to get my patch landed?

Thanks for this!
I can commit this for you (with your name in the 'Patch by' sign) in the case you don't have commit access yet?

[Nuullll]

In D100446#2756430, @djtodoro wrote:

In D100446#2756413, @Nuullll wrote:

Ah, this is my first time trying to make a llvm contribution.
What shall I do next to get my patch landed?

Thanks for this!
I can commit this for you (with your name in the 'Patch by' sign) in the case you don't have commit access yet?

That would be great! I don't think I have commit access.

llvm/lib/Transforms/Utils/Debugify.cpp

auto insertDbgVal = [&](Instruction &TemplateInst,
                                              /*AlwaysPreserve=*/true);
       DIB.insertDbgValueIntrinsic(V, LocalVar, DIB.createExpression(), Loc,
                                   InsertBefore);
     };
 
     for (BasicBlock &BB : F) {
       // Attach debug locations.
       for (Instruction &I : BB)
         I.setDebugLoc(DILocation::get(Ctx, NextLine++, 1, SP));
 
       if (DebugifyLevel < Level::LocationsAndVariables)
         continue;
 
       // Inserting debug values into EH pads can break IR invariants.
       if (BB.isEHPad())
         continue;
 
if (SP)
       LLVM_DEBUG(dbgs() << "  Collecting subprogram: " << *SP << '\n');
 
     for (BasicBlock &BB : F) {
       // Collect debug locations (!dbg attachments).
       // TODO: Collect dbg.values.
       for (Instruction &I : BB) {
         // Skip PHIs.
         if (isa<PHINode>(I))
           continue;
 
         // Skip debug instructions.
         if (isa<DbgInfoIntrinsic>(&I))
           continue;
 
         LLVM_DEBUG(dbgs() << "  Collecting info for inst: " << I << '\n');
 
         const DILocation *Loc = I.getDebugLoc().get();
bool checkDebugifyMetadata(Module &M,
   BitVector MissingLines{OriginalNumLines, true};
   BitVector MissingVars{OriginalNumVars, true};
   for (Function &F : Functions) {
     if (isFunctionSkipped(F))
       continue;
 
     // Find missing lines.
     for (Instruction &I : instructions(F)) {
-      if (isa<DbgValueInst>(&I) || isa<PHINode>(&I))
+      if (isa<DbgValueInst>(&I))
         continue;
 
       auto DL = I.getDebugLoc();
       if (DL && DL.getLine() != 0) {
         MissingLines.reset(DL.getLine() - 1);
         continue;
       }
 
-      if (!DL) {
+      if (!isa<PHINode>(&I) && !DL) {
         dbg() << "WARNING: Instruction with empty DebugLoc in function ";
         dbg() << F.getName() << " --";
         I.print(dbg());
         dbg() << "\n";
       }
     }
 
     // Find missing variables and mis-sized debug values.

llvm/test/DebugInfo/debugify-ignore-phi.ll

+; RUN: opt -check-debugify < %s -S 2>&1 | FileCheck %s
+
+define void @test_phi(i1 %cond) !dbg !6 {
+  br i1 %cond, label %1, label %2, !dbg !11
+
+1:                                                ; preds = %0
+  br label %2, !dbg !12
+
+2:                                                ; preds = %1, %0
+  %v = phi i32 [ 0, %0 ], [ 1, %1 ], !dbg !13
+  call void @llvm.dbg.value(metadata i32 %v, metadata !9, metadata !DIExpression()), !dbg !13
+  ret void, !dbg !14
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!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: "<stdin>", directory: "/")
+!2 = !{}
+!3 = !{i32 4}
+!4 = !{i32 1}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "test_phi", linkageName: "test_phi", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+!7 = !DISubroutineType(types: !2)
+!8 = !{!9}
+!9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 3, type: !10)
+!10 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
+!11 = !DILocation(line: 1, column: 1, scope: !6)
+!12 = !DILocation(line: 2, column: 1, scope: !6)
+!13 = !DILocation(line: 3, column: 1, scope: !6)
+!14 = !DILocation(line: 4, column: 1, scope: !6)
+
+; CHECK-NOT: WARNING: Missing line 3
+; CHECK: CheckModuleDebugify: PASS