[DebugInfo] Emit DBG_VALUE_LIST from ISel

[StephenTozer]

Summary

This patch continues implementing the ISel support for multi-location-operand debug values by enabling the emission of DBG_VALUE_LIST for variadic SDDbgValues. This patch does not add support for processing dbg.value intrinsics; that is added in a separate patch in this stack.

This patch is relatively small, with most of the patch being the extraction of location operand emission from EmitDbgValue to AddDbgValueLocationOps, so that it can be used for both the variadic set of location ops in DBG_VALUE_LIST as well as the single location op in DBG_VALUE. Outside of that, the only behaviour change is that the scheduler has a lambda added, HasUnknownVReg, to prevent us from attempting to emit a DBG_VALUE_LIST before all of its used VRegs have become available.

[StephenTozer]

Rebased onto recent master; no functional changes.

[aprantl]

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

31

Is there a way to assert while still getting the benefit of Clang's switch-doesnt-cover-all-enum-cases warning here?

[dblaikie]

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

31

Roughly speaking: Not really, no.

If you have a fully covered switch, you'll get a warning if you have a default (-Wcovered-switch-default). Generally unless you're reading from user input, you shouldn't worry too much about having invalid enum values - at least most of LLVM doesn't try to have defensive asserts for these sort of spoiled enum values.

[djtodoro]

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

40

MIB.addReg(0U, RegState::Debug);

[aprantl]

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

31

Thanks for clarifying — I guess the subtext here is that we could just remove the assertion and rely on the compiler warning instead, right?

[dblaikie]

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

31

