[GlobalISel][DebugInfo] salvageDebugInfo analogue for gMIR

[dzhidzhoev]

Summary

Salvage debug info of instruction that is about to be deleted as dead in Combiner pass. Currently supported instructions are COPY and G_TRUNC.

It allows to salvage debug info of some dead arguments of functions, by putting DWARF expression corresponding to the instruction being deleted into related DBG_VALUE instruction.

Here is an example of missing variables location https://godbolt.org/z/K48osb9dK. We see that arguments x, y of function foo are not available in debugger, and corresponding DBG_VALUE instructions have undefined register operand instead of variables locaton after Aarch64PreLegalizerCombiner pass. The reason is that registers where variables are located are removed as dead (with instruction G_TRUNC). We can use salvageDebugInfo analogue for gMIR to preserve debug locations of dead variables.

Statistics of llvm object files built with vs without this commit on -O2 optimization level (CMAKE_BUILD_TYPE=RelWithDebInfo, -fglobal-isel) on Aarch64 (macOS):

Number of variables with 100% of parent scope covered by DW_AT_location has been increased by 7,9%.
Number of variables with 0% coverage of parent scope has been decreased by 1,2%.
Number of variables processed by location statistics has been increased by 2,9%.
Average PC ranges coverage has been increased by 1,8 percentage points.

Coverage can be improved by supporting more instructions, or by calling salvageDebugInfo for instructions that are deleted during Combiner rules exection.

[dzhidzhoev]

Fix X86 test

[dzhidzhoev]

Statistics of llvm object files built with vs without this commit on -O2 optimization level (CMAKE_BUILD_TYPE=RelWithDebInfo, -fglobal-isel) on Aarch64 (macOS):

Number of variables with 100% coverage of parent scope has been increased by 7,9%.
Number of variables with 0% coverage of parent scope has been decreased by 1,2%.
Number of variables processed by location statistics has been increased by 2,9%.
Average PC ranges coverage has been increased by 1,8 percentage points.

[dzhidzhoev]

Ignore partially formed DBG_VALUEs being used only for testing

[jmorse]

This seems conceptually fine to me -- it's applying an existing pattern (of salvaging) in GISel where there's coverage loss. I don't feel confident enough to give a proper review though as I've never touched GISel. I would suggest though that you'll need some special handling for the DBG_VALUE "IsIndirect" flag (operand 1): when set to an integer rather than $noreg, it causes an implicit DW_OP_deref to be added at the end of compilation. If you convert the DIExpression to contain a DW_OP_stack_value, that doesn't work.

I feel obliged to point out that the "instruction referencing" re-design of variable locations on x86 [0] avoids problems like this by never referring to copies, so it never has to deal with problems like this. Implementing that for other architectures is work that I'm not able to do, alas.

[0] https://lists.llvm.org/pipermail/llvm-dev/2021-November/153653.html

[aprantl]

Nice! Is the primary implementation of salvageDebugInfoForDbgValue specific to GlobalISel, or should this be a general MIR utility that is accessible from all MIR passes?

[dzhidzhoev]

Ignore indirect DBG_VALUE instructions, addressing @jmorse comment.

[dzhidzhoev]

In D129909#3680392, @aprantl wrote:

Nice! Is the primary implementation of salvageDebugInfoForDbgValue specific to GlobalISel, or should this be a general MIR utility that is accessible from all MIR passes?

salvageDebugInfoForDbgValue is not specific to GlobalISel in fact. It could be accessible for other passes, with support of other unary/binary instructions.

[aprantl]

In D129909#3681263, @dzhidzhoev wrote:

In D129909#3680392, @aprantl wrote:

Nice! Is the primary implementation of salvageDebugInfoForDbgValue specific to GlobalISel, or should this be a general MIR utility that is accessible from all MIR passes?

salvageDebugInfoForDbgValue is not specific to GlobalISel in fact. It could be accessible for other passes, with support of other unary/binary instructions.

In that case would recommend to move it into, e.g., lib/CodeGen/MachineInstr.cpp so it is clearly marked as a generic utility that encourages integration in other passes.

[dzhidzhoev]

Rebasing & addressing @aprantl comment. Thanks!

[dzhidzhoev]

Fix build fail

[dzhidzhoev]

Fix build fail #2

[dzhidzhoev]

