[Debugify] Use DebugifyLevel in Debugify original mode

[djtodoro]

Summary

Before this patch the DebugifyLevel option was used for the synthetic mode, so after this, it will be used in the original mode as well.

[Orlando]

Hi @djtodoro. The patch SGTM but I do have one concern/question regarding the test (inline comment).

llvm/lib/Transforms/Utils/Debugify.cpp

323–336

nit: missing o in Collect and s in dbg.declares.

llvm/test/Transforms/Util/Debugify/loc-only-original-mode.ll

7

This test feels prone to rot because it will pass even if the feature is not working as intended if A) the pass stops dropping intrinsics or B) the warning message is changed. I'm not sure what we could do instead though - do you have any ideas?

[StephenTozer]

Aside from the mentioned nit, LGTM.

Normally I'd say that the test should also check that the debug values are actually being dropped, to ensure that the test doesn't silently become invalid if some future change causes the debug values to not be dropped. However, I don't think any future change would prevent the debug values from being dropped by deadargelim, so it should be safe I think.

llvm/lib/Transforms/Utils/Debugify.cpp

323–336

Accidentally removed the 'o' and 's'?

[StephenTozer]

Apologies, didn't see the existing comments on this review before submitting - given that the concern has been raised by another, it may be worth making a change to confirm that debug values have been dropped, and also check the error message. A simple way to do this might be to run the test twice, using -debugify-level=locations-and-variables for the second run, with a directive CHECK: drops dbg.value()/dbg.declare() - this should solve both issues by ensuring the test only passes if we are actually dropping debug info, and we are checking for the correct error message.

[Orlando]

In D115623#3194654, @StephenTozer wrote:

Apologies, didn't see the existing comments on this review before submitting - given that the concern has been raised by another, it may be worth making a change to confirm that debug values have been dropped, and also check the error message. A simple way to do this might be to run the test twice, using -debugify-level=locations-and-variables for the second run, with a directive CHECK: drops dbg.value()/dbg.declare() - this should solve both issues by ensuring the test only passes if we are actually dropping debug info, and we are checking for the correct error message.

FWIW this suggestion sounds like a good solution to me.

[djtodoro]

Sorry for delay here...

@Orlando @StephenTozer Thanks a lot for your comments!

[StephenTozer]

Thanks for the change, LGTM

llvm/lib/Transforms/Utils/Debugify.cpp

 cl::opt<bool> Quiet("debugify-quiet",
                     cl::desc("Suppress verbose debugify output"));
 
 enum class Level {
   Locations,
   LocationsAndVariables
 };
 
-// Used for the synthetic mode only.
 cl::opt<Level> DebugifyLevel(
     "debugify-level", cl::desc("Kind of debug info to add"),
     cl::values(clEnumValN(Level::Locations, "locations", "Locations only"),
                clEnumValN(Level::LocationsAndVariables, "location+variables",
                           "Locations and Variables")),
     cl::init(Level::LocationsAndVariables));
 
 raw_ostream &dbg() { return Quiet ? nulls() : errs(); }