Perhaps more than that - if it's a fully covered switch over an enum, you can't put a default in it without breaking the -Werror build. (this was a long-standing LLVM style nit before I implemented the -Wcovered-switch-default warning so we didn't have to identify this issue manually, but it's still in https://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations ).

So, yeah: If the switch covers all the enum values, do not include a default.

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.h

 #include "llvm/CodeGen/SelectionDAGNodes.h"
 
 namespace llvm {
 
 class MachineInstrBuilder;
 class MCInstrDesc;
 class SDDbgLabel;
 class SDDbgValue;
+class SDDbgOperand;
 class TargetLowering;
 class TargetMachine;
 
 class LLVM_LIBRARY_VISIBILITY InstrEmitter {
   MachineFunction *MF;
   MachineRegisterInfo *MRI;
   const TargetInstrInfo *TII;
   const TargetRegisterInfo *TRI;
class LLVM_LIBRARY_VISIBILITY InstrEmitter {
   void EmitRegSequence(SDNode *Node, DenseMap<SDValue, Register> &VRBaseMap,
                        bool IsClone, bool IsCloned);
 public:
   /// CountResults - The results of target nodes have register or immediate
   /// operands first, then an optional chain, and optional flag operands
   /// (which do not go into the machine instrs.)
   static unsigned CountResults(SDNode *Node);
 
+  void AddDbgValueLocationOps(MachineInstrBuilder &MIB,
+                              const MCInstrDesc &DbgValDesc,
+                              ArrayRef<SDDbgOperand> Locations,
+                              DenseMap<SDValue, Register> &VRBaseMap);
+
   /// EmitDbgValue - Generate machine instruction for a dbg_value node.
   ///
   MachineInstr *EmitDbgValue(SDDbgValue *SD,
                              DenseMap<SDValue, Register> &VRBaseMap);
 
   /// Attempt to emit a dbg_value as a DBG_INSTR_REF. May fail and return
   /// nullptr, in which case we fall back to plain EmitDbgValue.
   MachineInstr *EmitDbgInstrRef(SDDbgValue *SD,
InstrEmitter(const TargetMachine &TM, MachineBasicBlock *mbb,
                MachineBasicBlock::iterator insertpos);
 
 private:
   void EmitMachineNode(SDNode *Node, bool IsClone, bool IsCloned,
                        DenseMap<SDValue, Register> &VRBaseMap);
   void EmitSpecialNode(SDNode *Node, bool IsClone, bool IsCloned,
                        DenseMap<SDValue, Register> &VRBaseMap);
 };
-
-}
+} // namespace llvm
 
 #endif

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

 #include "llvm/CodeGen/StackMaps.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetLowering.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/PseudoProbe.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Target/TargetMachine.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "instr-emitter"
 
 /// MinRCSize - Smallest register class we allow when constraining virtual
 /// registers.  If satisfying all register class constraints would require
 /// using a smaller register class, emit a COPY to a new virtual register
 /// instead.
 const unsigned MinRCSize = 4;
 
 /// CountResults - The results of target nodes have register or immediate
 /// operands first, then an optional chain, and optional glue operands (which do
 /// not go into the resulting MachineInstr).
 unsigned InstrEmitter::CountResults(SDNode *Node) {
   unsigned N = Node->getNumValues();
InstrEmitter::EmitDbgValue(SDDbgValue *SD,
   MDNode *Var = SD->getVariable();
   MDNode *Expr = SD->getExpression();
   DebugLoc DL = SD->getDebugLoc();
   assert(cast<DILocalVariable>(Var)->isValidLocationForIntrinsic(DL) &&
          "Expected inlined-at fields to agree");
 
   SD->setIsEmitted();
 
+  ArrayRef<SDDbgOperand> LocationOps = SD->getLocationOps();
+  assert(!LocationOps.empty() && "dbg_value with no location operands?");
+
+  // Emit variadic dbg_value nodes as DBG_VALUE_LIST.
+  if (SD->isVariadic()) {
+    // DBG_VALUE_LIST := "DBG_VALUE_LIST" var, expression, loc (, loc)*
+    const MCInstrDesc &DbgValDesc = TII->get(TargetOpcode::DBG_VALUE_LIST);
+    // Build the DBG_VALUE_LIST instruction base.
+    auto MIB = BuildMI(*MF, DL, DbgValDesc);
+    MIB.addMetadata(Var);
+    MIB.addMetadata(Expr);
+    if (SD->isInvalidated()) {
+      // At least one node is no longer computed so we are unable to emit the
+      // location. Simply mark all of the operands as undef.
+      for (size_t i = 0; i < LocationOps.size(); ++i)
+        MIB.addReg(0U, RegState::Debug);
+    } else {
+      AddDbgValueLocationOps(MIB, DbgValDesc, LocationOps, VRBaseMap);
+    }
+    return &*MIB;
+  }
+
   if (SD->isInvalidated()) {
     // An invalidated SDNode must generate an undef DBG_VALUE: although the
     // original value is no longer computed, earlier DBG_VALUEs live ranges
     // must not leak into later code.
     auto MIB = BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE));
     MIB.addReg(0U);
     MIB.addReg(0U, RegState::Debug);
     MIB.addMetadata(Var);
     MIB.addMetadata(Expr);
     return &*MIB;
   }
 
   // Attempt to produce a DBG_INSTR_REF if we've been asked to.
+  // We currently exclude the possibility of instruction references for
+  // variadic nodes; if at some point we enable them, this should be moved
+  // above the variadic block.
   if (EmitDebugInstrRefs)
     if (auto *InstrRef = EmitDbgInstrRef(SD, VRBaseMap))
       return InstrRef;
 
-  SDDbgOperand SDOperand = SD->getLocationOps()[0];
-  if (SDOperand.getKind() == SDDbgOperand::FRAMEIX) {
-    // Stack address; this needs to be lowered in target-dependent fashion.
-    // EmitTargetCodeForFrameDebugValue is responsible for allocation.
-    auto FrameMI = BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE))
-                       .addFrameIndex(SDOperand.getFrameIx());
-    if (SD->isIndirect())
-      // Push [fi + 0] onto the DIExpression stack.
-      FrameMI.addImm(0);
-    else
-      // Push fi onto the DIExpression stack.
-      FrameMI.addReg(0);
-    return FrameMI.addMetadata(Var).addMetadata(Expr);
-  }
-  // Otherwise, we're going to create an instruction here.
-  const MCInstrDesc &II = TII->get(TargetOpcode::DBG_VALUE);
-  MachineInstrBuilder MIB = BuildMI(*MF, DL, II);
-  if (SDOperand.getKind() == SDDbgOperand::SDNODE) {
-    SDNode *Node = SDOperand.getSDNode();
-    SDValue Op = SDValue(Node, SDOperand.getResNo());
+  // Emit non-variadic dbg_value nodes as DBG_VALUE.
+  // DBG_VALUE := "DBG_VALUE" loc, isIndirect, var, expr
+  const MCInstrDesc &DbgValDesc = TII->get(TargetOpcode::DBG_VALUE);
+  // Build the DBG_VALUE instruction base.
+  auto MIB = BuildMI(*MF, DL, DbgValDesc);
+  // Add the single location componenet 'loc'.
+  assert(LocationOps.size() == 1 &&
+         "Non variadic dbg_value should have only one location op");
+  AddDbgValueLocationOps(MIB, DbgValDesc, LocationOps, VRBaseMap);
+  // Add 'isIndirect'.
+    MIB.addImm(0U);
+    MIB.addReg(0U, RegState::Debug);
+  MIB.addMetadata(Var);
+  MIB.addMetadata(Expr);
+  return &*MIB;
+
+void InstrEmitter::AddDbgValueLocationOps(
+    MachineInstrBuilder &MIB, const MCInstrDesc &DbgValDesc,
+    ArrayRef<SDDbgOperand> LocationOps,
+    DenseMap<SDValue, Register> &VRBaseMap) {
+  for (const SDDbgOperand &Op : LocationOps) {
+    switch (Op.getKind()) {
+    case SDDbgOperand::FRAMEIX:
+      MIB.addFrameIndex(Op.getFrameIx());
+      break;
+    case SDDbgOperand::VREG:
+      MIB.addReg(Op.getVReg(), RegState::Debug);
+      break;
+    case SDDbgOperand::SDNODE: {
+      SDValue V = SDValue(Op.getSDNode(), Op.getResNo());
       // It's possible we replaced this SDNode with other(s) and therefore
-    // didn't generate code for it.  It's better to catch these cases where
+      // didn't generate code for it. It's better to catch these cases where
       // they happen and transfer the debug info, but trying to guarantee that
       // in all cases would be very fragile; this is a safeguard for any
       // that were missed.
-    DenseMap<SDValue, Register>::iterator I = VRBaseMap.find(Op);
-    if (I==VRBaseMap.end())
-      MIB.addReg(0U);       // undef
+      if (VRBaseMap.count(V) == 0)
+        MIB.addReg(0U); // undef
       else
-      AddOperand(MIB, Op, (*MIB).getNumOperands(), &II, VRBaseMap,
+        AddOperand(MIB, V, (*MIB).getNumOperands(), &DbgValDesc, VRBaseMap,
                    /*IsDebug=*/true, /*IsClone=*/false, /*IsCloned=*/false);
-  } else if (SDOperand.getKind() == SDDbgOperand::VREG) {
-    MIB.addReg(SDOperand.getVReg(), RegState::Debug);
-  } else if (SDOperand.getKind() == SDDbgOperand::CONST) {
-    const Value *V = SDOperand.getConst();
+    } break;
+    case SDDbgOperand::CONST: {
+      const Value *V = Op.getConst();
       if (const ConstantInt *CI = dyn_cast<ConstantInt>(V)) {
         if (CI->getBitWidth() > 64)
           MIB.addCImm(CI);
         else
           MIB.addImm(CI->getSExtValue());
       } else if (const ConstantFP *CF = dyn_cast<ConstantFP>(V)) {
         MIB.addFPImm(CF);
       } else if (isa<ConstantPointerNull>(V)) {
         // Note: This assumes that all nullptr constants are zero-valued.
         MIB.addImm(0);
       } else {
-      // Could be an Undef.  In any case insert an Undef so we can see what we
+        // Could be an Undef. In any case insert an Undef so we can see what we
         // dropped.
         MIB.addReg(0U);
       }
-  } else {
-    // Insert an Undef so we can see what we dropped.
-    MIB.addReg(0U);
+    } break;
+    }
   }
-
-  // Indirect addressing is indicated by an Imm as the second parameter.
-  if (SD->isIndirect())
-    MIB.addImm(0U);
-  else
-    MIB.addReg(0U, RegState::Debug);
-
-  MIB.addMetadata(Var);
-  MIB.addMetadata(Expr);
-
-  return &*MIB;
 }
 
 MachineInstr *
 InstrEmitter::EmitDbgInstrRef(SDDbgValue *SD,
                               DenseMap<SDValue, Register> &VRBaseMap) {
   // No support for instruction references for variadic values yet.
   if (SD->isVariadic())
     return nullptr;

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

 /// ProcessSDDbgValues - Process SDDbgValues associated with this node.
 static void
 ProcessSDDbgValues(SDNode *N, SelectionDAG *DAG, InstrEmitter &Emitter,
                    SmallVectorImpl<std::pair<unsigned, MachineInstr*> > &Orders,
                    DenseMap<SDValue, Register> &VRBaseMap, unsigned Order) {
   if (!N->getHasDebugValue())
     return;
 
+  /// Returns true if \p DV has any VReg operand locations which don't exist in
+  /// VRBaseMap.
+  auto HasUnknownVReg = [&VRBaseMap](SDDbgValue *DV) {
+    for (SDDbgOperand L : DV->getLocationOps()) {
+      if (L.getKind() == SDDbgOperand::SDNODE &&
+          VRBaseMap.count({L.getSDNode(), L.getResNo()}) == 0)
+        return true;
+    }
+    return false;
+  };
+
   // Opportunistically insert immediate dbg_value uses, i.e. those with the same
   // source order number as N.
   MachineBasicBlock *BB = Emitter.getBlock();
   MachineBasicBlock::iterator InsertPos = Emitter.getInsertPos();
   for (auto DV : DAG->GetDbgValues(N)) {
     if (DV->isEmitted())
       continue;
     unsigned DVOrder = DV->getOrder();
-    if (!Order || DVOrder == Order) {
-      MachineInstr *DbgMI = Emitter.EmitDbgValue(DV, VRBaseMap);
-      if (DbgMI) {
-        Orders.push_back({DVOrder, DbgMI});
-        BB->insert(InsertPos, DbgMI);
-      }
-    }
-  }
-}
+    if (Order != 0 && DVOrder != Order)
+      continue;
+    // If DV has any VReg location operands which haven't been mapped then
+    // either that node is no longer available or we just haven't visited the
+    // node yet. In the former case we should emit an undef dbg_value, but we
+    // can do it later. And for the latter we'll want to wait until all
+    // dependent nodes have been visited.
+    if (HasUnknownVReg(DV))
+      continue;
+    if (!DbgMI)
+      continue;
 
 // ProcessSourceNode - Process nodes with source order numbers. These are added
 // to a vector which EmitSchedule uses to determine how to insert dbg_value
 // instructions in the right order.
 static void
 ProcessSourceNode(SDNode *N, SelectionDAG *DAG, InstrEmitter &Emitter,
                   DenseMap<SDValue, Register> &VRBaseMap,
                   SmallVectorImpl<std::pair<unsigned, MachineInstr *>> &Orders,

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.
   for (unsigned i = 0, e = FuncInfo->ArgDbgValues.size(); i != e; ++i) {
-    MachineInstr *MI = FuncInfo->ArgDbgValues[e-i-1];
+    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))
       EntryMBB->insert(EntryMBB->begin(), MI);
     else {
       MachineInstr *Def = RegInfo->getVRegDef(Reg);
       if (Def) {
if (LDI != LiveInMap.end()) {
       const MDNode *Expr = MI->getDebugExpression();
       DebugLoc DL = MI->getDebugLoc();
       bool IsIndirect = MI->isIndirectDebugValue();
       if (IsIndirect)
         assert(MI->getOperand(1).getImm() == 0 &&
                "DBG_VALUE with nonzero offset");
       assert(cast<DILocalVariable>(Variable)->isValidLocationForIntrinsic(DL) &&
              "Expected inlined-at fields to agree");
+      assert(MI->getOpcode() != TargetOpcode::DBG_VALUE_LIST &&
+             "Didn't expect to see a DBG_VALUE_LIST here");
       // Def is never a terminator here, so it is ok to increment InsertPos.
       BuildMI(*EntryMBB, ++InsertPos, DL, TII->get(TargetOpcode::DBG_VALUE),
               IsIndirect, LDI->second, Variable, Expr);
 
       // If this vreg is directly copied into an exported register then
       // that COPY instructions also need DBG_VALUE, if it is the only
       // user of LDI->second.
       MachineInstr *CopyUseMI = nullptr;