[DebugInfo][InstrRef] Don't use instruction referencing for unoptimised functions

[jmorse]

Summary

InstrRefBasedLDV is marginally slower than VarlocBasedLDV when analysing optimised code -- however, it's much slower when analysing code compiled -O0. Up to 25% slower according to [0], which is too much!

To avoid this: don't use instruction referencing for -O0 functions. In the "pure" case of unoptimised code, this won't really harm the debugging experience because most variables won't have been promoted off the stack, so can't go missing. It becomes more complicated when optimised code is inlined into functions marked optnone; however these are rare, and as -O0 doesn't run many optimisations there should be little damage to the debug experience as a result.

I've taken the opportunity to refactor testing for instruction-referencing into a MachineFunction method, which seems the most appropriate place to put it.

[0] http://llvm-compile-time-tracker.com/compare.php?from=ee8da6369225f47f85e61e1ef2807af6a8677a0d&to=3313f75407bf1f1f4dd787e5233d99cc9e42ba55&stat=instructions

[Orlando]

LGTM with 2 small inline questions.

llvm/include/llvm/CodeGen/MachineFunction.h

539–540

Nit/question. Am I right in thinking this returns the same value for all MachineFunctions?
If so, then maybe this method should be static? And in the comment I think functions should just become function or be removed. Alternatively, if I'm wrong and this can return a different value for different MachineFunctions (or may do so in the future), then I think the comment should start Returns true if the function's variable....

llvm/test/DebugInfo/X86/instr-ref-opt-levels.ll

12

Should this be O2: DBG_INSTR_REF rather than O2: DBG_VALUE?

[jmorse]

Ouch, looks like I missed several things: I was intending that functions marked optnone would have instr-ref disabled too, I've added that filter into this patch, and expanded the test case to have an optnone function that always gets a DBG_VALUE.

Additionally there were a bunch of sites where the command-line-option was being tested rather than using this utility method, I'd failed to grep properly for it, oooff. Revised patch modifies all uses of the command line option flag to use the utility method instead. I've also added a RUN line in the test for fast-isel, to check that it behaves the same way as normal isel.

llvm/include/llvm/CodeGen/MachineFunction.h

public:
 
   /// Finalise any partially emitted debug instructions. These are DBG_INSTR_REF
   /// instructions where we only knew the vreg of the value they use, not the
   /// instruction that defines that vreg. Once isel finishes, we should have
   /// enough information for every DBG_INSTR_REF to point at an instruction
   /// (or DBG_PHI).
   void finalizeDebugInstrRefs();
 
+  /// Returns true if functions variable locations should be tracked with
+  /// instruction referencing.
+  bool useDebugInstrRef() const;
+
   MachineFunction(Function &F, const LLVMTargetMachine &Target,
                   const TargetSubtargetInfo &STI, unsigned FunctionNum,
                   MachineModuleInfo &MMI);
   MachineFunction(const MachineFunction &) = delete;
   MachineFunction &operator=(const MachineFunction &) = delete;
   ~MachineFunction();
 
   /// Reset the instance as if it was just created.

llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp

 LiveDebugValues::LiveDebugValues() : MachineFunctionPass(ID) {
   initializeLiveDebugValuesPass(*PassRegistry::getPassRegistry());
   TheImpl = nullptr;
 }
 
 bool LiveDebugValues::runOnMachineFunction(MachineFunction &MF) {
   if (!TheImpl) {
     TPC = getAnalysisIfAvailable<TargetPassConfig>();
-
-    bool InstrRefBased = false;
-    if (TPC) {
-      auto &TM = TPC->getTM<TargetMachine>();
-      InstrRefBased = TM.Options.ValueTrackingVariableLocations;
-    }
+    bool InstrRefBased = MF.useDebugInstrRef();
 
     // Allow the user to force selection of InstrRef LDV.
     InstrRefBased |= ForceInstrRefLDV;
 
     if (InstrRefBased)
       TheImpl = llvm::makeInstrRefBasedLiveDebugValues();
     else
       TheImpl = llvm::makeVarLocBasedLiveDebugValues();
   }
 
   return TheImpl->ExtendRanges(MF, TPC, InputBBLimit, InputDbgValueLimit);
 }

llvm/lib/CodeGen/LiveDebugVariables.cpp

if (!EnableLDV)
     return false;
   if (!mf.getFunction().getSubprogram()) {
     removeDebugInstrs(mf);
     return false;
   }
 
   // Have we been asked to track variable locations using instruction
   // referencing?
