[MachineDebugify] Insert synthetic DBG_VALUE instructions

[vsk]

Summary

Teach MachineDebugify how to insert DBG_VALUE instructions. This can
help find bugs causing CodeGen differences when debug info is present.
DBG_VALUE instructions are only emitted when -debugify-level is set to
locations+variables.

There is essentially no attempt made to match up DBG_VALUE register
operands with the local variables they ought to correspond to. I'm not
sure how to improve the situation. In some cases (MachineMemOperand?)
it's possible to find the IR instruction a MachineInstr corresponds to,
but in general this seems to call for "undoing" the work done by ISel.

[vsk]

Do not insert DBG_VALUEs after phis, this is illegal. Test coverage for this comes from legalize-phi-insertpt-decrement.mir.

[dsanders]

There is essentially no attempt made to match up DBG_VALUE register
operands with the local variables they ought to correspond to. I'm not
sure how to improve the situation.

Yeah, and it gets harder the further down the pipeline you go. Especially since the pipeline is fairly free form and even early passes are allowed to select target instructions. I don't think it really matters too much if the IR-level dbg.value maps to the 'correct' MIR-level vreg as we're really looking for places the DBG_VALUE causes crashes or causes CodeGen changes. Eventually we might expand into checking that DBG_VALUE's are handled as opposed to accidentally lost. Even then though we're more concerned with preserving MIR level information rather than the actual mapping to IR-level

llvm/lib/CodeGen/MachineDebugify.cpp

52–61

I notice that this only generates enough information to cover the line numbers in the IR-level. That limits the vregs covered by DBG_VALUE's to the first few as there's typically more MIR instructions than IR instructions. Is it possible to cover more of the MIR vregs?

llvm/test/CodeGen/Generic/MIRDebugify/locations.mir

1

We ought to rename this test if it's going to cover the DBG_VALUE case too

[vsk]

Cover each non-phi reg def with a DBG_VALUE, per Daniel's suggestion.

I thought I fixed the last CodeGen-difference-with-debug bug in the AArch64 backend with D78157, but with this change, I've found a few more :).

[paquette]

llvm/lib/CodeGen/MachineDebugify.cpp

72–75

Maybe a bit nicer:

if (Line2Var.empty())
  continue;

for (MachineOperand &MO : MI.operands())
  ...

[vsk]

Early-exit in loop

[vsk]

One more (and I think the last!) update to the testing strategy.

Instead of limiting -mir-debugify to inserting DBG_VALUEs after register
definitions, have it insert DBG_VALUEs after /every/ real instruction. When a
register def isn't available, just describe a constant.

Also, sometimes MIR tests are written with skeletal (or no) IR. We still want
-mir-debugify test coverage in these cases, so force the IR debugify pass to
describe at least one variable per function.

This uncovers a few more CodeGen-difference-with-debug issues.

[dsanders]

LGTM

[thakis]

Looks like the tests added here fail:
http://lab.llvm.org:8011/#/builders/109/builds/4730
http://45.33.8.238/linux/35299/step_12.txt

Since the tree is in pretty bad shape right now (this is at least the 3rd consecutive breakage), I'm reverting this.

[thakis]

Sorry, I somehow thought this landed an hour ago, not many months. Will reland, apologies :/