Move salvageDebugInfo to CodeGenCommonISel.cpp, since it could be used from GlobalISel and other codegens

[dzhidzhoev]

Fix LLVM_DEBUG error

[aprantl]

I probably would have moved it even outside of CodeGenCommonIsel into the something that is included by all of CodeGen, since it's not ISel-specific and other passes might also delete instructions, but this is fine.

llvm/test/CodeGen/AArch64/GlobalISel/salvage-debug-info-dead.mir

3

Please comment what's being tested here.

[dzhidzhoev]

Addressing @aprantl comment.

[dzhidzhoev]

Please, commit this for me, because I have no commit access.
Vladislav Dzhidzhoev <[e-mail]>

llvm/include/llvm/CodeGen/CodeGenCommonISel.h

 /// Evaluates if the specified FP class test is an inversion of a simpler test.
 /// An example is the test "inf|normal|subnormal|zero", which is an inversion
 /// of "nan".
 /// \param Test The test as specified in 'is_fpclass' intrinsic invocation.
 /// \returns The inverted test, or zero, if inversion does not produce simpler
 /// test.
 unsigned getInvertedFPClassTest(unsigned Test);
 
+/// Assuming the instruction \p MI is going to be deleted, attempt to salvage
+/// debug users of \p MI by writing the effect of \p MI in a DIExpression.
+void salvageDebugInfoForDbgValue(const MachineRegisterInfo &MRI,
+                                 MachineInstr &MI,
+                                 ArrayRef<MachineOperand *> DbgUsers);
+
 } // namespace llvm
 
 #endif // LLVM_CODEGEN_CODEGENCOMMONISEL_H

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

 void saveUsesAndErase(MachineInstr &MI, MachineRegisterInfo &MRI,
                       LostDebugLocObserver *LocObserver,
                       SmallInstListTy &DeadInstChain);
 void eraseInstrs(ArrayRef<MachineInstr *> DeadInstrs, MachineRegisterInfo &MRI,
                  LostDebugLocObserver *LocObserver = nullptr);
 void eraseInstr(MachineInstr &MI, MachineRegisterInfo &MRI,
                 LostDebugLocObserver *LocObserver = nullptr);
 
+/// Assuming the instruction \p MI is going to be deleted, attempt to salvage
+/// debug users of \p MI by writing the effect of \p MI in a DIExpression.
+void salvageDebugInfo(const MachineRegisterInfo &MRI, MachineInstr &MI);
+
 } // End namespace llvm.
 #endif

llvm/lib/CodeGen/CodeGenCommonISel.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/CodeGenCommonISel.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetOpcodes.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+
+#define DEBUG_TYPE "codegen-common"
 
 using namespace llvm;
 
 /// Add a successor MBB to ParentMBB< creating a new MachineBB for BB if SuccMBB
 /// is 0.
 MachineBasicBlock *
 StackProtectorDescriptor::addSuccessorMBB(
     const BasicBlock *BB, MachineBasicBlock *ParentMBB, bool IsLikely,
unsigned llvm::getInvertedFPClassTest(unsigned Test) {
   case fcNegZero:
   case fcFinite:
   case fcPosFinite:
   case fcNegFinite:
     return InvertedTest;
   }
   return 0;
 }