-  bool InstrRef = false;
-  auto *TPC = getAnalysisIfAvailable<TargetPassConfig>();
-  if (TPC) {
-    auto &TM = TPC->getTM<TargetMachine>();
-    InstrRef = TM.Options.ValueTrackingVariableLocations;
-  }
+  bool InstrRef = mf.useDebugInstrRef();
 
   if (!pImpl)
     pImpl = new LDVImpl(this);
   return static_cast<LDVImpl *>(pImpl)->runOnMachineFunction(mf, InstrRef);
 }
 
 void LiveDebugVariables::releaseMemory() {
   if (pImpl)

llvm/lib/CodeGen/MachineFunction.cpp

void MachineFunction::finalizeDebugInstrRefs() {
 
   auto MakeDbgValue = [&](MachineInstr &MI) {
     const MCInstrDesc &RefII = TII->get(TargetOpcode::DBG_VALUE);
     MI.setDesc(RefII);
     MI.getOperand(1).ChangeToRegister(0, false);
     MI.getOperand(0).setIsDebug();
   };
 
-  if (!getTarget().Options.ValueTrackingVariableLocations)
+  if (!useDebugInstrRef())
     return;
 
   for (auto &MBB : *this) {
     for (auto &MI : MBB) {
       if (!MI.isDebugRef() || !MI.getOperand(0).isReg())
         continue;
 
       Register Reg = MI.getOperand(0).getReg();
for (auto &MI : MBB) {
         unsigned ID = DefMI.getDebugInstrNum();
         MI.getOperand(0).ChangeToImmediate(ID);
         MI.getOperand(1).setImm(OperandIdx);
       }
     }
   }
 }
 
+bool MachineFunction::useDebugInstrRef() const {
+  // Disable instr-ref at -O0: it's very slow (in compile time). We can still
+  // have optimized code inlined into this unoptimized code, however with
+  // fewer and less aggressive optimizations happening, coverage and accuracy
+  // should not suffer.
+  if (getTarget().getOptLevel() == CodeGenOpt::None)
+    return false;
+
+  // Don't use instr-ref if this function is marked optnone.
+  if (F.hasFnAttribute(Attribute::OptimizeNone))
+    return false;
+
+  if (getTarget().Options.ValueTrackingVariableLocations)
+    return true;
+
+  return false;
+}
+
 /// \}
 
 //===----------------------------------------------------------------------===//
 //  MachineJumpTableInfo implementation
 //===----------------------------------------------------------------------===//
 
 /// Return the size of each entry in the jump table.
 unsigned MachineJumpTableInfo::getEntrySize(const DataLayout &TD) const {

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp

if (Op) {
       auto Builder =
           BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
                   TII.get(TargetOpcode::DBG_VALUE), /*IsIndirect*/ true, *Op,
                   DI->getVariable(), DI->getExpression());
 
       // If using instruction referencing, mutate this into a DBG_INSTR_REF,
       // to be later patched up by finalizeDebugInstrRefs. Tack a deref onto
       // the expression, we don't have an "indirect" flag in DBG_INSTR_REF.
-      if (TM.Options.ValueTrackingVariableLocations && Op->isReg()) {
+      if (FuncInfo.MF->useDebugInstrRef() && Op->isReg()) {
         Builder->setDesc(TII.get(TargetOpcode::DBG_INSTR_REF));
         Builder->getOperand(1).ChangeToImmediate(0);
         auto *NewExpr =
            DIExpression::prepend(DI->getExpression(), DIExpression::DerefBefore);
         Builder->getOperand(3).setMetadata(NewExpr);
       }
     } else {
       // We can't yet handle anything else here because it would require
if (!V || isa<UndefValue>(V) || DI->hasArgList()) {
       // FIXME: This does not handle register-indirect values at offset 0.
       bool IsIndirect = false;
       auto Builder =
           BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, II, IsIndirect, Reg,
                   DI->getVariable(), DI->getExpression());
 
       // If using instruction referencing, mutate this into a DBG_INSTR_REF,
       // to be later patched up by finalizeDebugInstrRefs.
-      if (TM.Options.ValueTrackingVariableLocations) {
+      if (FuncInfo.MF->useDebugInstrRef()) {
         Builder->setDesc(TII.get(TargetOpcode::DBG_INSTR_REF));
         Builder->getOperand(1).ChangeToImmediate(0);
       }
     } else {
       // We don't know how to handle other cases, so we drop.
       LLVM_DEBUG(dbgs() << "Dropping debug info for " << *DI << "\n");
     }
     return true;

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

 /// at the given position in the given block.
 InstrEmitter::InstrEmitter(const TargetMachine &TM, MachineBasicBlock *mbb,
                            MachineBasicBlock::iterator insertpos)
     : MF(mbb->getParent()), MRI(&MF->getRegInfo()),
       TII(MF->getSubtarget().getInstrInfo()),
       TRI(MF->getSubtarget().getRegisterInfo()),
       TLI(MF->getSubtarget().getTargetLowering()), MBB(mbb),
       InsertPos(insertpos) {
-  EmitDebugInstrRefs = TM.Options.ValueTrackingVariableLocations;
+  EmitDebugInstrRefs = MF->useDebugInstrRef();
 }

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

bool SelectionDAGBuilder::EmitFuncArgumentDbgValue(
 
   MachineFunction &MF = DAG.getMachineFunction();
   const TargetInstrInfo *TII = DAG.getSubtarget().getInstrInfo();
 
   // Helper to create DBG_INSTR_REFs or DBG_VALUEs, depending on what kind
   // we've been asked to pursue.
   auto MakeVRegDbgValue = [&](Register Reg, DIExpression *FragExpr,
                               bool Indirect) {
-    if (Reg.isVirtual() && TM.Options.ValueTrackingVariableLocations) {
+    if (Reg.isVirtual() && MF.useDebugInstrRef()) {
       // For VRegs, in instruction referencing mode, create a DBG_INSTR_REF
       // pointing at the VReg, which will be patched up later.
       auto &Inst = TII->get(TargetOpcode::DBG_INSTR_REF);
       auto MIB = BuildMI(MF, DL, Inst);
       MIB.addReg(Reg, RegState::Debug);
       MIB.addImm(0);
       MIB.addMetadata(Variable);
       auto *NewDIExpr = FragExpr;

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
 
   DenseMap<unsigned, unsigned> LiveInMap;
   if (!FuncInfo->ArgDbgValues.empty())
     for (std::pair<unsigned, unsigned> LI : RegInfo->liveins())
       if (LI.second)
         LiveInMap.insert(LI);
 
   // Insert DBG_VALUE instructions for function arguments to the entry block.
-  bool InstrRef = TM.Options.ValueTrackingVariableLocations;
+  bool InstrRef = MF->useDebugInstrRef();
   for (unsigned i = 0, e = FuncInfo->ArgDbgValues.size(); i != e; ++i) {
     MachineInstr *MI = FuncInfo->ArgDbgValues[e - i - 1];
     assert(MI->getOpcode() != TargetOpcode::DBG_VALUE_LIST &&
            "Function parameters should not be described by DBG_VALUE_LIST.");
     bool hasFI = MI->getOperand(0).isFI();
     Register Reg =
         hasFI ? TRI.getFrameRegister(*MF) : MI->getOperand(0).getReg();
     if (Register::isPhysicalRegister(Reg))

llvm/test/DebugInfo/X86/instr-ref-opt-levels.ll

+; RUN: llc -mtriple=x86_64-- %s -o - -O0 -stop-before=finalize-isel \
+; RUN:      -experimental-debug-variable-locations \
+; RUN:    | FileCheck %s --check-prefix=O0 --implicit-check-not=DBG_INSTR_REF
+; RUN: llc -mtriple=x86_64-- %s -o - -O2 -stop-before=finalize-isel \
+; RUN:      -experimental-debug-variable-locations \
+; RUN:    | FileCheck %s --check-prefix=O2 --implicit-check-not=DBG_VALUE
+; RUN: llc -mtriple=x86_64-- %s -o - -stop-before=finalize-isel \
+; RUN:      -fast-isel -experimental-debug-variable-locations \
+; RUN:    | FileCheck %s --check-prefix=FASTISEL --implicit-check-not=DBG_VALUE
+
+; Test that instruction-referencing variable locations are issued at -O2, but
+; normal DBG_VALUEs are issued at -O0. This behaviour is desired as the former
+; is slow when applied to unoptimized code.
+; (Copy + pasted from ./dead-store-elimination-marks-undef.ll),
+
+; O0-LABEL: name: main
+; O0:       DBG_VALUE
+; O0-LABEL: name: fn_optnone
+; O0:       DBG_VALUE
+
+; O2-LABEL: name: main
+; O2:       DBG_INSTR_REF
+; O2-LABEL: name: fn_optnone
+; O2:       DBG_VALUE
+
+; FASTISEL-LABEL: name: main
+; FASTISEL:       DBG_INSTR_REF
+; FASTISEL-LABEL: name: fn_optnone
+; FASTISEL:       DBG_VALUE
+
+@b = common dso_local local_unnamed_addr global i32 0, align 1
+
+define dso_local i32 @main() local_unnamed_addr !dbg !7 {
+  %1 = alloca i32, align 4
+  %2 = load i32, i32* @b, align 1, !dbg !13
+  call void @llvm.dbg.value(metadata i32 %2, metadata !12, metadata !DIExpression()), !dbg !13
+  store i32 %2, i32* %1, align 4, !dbg !13
+  ret i32 0, !dbg !13
+}
+
+define dso_local i32 @fn_optnone() local_unnamed_addr #0 !dbg !27 {
+  %1 = alloca i32, align 4
+  %2 = load i32, i32* @b, align 1, !dbg !33
+  call void @llvm.dbg.value(metadata i32 %2, metadata !32, metadata !DIExpression()), !dbg !33
+  store i32 %2, i32* %1, align 4, !dbg !33
+  ret i32 0, !dbg !33
+}
+
+attributes #0 = {optnone noinline}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !2, nameTableKind: None)
+!1 = !DIFile(filename: "dead-store-elimination-marks-undef.ll", directory: "/temp/bz45080")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = !{!"clang version 10.0.0"}
+!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!12}
+!12 = !DILocalVariable(name: "l_2864", scope: !7, file: !1, line: 4, type: !10)
+!13 = !DILocation(line: 5, column: 12, scope: !7)
+!27 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !31)
+!31 = !{!32}
+!32 = !DILocalVariable(name: "l_2864", scope: !27, file: !1, line: 4, type: !10)
+!33 = !DILocation(line: 5, column: 12, scope: !27)