[WIP][DebugInfo] Avoid SelectionDAG un-necessarily debug-referring to dead VRegs

[jmorse]

Summary

[Not for review or submission yet, aim is to evaluate this first]

Hi Bjorn,

This patch hopefully alleviates some of the regressions seen with D56151 / PR40010, where unfortunate DBG_VALUE creation by SelectionDAG generates debug uses of dead VRegs, that then get dropped later. More information on what's going on is in PR41583: this patch causes SelectionDAG to point/place DBG_VALUEs at the VReg/instruction where an exported Value is written to its long-term VReg, rather than where the Value is computed.

This isn't a panacea: fractionally more variable locations get dropped in a clang-3.4 build, largely because of long-term VRegs subsequently being optimised out by OptimizePHIs and similar passes. In effect this change would swap (assuming D56151 is applied) locations dropped due to liveness issues, for locations dropped due to long-term VRegs being optimised out. (For me, the latter is more preferable).

There's the possibility that this won't resolve the regressions seen with D56151 if the problem really hinges on register class copies: I haven't managed to replicate such a scenario on X86 or AArch64 as they have simple register hierarchies. The problem would be analogous (differences between value-computed and value-used VRegs and locations) but would be more unpleasent to solve.

[aprantl]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1272

Comment that explains what this function does?

1283

Perhaps:

if (!RFV.occupiesMultipleRegs()) {
   SDV = DAG.getVRegDbgValue(Var, Expr, Reg, Node, dl, Order);
   DAG.AddDbgValue(SDV, Node, false);
   return;
}
...

1286

unsigned BitsToDescribe = Var->getSizeInBits().getValueOr(0);

[jmorse]

Scatter comments / address review

[jmorse]

Remove an accidental isIndirect=true flag, my bad.

NB, this patch does make four tests fail, all due to subtle shifts in DBG_VALUE placement (which is expected). I'll add the updates for those if this turns out to be a profitable direction.

[aprantl]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1276

///

1291

early return?

[bjope]

Sorry. I realize that I've completely forgotten to look at this. I'll try to find some time before the end of the week.

[jmorse]

Address feedback / review

[jmorse]

Sort-of ping at @bjope on this: this is fundamentally a speculative patch as I can't dig into the regressions being seen in D56151, and I have no expectations on other peoples time; however we'll need to think about whether those regressions block the placeDbgValues chain-of-things going into llvm-9, which will branch sometime soon (next month?). Similar to what I wrote in D56151, we have a choice of poisons for llvm-9:

• Leave placeDbgValues as it is,
• Disable placeDbgValues but don't commit D56151, which may cause dead variable locations to be resurrected during register coalescing,
• Disable placeDbgValues, commit D56151, triggering the regressions @bjope saw there, which may or may not be alleviated by this patch.

I'm fairly confident the issue in PR41583 (which I believe causes the seen regressions) can be addressed comprehensively, but likely not quickly. Delaying all these things doesn't bother me (fixing placeDbgValues has uncovered all kinds of other fun), but it should be a proactive decision.

[jmorse]

Abandoning this WIP -- addressing the "values are in two places in SelectionDAG" problem isn't going to be so easy to solve.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

public:
   DenseMap<MachineBasicBlock *, SmallVector<unsigned, 4>> LPadToCallSiteMap;
 
   /// This is set to true if a call in the current block has been translated as
   /// a tail call. In this case, no subsequent DAG nodes should be created.
   bool HasTailCall = false;
 
   LLVMContext *Context;
 
+  DenseMap<const Value *, SDValue> ExportedMap;
+
   SelectionDAGBuilder(SelectionDAG &dag, FunctionLoweringInfo &funcinfo,
                       SwiftErrorValueTracking &swifterror, CodeGenOpt::Level ol)
       : SDNodeOrder(LowestSDNodeOrder), TM(dag.getTarget()), DAG(dag),
         SL(make_unique<SDAGSwitchLowering>(this, funcinfo)), FuncInfo(funcinfo),
         SwiftError(swifterror) {}
 
   void init(GCFunctionInfo *gfi, AliasAnalysis *AA,
             const TargetLibraryInfo *li);