+
+static MachineOperand *getSalvageOpsForCopy(const MachineRegisterInfo &MRI,
+                                            MachineInstr &Copy) {
+  assert(Copy.getOpcode() == TargetOpcode::COPY && "Must be a COPY");
+
+  return &Copy.getOperand(1);
+}
+
+static MachineOperand *getSalvageOpsForTrunc(const MachineRegisterInfo &MRI,
+                                            MachineInstr &Trunc,
+                                            SmallVectorImpl<uint64_t> &Ops) {
+  assert(Trunc.getOpcode() == TargetOpcode::G_TRUNC && "Must be a G_TRUNC");
+
+  const auto FromLLT = MRI.getType(Trunc.getOperand(1).getReg());
+  const auto ToLLT = MRI.getType(Trunc.defs().begin()->getReg());
+
+  // TODO: Support non-scalar types.
+  if (!FromLLT.isScalar()) {
+    return nullptr;
+  }
+
+  auto ExtOps = DIExpression::getExtOps(FromLLT.getSizeInBits(),
+                                        ToLLT.getSizeInBits(), false);
+  Ops.append(ExtOps.begin(), ExtOps.end());
+  return &Trunc.getOperand(1);
+}
+
+static MachineOperand *salvageDebugInfoImpl(const MachineRegisterInfo &MRI,
+                                            MachineInstr &MI,
+                                            SmallVectorImpl<uint64_t> &Ops) {
+  switch (MI.getOpcode()) {
+  case TargetOpcode::G_TRUNC:
+    return getSalvageOpsForTrunc(MRI, MI, Ops);
+  case TargetOpcode::COPY:
+    return getSalvageOpsForCopy(MRI, MI);
+  default:
+    return nullptr;
+  }
+}
+
+void llvm::salvageDebugInfoForDbgValue(const MachineRegisterInfo &MRI,
+                                       MachineInstr &MI,
+                                       ArrayRef<MachineOperand *> DbgUsers) {
+  // These are arbitrary chosen limits on the maximum number of values and the
+  // maximum size of a debug expression we can salvage up to, used for
+  // performance reasons.
+  const unsigned MaxExpressionSize = 128;
+
+  for (auto *DefMO : DbgUsers) {
+    MachineInstr *DbgMI = DefMO->getParent();
+    if (DbgMI->isIndirectDebugValue()) {
+      continue;
+    }
+
+    int UseMOIdx = DbgMI->findRegisterUseOperandIdx(DefMO->getReg());
+    assert(UseMOIdx != -1 && DbgMI->hasDebugOperandForReg(DefMO->getReg()) &&
+           "Must use salvaged instruction as its location");
+
+    // TODO: Support DBG_VALUE_LIST.
+    if (DbgMI->getOpcode() != TargetOpcode::DBG_VALUE) {
+      assert(DbgMI->getOpcode() == TargetOpcode::DBG_VALUE_LIST &&
+             "Must be either DBG_VALUE or DBG_VALUE_LIST");
+      continue;
+    }
+
+    const DIExpression *SalvagedExpr = DbgMI->getDebugExpression();
+
+    SmallVector<uint64_t, 16> Ops;
+    auto Op0 = salvageDebugInfoImpl(MRI, MI, Ops);
+    if (!Op0)
+      continue;
+    SalvagedExpr = DIExpression::appendOpsToArg(SalvagedExpr, Ops, 0, true);
+
+    bool IsValidSalvageExpr =
+        SalvagedExpr->getNumElements() <= MaxExpressionSize;
+    if (IsValidSalvageExpr) {
+      auto &UseMO = DbgMI->getOperand(UseMOIdx);
+      UseMO.setReg(Op0->getReg());
+      UseMO.setSubReg(Op0->getSubReg());
+      DbgMI->getDebugExpressionOp().setMetadata(SalvagedExpr);
+
+      LLVM_DEBUG(dbgs() << "SALVAGE: " << *DbgMI << '\n');
+    }
+  }
+}

llvm/lib/CodeGen/GlobalISel/Combiner.cpp

if (CSEInfo)
       WrapperObserver.addObserver(CSEInfo);
     RAIIDelegateInstaller DelInstall(MF, &WrapperObserver);
     for (MachineBasicBlock *MBB : post_order(&MF)) {
       for (MachineInstr &CurMI :
            llvm::make_early_inc_range(llvm::reverse(*MBB))) {
         // Erase dead insts before even adding to the list.
         if (isTriviallyDead(CurMI, *MRI)) {
           LLVM_DEBUG(dbgs() << CurMI << "Is dead; erasing.\n");
+          llvm::salvageDebugInfo(*MRI, CurMI);
           CurMI.eraseFromParent();
           continue;
         }
         WorkList.deferred_insert(&CurMI);
       }
     }
     WorkList.finalize();
     // Main Loop. Process the instructions here.

llvm/lib/CodeGen/GlobalISel/Utils.cpp

 //===- llvm/CodeGen/GlobalISel/Utils.cpp -------------------------*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file This file implements the utility functions used by the GlobalISel
 /// pipeline.
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/GlobalISel/Utils.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/CodeGen/CodeGenCommonISel.h"
 #include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
 #include "llvm/CodeGen/GlobalISel/GISelKnownBits.h"
 #include "llvm/CodeGen/GlobalISel/GenericMachineInstrs.h"
 #include "llvm/CodeGen/GlobalISel/LostDebugLocObserver.h"
 #include "llvm/CodeGen/GlobalISel/MIPatternMatch.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