llvm/lib/CodeGen/MachineDebugify.cpp

 //===- MachineDebugify.cpp - Attach synthetic debug info to everything ----===//
 //
 // 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 pass attaches synthetic debug info to everything. It can be used
 /// to create targeted tests for debug info preservation.
 ///
 /// This isn't intended to have feature parity with Debugify.
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/Passes.h"
+#include "llvm/CodeGen/TargetInstrInfo.h"
+#include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/DebugInfo.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Transforms/Utils/Debugify.h"
 
 #define DEBUG_TYPE "mir-debugify"
 
 using namespace llvm;
 
 namespace {
 bool applyDebugifyMetadataToMachineFunction(MachineModuleInfo &MMI,
                                             DIBuilder &DIB, Function &F) {
   MachineFunction &MF = MMI.getOrCreateMachineFunction(F);
+  const TargetInstrInfo &TII = *MF.getSubtarget().getInstrInfo();
 
   DISubprogram *SP = F.getSubprogram();
   assert(SP && "IR Debugify just created it?");
 
-  LLVMContext &Ctx = F.getParent()->getContext();
+  Module &M = *F.getParent();
+  LLVMContext &Ctx = M.getContext();
   unsigned NextLine = SP->getLine();
 
+  // Find local variables defined by debugify. Later, we can insert DBG_VALUE
+  // insts that point to these local variables.
+  Function *DbgValF = M.getFunction("llvm.dbg.value");
+  DenseMap<unsigned, DILocalVariable *> Line2Var;
+  SmallVector<MachineOperand *, 32> DefinedValues;
+  DIExpression *Expr = nullptr;
+  if (DbgValF) {
+    for (const Use &U : DbgValF->uses()) {
+      if (auto *DbgValInst = dyn_cast<DbgValueInst>(U.getUser())) {
+        Line2Var[DbgValInst->getDebugLoc().getLine()] =
+            DbgValInst->getVariable();
+        if (!Expr)
+          Expr = DbgValInst->getExpression();
+      }
+    }
+  }
+
   for (MachineBasicBlock &MBB : MF) {
     for (MachineInstr &MI : MBB) {
       // This will likely emit line numbers beyond the end of the imagined
       // source function and into subsequent ones. We don't do anything about
       // that as it doesn't really matter to the compiler where the line is in
       // the imaginary source code.
       MI.setDebugLoc(DILocation::get(Ctx, NextLine++, 1, SP));
-    }
+
+      // Keep track of defined regs. This lets us insert DBG_VALUEs later.
+      for (MachineOperand &MO : MI.operands())
+        if (MO.isReg() && MO.isDef() && MO.getReg())
+          DefinedValues.push_back(&MO);
+    }
+
+  // Insert DBG_VALUEs for all defined regs.
+  // FIXME: This uses a matching-line heuristic to pair local variables with
+  // register definitions, which is pretty bogus.
+  const MCInstrDesc &DbgValDesc = TII.get(TargetOpcode::DBG_VALUE);
+  for (MachineOperand *MO : DefinedValues) {
+    MachineInstr &MI = *MO->getParent();
+    // Just like at the IR level, it's not expected for there to be non-PHI
+    // instructions in between PHIs at the MIR level.
+    if (MI.isPHI())
+      continue;
+    DILocalVariable *LocalVar = Line2Var[MI.getDebugLoc().getLine()];
+    if (!LocalVar)
+      continue;
+    BuildMI(*MI.getParent(), std::next(MI.getIterator()), MI.getDebugLoc(),
+            DbgValDesc, /*IsIndirect=*/false, *MO, LocalVar, Expr);
   }
 
   return true;
 }
 
 /// ModulePass for attaching synthetic debug info to everything, used with the
 /// legacy module pass manager.
 struct DebugifyMachineModule : public ModulePass {

llvm/test/CodeGen/AArch64/GlobalISel/legalize-phi-insertpt-decrement.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -O0 -mtriple=aarch64-unknown-unknown -verify-machineinstrs -run-pass=legalizer %s -o - | FileCheck %s
+# RUN: llc -debugify-and-strip-all-safe -O0 -mtriple=aarch64-unknown-unknown -verify-machineinstrs -run-pass=legalizer %s -o - | FileCheck %s
 --- |
   target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
   define void @snork() {
   bb:
     br label %bb1
 
   bb1:                                              ; preds = %bb3, %bb

llvm/test/CodeGen/Generic/MIRDebugify/locations.mir

-# RUN: llc -run-pass=mir-debugify -o - %s | FileCheck --check-prefixes=ALL,VALUE %s
+# RUN: llc -run-pass=mir-debugify -o - %s | FileCheck --check-prefixes=ALL,VALUE --implicit-check-not=DBG_VALUE %s
 # RUN: llc -run-pass=mir-debugify -debugify-level=locations -o - %s | FileCheck --check-prefixes=ALL --implicit-check-not=dbg.value %s
-# RUN: llc -run-pass=mir-debugify,mir-strip-debug,mir-debugify -o - %s | FileCheck --check-prefixes=ALL,VALUE %s
+# RUN: llc -run-pass=mir-debugify,mir-strip-debug,mir-debugify -o - %s | FileCheck --check-prefixes=ALL,VALUE --implicit-check-not=DBG_VALUE %s
 --- |
   ; ModuleID = 'loc-only.ll'
   source_filename = "loc-only.ll"
   
   ; ALL-LABEL: @test
   define i32 @test(i32 %a, i32 %b) {
     %add = add i32 %a, 2
   ; ALL-NEXT:  %add = add i32 %a, 2, !dbg [[L1:![0-9]+]]
   ; VALUE-NEXT: call void @llvm.dbg.value(metadata i32 %add, metadata [[add:![0-9]+]], metadata !DIExpression()), !dbg [[L1]]
     %sub = sub i32 %add, %b
   ; ALL-NEXT: %sub = sub i32 %add, %b, !dbg [[L2:![0-9]+]]
   ; VALUE-NEXT: call void @llvm.dbg.value(metadata i32 %sub, metadata [[sub:![0-9]+]], metadata !DIExpression()), !dbg [[L2]]
   ; ALL-NEXT: ret i32 %sub, !dbg [[L3:![0-9]+]]
     ret i32 %sub
   }
 
-  ; CHECK: !llvm.dbg.cu = !{!0}
-  ; CHECK: !llvm.debugify =
-  ; CHECK: !llvm.module.flags = !{![[VERSION:[0-9]+]]}
-  ; CHECK: !0 = distinct !DICompileUnit(
-  ; CHECK: ![[VERSION]] = !{i32 2, !"Debug Info Version", i32 3}
+  ; ALL: !llvm.dbg.cu = !{!0}
+  ; ALL: !llvm.debugify =
+  ; ALL: !llvm.module.flags = !{![[VERSION:[0-9]+]]}
+  ; ALL: !0 = distinct !DICompileUnit(
+  ; ALL: ![[VERSION]] = !{i32 2, !"Debug Info Version", i32 3}
+  ; VALUE: [[VAR1:![0-9]+]] = !DILocalVariable(name: "1"
+  ; VALUE: [[VAR2:![0-9]+]] = !DILocalVariable(name: "2"
 
 ...
 ---
 name:            test
 body:             |
   bb.1 (%ir-block.0):
     %0:_(s32) = IMPLICIT_DEF
     %1:_(s32) = IMPLICIT_DEF
     %2:_(s32) = G_CONSTANT i32 2
     %3:_(s32) = G_ADD %0, %2
     %4:_(s32) = G_SUB %3, %1
     ; There's no attempt to have the locations make sense as it's an imaginary
     ; source file anyway. These first three coincide with IR-level information
     ; and therefore use metadata references.
     ; ALL: %0:_(s32) = IMPLICIT_DEF debug-location [[L1]]
+    ; VALUE: DBG_VALUE %0(s32), $noreg, [[VAR1]], !DIExpression(), debug-location [[L1]]
     ; ALL: %1:_(s32) = IMPLICIT_DEF debug-location [[L2]]
+    ; VALUE: DBG_VALUE %1(s32), $noreg, [[VAR2]], !DIExpression(), debug-location [[L2]]
     ; ALL: %2:_(s32) = G_CONSTANT i32 2, debug-location [[L3]]
     ; ALL: %3:_(s32) = G_ADD %0, %2, debug-location !DILocation(line: 4, column: 1, scope: !6)
     ; ALL: %4:_(s32) = G_SUB %3, %1, debug-location !DILocation(line: 5, column: 1, scope: !6)
 ...