SDValue getCopyFromRegs(SelectionDAG &DAG, FunctionLoweringInfo &FuncInfo,
                           const SDLoc &dl, SDValue &Chain, SDValue *Flag,
                           const Value *V = nullptr) const;
 
   /// Emit a series of CopyToReg nodes that copies the specified value into the
   /// registers specified by this object. This uses Chain/Flag as the input and
   /// updates them for the output Chain/Flag. If the Flag pointer is nullptr, no
   /// flag is used. If V is not nullptr, then it is used in printing better
   /// diagnostic messages on error.
-  void getCopyToRegs(SDValue Val, SelectionDAG &DAG, const SDLoc &dl,
+  SDValue getCopyToRegs(SDValue Val, SelectionDAG &DAG, const SDLoc &dl,
                      SDValue &Chain, SDValue *Flag, const Value *V = nullptr,
                      ISD::NodeType PreferredExtendType = ISD::ANY_EXTEND) const;
 
   /// Add this value to the specified inlineasm node operand list. This adds the
   /// code marker, matching input operand index (if applicable), and includes
   /// the number of values added into it.
   void AddInlineAsmOperands(unsigned Code, bool HasMatching,
                             unsigned MatchingIdx, const SDLoc &dl,

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

Values[Value] = getCopyFromParts(DAG, dl, Parts.begin(), NumRegs,
                                      RegisterVT, ValueVT, V, CallConv);
     Part += NumRegs;
     Parts.clear();
   }
 
   return DAG.getNode(ISD::MERGE_VALUES, dl, DAG.getVTList(ValueVTs), Values);
 }
 