while (!DeadInstChain.empty()) {
     saveUsesAndErase(*Inst, MRI, LocObserver, DeadInstChain);
   }
 }
 
 void llvm::eraseInstr(MachineInstr &MI, MachineRegisterInfo &MRI,
                       LostDebugLocObserver *LocObserver) {
   return eraseInstrs({&MI}, MRI, LocObserver);
 }
+
+void llvm::salvageDebugInfo(const MachineRegisterInfo &MRI, MachineInstr &MI) {
+  for (auto &Def : MI.defs()) {
+    assert(Def.isReg() && "Must be a reg");
+
+    SmallVector<MachineOperand *, 16> DbgUsers;
+    for (auto &MOUse : MRI.use_operands(Def.getReg())) {
+      MachineInstr *DbgValue = MOUse.getParent();
+      // Ignore partially formed DBG_VALUEs.
+      if (DbgValue->isNonListDebugValue() && DbgValue->getNumOperands() == 4) {
+        DbgUsers.push_back(&MOUse);
+      }
+    }
+
+    if (!DbgUsers.empty()) {
+      salvageDebugInfoForDbgValue(MRI, MI, DbgUsers);
+    }
+  }
+}

llvm/test/CodeGen/AArch64/GlobalISel/salvage-debug-info-dead.mir

+# RUN: llc -global-isel=1 -O0 -run-pass=aarch64-O0-prelegalizer-combiner %s -o - -verify-machineinstrs | FileCheck %s --check-prefix=CHECK
+
+--- |
+  ; ModuleID = 'salvage-debug-info-dead.mir'
+  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+  target triple = "aarch64--"
+
+  @A = global i8 1234
+
+  declare external i128 @foo()
+
+  define void @main() !dbg !6 {
+    ret void
+  }
+
+  !llvm.dbg.cu = !{!2}
+  !llvm.module.flags = !{!10, !11, !12}
+  !llvm.ident = !{!20}
+
+  !1 = distinct !DIGlobalVariable(name: "A", scope: !2, file: !3, line: 1, type: !7, isLocal: false, isDefinition: true)
+  !2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !3, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+  !3 = !DIFile(filename: "test.ll", directory: "/")
+  !7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !8 = !DISubroutineType(types: !23)
+  !9 = distinct !DIGlobalVariable(name: "C", scope: !2, file: !3, line: 3, type: !7, isLocal: false, isDefinition: true)
+  !10 = !{i32 7, !"Dwarf Version", i32 4}
+  !11 = !{i32 2, !"Debug Info Version", i32 3}
+  !12 = !{i32 1, !"wchar_size", i32 4}
+  !20 = !{!"clang"}
+  !6 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 5, type: !8, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !24)
+  !23 = !{!7}
+  !24 = !{}
+  !25 = !DILocalVariable(name: "X", scope: !6, file: !3, line: 6, type: !7)
+  !42 = !DILocalVariable(name: "Y", scope: !6, file: !3, line: 12, type: !7)
+  !52 = !DILocalVariable(name: "Z", scope: !6, file: !3, line: 16, type: !7)
+  !56 = !DILocalVariable(name: "W", scope: !6, file: !3, line: 18, type: !7)
+  !57 = !DILocalVariable(name: "P", scope: !6, file: !3, line: 18, type: !7)
+...
+---
+# Check that we salvage debug information for instructions
+# deleted as dead in Combiner pass
+name: main
+tracksRegLiveness: true
+body:             |
+  ; CHECK-LABEL: name: main
+  ; CHECK: bb.0:
+  ; CHECK:   liveins: $x0
+  ; CHECK-NEXT: {{   $}}
+  ; CHECK-NEXT:   DBG_VALUE $x0, $noreg, {{.*}}, !DIExpression(DW_OP_LLVM_convert, 64, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value), debug-location !DILocation(line: 6, column: 7, scope: !6)
+  ; CHECK-NEXT:   DBG_VALUE $x0, $noreg, {{.*}}, !DIExpression(DW_OP_LLVM_convert, 64, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_convert, 16, DW_ATE_unsigned, DW_OP_stack_value), debug-location !DILocation(line: 7, column: 7, scope: !6)
+  ; CHECK-NEXT:   DBG_VALUE $x0, $noreg, {{.*}}, !DIExpression(DW_OP_LLVM_convert, 64, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_convert, 16, DW_ATE_unsigned, DW_OP_LLVM_convert, 16, DW_ATE_unsigned, DW_OP_LLVM_convert, 8, DW_ATE_unsigned, DW_OP_stack_value), debug-location !DILocation(line: 8, column: 7, scope: !6)
+  ; CHECK: bb.1:
+  ; CHECK-NEXT:   DBG_VALUE $x0, $noreg, {{.*}}, !DIExpression(DW_OP_LLVM_convert, 64, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value), debug-location !DILocation(line: 9, column: 7, scope: !6)
+  ; CHECK-NEXT:   DBG_VALUE %4:_(s32), 0, {{.*}}, !DIExpression(), debug-location !DILocation(line: 9, column: 7, scope: !6)
+  bb.0:
+    liveins: $x0
+    %0:_(s64) = COPY $x0
+    %1:_(s32) = G_TRUNC %0:_(s64)
+    DBG_VALUE %1:_(s32), $noreg, !25, !DIExpression(), debug-location !DILocation(line: 6, column: 7, scope: !6)
+
+    %2:_(s16) = G_TRUNC %1:_(s32)
+    DBG_VALUE %2:_(s16), $noreg, !42, !DIExpression(), debug-location !DILocation(line: 7, column: 7, scope: !6)
+
+    %3:_(s8) = G_TRUNC %2:_(s16)
+    DBG_VALUE %3:_(s8), $noreg, !52, !DIExpression(), debug-location !DILocation(line: 8, column: 7, scope: !6)
+
+  bb.1:
+    %4:_(s32) = G_TRUNC %0:_(s64)
+    DBG_VALUE %4:_(s32), $noreg, !56, !DIExpression(), debug-location !DILocation(line: 9, column: 7, scope: !6)
+    DBG_VALUE %4:_(s32), 0, !57, !DIExpression(), debug-location !DILocation(line: 9, column: 7, scope: !6)
+
+...