for (Function &F : Functions) {
 
     for (BasicBlock &BB : F) {
       // Collect debug locations (!dbg) and debug variable intrinsics.
       for (Instruction &I : BB) {
         // Skip PHIs.
         if (isa<PHINode>(I))
           continue;
 
-        // Collect dbg.values and dbg.declares.
+        // Cllect dbg.values and dbg.declare.
+        if (DebugifyLevel > Level::Locations) {
           if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&I)) {
             if (!SP)
               continue;
             // Skip inlined variables.
             if (I.getDebugLoc().getInlinedAt())
               continue;
             // Skip undef values.
             if (DVI->isUndef())
               continue;
 
             auto *Var = DVI->getVariable();
             DebugInfoBeforePass.DIVariables[Var]++;
             continue;
           }
+        }
 
         // Skip debug instructions other than dbg.value and dbg.declare.
         if (isa<DbgInfoIntrinsic>(&I))
           continue;
 
         LLVM_DEBUG(dbgs() << "  Collecting info for inst: " << I << '\n');
         DebugInfoBeforePass.InstToDelete.insert({&I, &I});
 
for (Function &F : Functions) {
     for (BasicBlock &BB : F) {
       // Collect debug locations (!dbg) and debug variable intrinsics.
       for (Instruction &I : BB) {
         // Skip PHIs.
         if (isa<PHINode>(I))
           continue;
 
         // Collect dbg.values and dbg.declares.
+        if (DebugifyLevel > Level::Locations) {
           if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&I)) {
             if (!SP)
               continue;
             // Skip inlined variables.
             if (I.getDebugLoc().getInlinedAt())
               continue;
             // Skip undef values.
             if (DVI->isUndef())
               continue;
 
             auto *Var = DVI->getVariable();
             DebugInfoAfterPass.DIVariables[Var]++;
             continue;
           }
+        }
 
         // Skip debug instructions other than dbg.value and dbg.declare.
         if (isa<DbgInfoIntrinsic>(&I))
           continue;
 
         LLVM_DEBUG(dbgs() << "  Collecting info for inst: " << I << '\n');
 
         const DILocation *Loc = I.getDebugLoc().get();

llvm/test/Transforms/Util/Debugify/loc-only-original-mode.ll

+; RUN: opt < %s -deadargelim -enable-new-pm=false \
+; RUN:     -verify-each-debuginfo-preserve \
+; RUN:     -debugify-level=locations -S 2>&1 | FileCheck %s
+
+; RUN: opt < %s -deadargelim -enable-new-pm=false \
+; RUN:     -verify-each-debuginfo-preserve \
+; RUN:     -debugify-level=location+variables -S 2>&1 | FileCheck %s --check-prefix=CHECK-DROP
+
+; CHECK-NOT: drops dbg.value()/dbg.declare()
+; CHECK-DROP: drops dbg.value()/dbg.declare()
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define dso_local i32 @fn2(i32 %l, i32 %k) !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata i32 %l, metadata !12, metadata !DIExpression()), !dbg !15
+  call void @llvm.dbg.value(metadata i32 %k, metadata !13, metadata !DIExpression()), !dbg !15
+  %call = call i32 (...) @fn3(), !dbg !16
+  call void @llvm.dbg.value(metadata i32 %call, metadata !14, metadata !DIExpression()), !dbg !15
+  ret i32 %call, !dbg !17
+}
+
+declare !dbg !18 dso_local i32 @fn3(...)
+
+define dso_local i32 @fn(i32 %x, i32 %y) !dbg !22 {
+entry:
+  call void @llvm.dbg.value(metadata i32 %x, metadata !24, metadata !DIExpression()), !dbg !27
+  call void @llvm.dbg.value(metadata i32 %y, metadata !25, metadata !DIExpression()), !dbg !27
+  %call = call i32 @fn2(i32 %x, i32 %y), !dbg !27
+  call void @llvm.dbg.value(metadata i32 %call, metadata !26, metadata !DIExpression()), !dbg !27
+  %add = add nsw i32 %call, %x, !dbg !27
+  %add1 = add nsw i32 %add, %y, !dbg !27
+  ret i32 %add1, !dbg !27
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 14.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/dir")
+!2 = !{i32 7, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 7, !"uwtable", i32 1}
+!6 = !{!"clang version 14.0.0"}
+!7 = distinct !DISubprogram(name: "fn2", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0,
+ retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!12, !13, !14}
+!12 = !DILocalVariable(name: "l", arg: 1, scope: !7, file: !1, line: 3, type: !10)
+!13 = !DILocalVariable(name: "k", arg: 2, scope: !7, file: !1, line: 3, type: !10)
+!14 = !DILocalVariable(name: "s", scope: !7, file: !1, line: 4, type: !10)
+!15 = !DILocation(line: 0, scope: !7)
+!16 = !DILocation(line: 4, column: 11, scope: !7)
+!17 = !DILocation(line: 5, column: 3, scope: !7)
+!18 = !DISubprogram(name: "fn3", scope: !1, file: !1, line: 1, type: !19, spFlags: DISPFlagOptimized, retainedNodes: !21)
+!19 = !DISubroutineType(types: !20)
+!20 = !{!10}
+!21 = !{}
+!22 = distinct !DISubprogram(name: "fn", scope: !1, file: !1, line: 8, type: !8, scopeLine: 8, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0,
+ retainedNodes: !23)
+!23 = !{!24, !25, !26}
+!24 = !DILocalVariable(name: "x", arg: 1, scope: !22, file: !1, line: 8, type: !10)
+!25 = !DILocalVariable(name: "y", arg: 2, scope: !22, file: !1, line: 8, type: !10)
+!26 = !DILocalVariable(name: "local", scope: !22, file: !1, line: 9, type: !10)
+!27 = !DILocation(line: 0, scope: !22)