-void RegsForValue::getCopyToRegs(SDValue Val, SelectionDAG &DAG,
+SDValue RegsForValue::getCopyToRegs(SDValue Val, SelectionDAG &DAG,
                                  const SDLoc &dl, SDValue &Chain, SDValue *Flag,
                                  const Value *V,
                                  ISD::NodeType PreferredExtendType) const {
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   ISD::NodeType ExtendKind = PreferredExtendType;
 
   // Get the list of the values's legal parts.
   unsigned NumRegs = Regs.size();
for (unsigned Value = 0, Part = 0, e = ValueVTs.size(); Value != e; ++Value) {
 
     getCopyToParts(DAG, dl, Val.getValue(Val.getResNo() + Value), &Parts[Part],
                    NumParts, RegisterVT, V, CallConv, ExtendKind);
     Part += NumParts;
   }
 
   // Copy the parts into the registers.
   SmallVector<SDValue, 8> Chains(NumRegs);
-  for (unsigned i = 0; i != NumRegs; ++i) {
-    SDValue Part;
+  for (unsigned i = 0; i != NumRegs; ++i) {
     if (!Flag) {
       Part = DAG.getCopyToReg(Chain, dl, Regs[i], Parts[i]);
     } else {
       Part = DAG.getCopyToReg(Chain, dl, Regs[i], Parts[i], *Flag);
       *Flag = Part.getValue(1);
     }
 
     Chains[i] = Part.getValue(0);
   }
 
   if (NumRegs == 1 || Flag)
     // If NumRegs > 1 && Flag is used then the use of the last CopyToReg is
     // flagged to it. That is the CopyToReg nodes and the user are considered
     // a single scheduling unit. If we create a TokenFactor and return it as
     // chain, then the TokenFactor is both a predecessor (operand) of the
     // user as well as a successor (the TF operands are flagged to the user).
     // c1, f1 = CopyToReg
     // c2, f2 = CopyToReg
     // c3     = TokenFactor c1, c2
     // ...
     //        = op c3, ..., f2
     Chain = Chains[NumRegs-1];
   else
     Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other, Chains);
+
+  return Part;
 }
 
 void RegsForValue::AddInlineAsmOperands(unsigned Code, bool HasMatching,
                                         unsigned MatchingIdx, const SDLoc &dl,
                                         SelectionDAG &DAG,
                                         std::vector<SDValue> &Ops) const {
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
 
void SelectionDAGBuilder::clear() {
   NodeMap.clear();
   UnusedArgNodeMap.clear();
   PendingLoads.clear();
   PendingExports.clear();
   CurInst = nullptr;
   HasTailCall = false;
   SDNodeOrder = LowestSDNodeOrder;
   StatepointLowering.clear();
+  ExportedMap.clear();
 }
 
 void SelectionDAGBuilder::clearDanglingDebugInfo() {
   DanglingDebugInfoMap.clear();
 }
 
 SDValue SelectionDAGBuilder::getRoot() {
   if (PendingLoads.empty())
void SelectionDAGBuilder::salvageUnresolvedDbgValue(DanglingDebugInfo &DDI) {
   DAG.AddDbgValue(SDV, nullptr, false);
 
   LLVM_DEBUG(dbgs() << "Dropping debug value info for:\n  " << DDI.getDI()
                     << "\n");
   LLVM_DEBUG(dbgs() << "  Last seen at:\n    " << *DDI.getDI()->getOperand(0)
                     << "\n");
 }
 
+/// Helper for handleDebugValue to deal with dbg.value intrinsics that refer
+/// to a virtual register. If the original IR type was not a primitive, the
+/// variable location may have been split over several machine vregs, meaning
+/// the dbg.value must decompose into several DBG_VALUE insts and fragment
+/// expressions.
+static void handleVRegDebugValue(const Value *V, DILocalVariable *Var,
+                                 DIExpression *Expr, DebugLoc dl,
+                                 DebugLoc InstDL, unsigned Order, unsigned Reg,
+                                 SDNode *Node, const TargetLowering &TLI,
+                                 SelectionDAG &DAG) {
+  SDDbgValue *SDV;
+  // If this is a PHI node, it may be split up into several MI PHI nodes
+  // (in FunctionLoweringInfo::set).
+  RegsForValue RFV(V->getContext(), TLI, DAG.getDataLayout(), Reg, V->getType(),
+                   None);
+  if (!RFV.occupiesMultipleRegs()) {
+    // Simple case: the value resides in one virtual register.
+    SDV = DAG.getVRegDbgValue(Var, Expr, Reg, false, dl, Order);
+    DAG.AddDbgValue(SDV, Node, false);
+    return;
+  }
+
+  // Hard case: split variable location into multiple vregs and fragments.
+  unsigned Offset = 0;
+  unsigned BitsToDescribe = Var->getSizeInBits().getValueOr(0);
+  for (auto RegAndSize : RFV.getRegsAndSizes()) {
+    unsigned RegisterSize = RegAndSize.second;
+    // Bail out if all bits are described already.
+    if (Offset >= BitsToDescribe)
+      break;
+    unsigned FragmentSize = (Offset + RegisterSize > BitsToDescribe)
+                                ? BitsToDescribe - Offset
+                                : RegisterSize;
+    auto FragmentExpr =
+        DIExpression::createFragmentExpression(Expr, Offset, FragmentSize);
+    if (!FragmentExpr)
+      continue;
+    SDV = DAG.getVRegDbgValue(Var, *FragmentExpr, RegAndSize.first, false, dl,
+                              Order);
+    DAG.AddDbgValue(SDV, Node, false);
+    Offset += RegisterSize;
+  }
+}
+
 bool SelectionDAGBuilder::handleDebugValue(const Value *V, DILocalVariable *Var,
                                            DIExpression *Expr, DebugLoc dl,
                                            DebugLoc InstDL, unsigned Order) {
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   SDDbgValue *SDV;
   if (isa<ConstantInt>(V) || isa<ConstantFP>(V) || isa<UndefValue>(V) ||
       isa<ConstantPointerNull>(V)) {
     SDV = DAG.getConstantDbgValue(Var, Expr, V, dl, SDNodeOrder);
bool SelectionDAGBuilder::handleDebugValue(const Value *V, DILocalVariable *Var,
   // Do not use getValue() in here; we don't want to generate code at
   // this point if it hasn't been done yet.
   SDValue N = NodeMap[V];
   if (!N.getNode() && isa<Argument>(V)) // Check unused arguments map.
     N = UnusedArgNodeMap[V];
   if (N.getNode()) {
     if (EmitFuncArgumentDbgValue(V, Var, Expr, dl, false, N))
       return true;
+
+    // If this Value is exported from the current block, attach our debug
+    // record to the exporting CopyToReg, to avoid liveness issues.
+    if (ExportedMap.count(V)) {
+      SDValue &TheVal = ExportedMap.find(V)->second;
+      unsigned Reg = FuncInfo.ValueMap[V];
+
+      handleVRegDebugValue(V, Var, Expr, dl, InstDL, SDNodeOrder, Reg,
+                           TheVal.getNode(), TLI, DAG);
+      return true;
+    }
+
     SDV = getDbgValue(N, Var, Expr, dl, SDNodeOrder);
     DAG.AddDbgValue(SDV, N.getNode(), false);
     return true;
   }
 
   // Special rules apply for the first dbg.values of parameter variables in a
   // function. Identify them by the fact they reference Argument Values, that
   // they're parameters, and they are parameters of the current function. We
   // need to let them dangle until they get an SDNode.
   bool IsParamOfFunc = isa<Argument>(V) && Var->isParameter() &&
                        !InstDL.getInlinedAt();
   if (!IsParamOfFunc) {
     // The value is not used in this block yet (or it would have an SDNode).
     // We still want the value to appear for the user if possible -- if it has
     // an associated VReg, we can refer to that instead.
     auto VMI = FuncInfo.ValueMap.find(V);
     if (VMI != FuncInfo.ValueMap.end()) {
       unsigned Reg = VMI->second;
-      // If this is a PHI node, it may be split up into several MI PHI nodes
-      // (in FunctionLoweringInfo::set).
-      RegsForValue RFV(V->getContext(), TLI, DAG.getDataLayout(), Reg,
-                       V->getType(), None);
-      if (RFV.occupiesMultipleRegs()) {
-        unsigned Offset = 0;
-        unsigned BitsToDescribe = 0;
-        if (auto VarSize = Var->getSizeInBits())
-          BitsToDescribe = *VarSize;
-        if (auto Fragment = Expr->getFragmentInfo())
-          BitsToDescribe = Fragment->SizeInBits;
-        for (auto RegAndSize : RFV.getRegsAndSizes()) {
-          unsigned RegisterSize = RegAndSize.second;
-          // Bail out if all bits are described already.
-          if (Offset >= BitsToDescribe)
-            break;
-          unsigned FragmentSize = (Offset + RegisterSize > BitsToDescribe)
-              ? BitsToDescribe - Offset
-              : RegisterSize;
-          auto FragmentExpr = DIExpression::createFragmentExpression(
-              Expr, Offset, FragmentSize);
-          if (!FragmentExpr)
-              continue;
-          SDV = DAG.getVRegDbgValue(Var, *FragmentExpr, RegAndSize.first,
-                                    false, dl, SDNodeOrder);
-          DAG.AddDbgValue(SDV, nullptr, false);
-          Offset += RegisterSize;
-        }
-      } else {
-        SDV = DAG.getVRegDbgValue(Var, Expr, Reg, false, dl, SDNodeOrder);
-        DAG.AddDbgValue(SDV, nullptr, false);
-      }
+      handleVRegDebugValue(V, Var, Expr, dl, InstDL, SDNodeOrder, Reg, nullptr,
+                           TLI, DAG);
       return true;
     }
   }
 
   return false;
 }
 
 void SelectionDAGBuilder::resolveOrClearDbgInfo() {
SelectionDAGBuilder::CopyValueToVirtualRegister(const Value *V, unsigned Reg) {
   RegsForValue RFV(V->getContext(), TLI, DAG.getDataLayout(), Reg, V->getType(),
                    None); // This is not an ABI copy.
   SDValue Chain = DAG.getEntryNode();
 
   ISD::NodeType ExtendType = (FuncInfo.PreferredExtendType.find(V) ==
                               FuncInfo.PreferredExtendType.end())
                                  ? ISD::ANY_EXTEND
                                  : FuncInfo.PreferredExtendType[V];
+  SDValue CopyNode =
       RFV.getCopyToRegs(Op, DAG, getCurSDLoc(), Chain, nullptr, V, ExtendType);
   PendingExports.push_back(Chain);
+  ExportedMap.insert(std::make_pair(V, CopyNode));
 }
 
 #include "llvm/CodeGen/SelectionDAGISel.h"
 
 /// isOnlyUsedInEntryBlock - If the specified argument is only used in the
 /// entry block, return true.  This includes arguments used by switches, since
 /// the switch may expand into multiple basic blocks.
 static bool isOnlyUsedInEntryBlock(const Argument *A, bool FastISel) {