llvm/test/DebugInfo/AArch64/debug-reg-bank.ll

 ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 ; RUN: llc < %s -mtriple=aarch64-linux-gnu -O0 -global-isel -stop-after instruction-select | FileCheck %s
 
 define void @foo(i32 %n) !dbg !7 {
   ; CHECK-LABEL: name: foo
   ; CHECK: bb.1.entry:
   ; CHECK-NEXT:   liveins: $w0
   ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   DBG_VALUE %0:_, $noreg, !12, !DIExpression(), debug-location !19
+  ; CHECK-NEXT:   DBG_VALUE %{{[0-9]+}}:_, $noreg, !12, !DIExpression(), debug-location !19
   ; CHECK-NEXT:   RET_ReallyLR debug-location !20
 entry:
-  call void @llvm.dbg.value(metadata i32 %n, i64 0, metadata !12, metadata !19), !dbg !20
+  %m = mul i32 %n, 13
+  call void @llvm.dbg.value(metadata i32 %m, i64 0, metadata !12, metadata !19), !dbg !20
   ret void, !dbg !21
 }
 
 declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4, !5}
 !llvm.ident = !{!6}

llvm/test/DebugInfo/X86/debug-reg-bank.ll

 ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 ; RUN: llc < %s -mtriple=x86_64-linux-gnu -O0 -global-isel -stop-after instruction-select | FileCheck %s
 
 define void @foo(i32 %n) !dbg !7 {
   ; CHECK-LABEL: name: foo
   ; CHECK: bb.1.entry:
   ; CHECK-NEXT:   liveins: $edi
   ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   DBG_VALUE %0:gpr, $noreg, !12, !DIExpression(), debug-location !19
+  ; CHECK-NEXT:   DBG_VALUE %{{[0-9]+}}:_, $noreg, !12, !DIExpression(), debug-location !19
   ; CHECK-NEXT:   RET 0, debug-location !20
 entry:
-  call void @llvm.dbg.value(metadata i32 %n, i64 0, metadata !12, metadata !19), !dbg !20
+  %m = mul i32 %n, 13
+  call void @llvm.dbg.value(metadata i32 %m, i64 0, metadata !12, metadata !19), !dbg !20
   ret void, !dbg !21
 }
 
 declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4, !5}
 !llvm.ident = !{!6}