[CSInfo][NFC] Interpret loaded parameter value separately

[ntesic]

Summary

Refactor of DwarfDebug.cpp code.

Currently, collectCallSiteParameters method searches for instructions which load values into
registers used for parameters passing. Previously, interpretation of those values, loaded by one such
instruction, was implemented inside collectCallSiteParameters method.

This patch moves the interpretation code from collectCallSiteParameters method
into a separate static method named interpretValue. New method is called from
collectCallSiteParameters to process each instruction from targeted instruction scope.

Currently collectCallSiteParameters searches for loaded parameter value among instructions
which precede the call instruction, inside the same basic block.
When needed, new method (interpretValue) could be used for searching any instruction scope.

This is preparation for search of parameter value, loaded inside call delay slot.

[dstenb]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

797

Personal opinion, but I instinctively interpret the return value from interpretValue as to whether or not the value was described, not whether or not we are done describing all parameters. Perhaps it might be worth moving those conditions out to this loop instead, and not have interpretValue return anything?

And to avoid code duplication for the upcoming delay slot patch, I guess we can use a lambda for those conditions.

[dstenb]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

645

As I assume the DIExpression lookup is quite cheap, perhaps we should consider getting it inside this function instead of passing it as a parameter.

646

This is unused as far as I can tell.

647

As this is only used as a temporary container inside this function, and is cleared before returning, maybe it should be renamed to something to indicate that, e.g. something with Tmp.

[ntesic]

@dstenb
Thanks for the review.
I agree with all suggestions.

[dstenb]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

739

Nit: curMI -> CurMI

799–800

Sorry, but I don't understand what it means to skip "NOP description" in this context.

[ntesic]

Address comments.

[ntesic]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

799–800

The bad comment was here by mistake. Thanks.

[dstenb]

One minor comment about singular/plural phrasing. Otherwise this looks good to me!

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

740

As an instruction may produce multiple register values, maybe this should be called interpretValues, and the comment above updated to reflect that?

[ntesic]

Address comment.

[ntesic]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

740

Yes, It makes more sense. Thanks.

[vsk]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

796

What does nextInstrInterpret need to capture via [&]?

802

nit, the parens around *I aren't needed.

[djtodoro]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

796

Also the getForwardingRegsDefinedByMI does not need to capture via [&].

[djtodoro]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

796

https://reviews.llvm.org/D79616

[djtodoro]

Some nits included.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

745

MachineFunction *MF

841

MachineFunction *MF

849

MachineBasicBlock *MBB

[djtodoro]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

796

Sorry for the confusion.
After the conversation from D79616, I realized that default reference capturing is recommended by the LLVM Style Guide when lambda does not escape its context.

But anyway, this lambda (nextInstrInterpret) captures by reference but it does not use any data from the function, so the [&] could be deleted. Furthermore, maybe nextInstrInterpret could be factored out into a static function? I am OK with either way.
@vsk WDYT?

In addition:

nextInstrInterpret -> interpretNextInstr ?

[ntesic]

Address comments.
Transform lambda

nextInstrInterpret

into static function

interpretNextInstr

[djtodoro]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

806

clang-format please

[ntesic]

Clang format.

[djtodoro]

Thanks, lgtm. Please wait for others (for another day at least) to give their final comments.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (auto Param : ParamsToAdd) {
     // instructions we may have already created an expression for the
     // parameter when walking through the instructions. Append that to the
     // new expression.
     const DIExpression *CombinedExpr = combineDIExpressions(Expr, Param.Expr);
     ParamsForFwdReg.push_back({Param.ParamReg, CombinedExpr});
   }
 }
 
-/// Try to interpret values loaded into registers that forward parameters
-/// for \p CallMI. Store parameters with interpreted value into \p Params.
-static void collectCallSiteParameters(const MachineInstr *CallMI,
-                                      ParamSet &Params) {
-  auto *MF = CallMI->getMF();
-  auto CalleesMap = MF->getCallSitesInfo();
-  auto CallFwdRegsInfo = CalleesMap.find(CallMI);
-
-  // There is no information for the call instruction.
-  if (CallFwdRegsInfo == CalleesMap.end())
-    return;
+/// Interpret the value loaded into register by \p curMI.
+static bool interpretValue(const MachineInstr *CurMI,
+                           FwdRegWorklist &ForwardedRegWorklist,
+                           const DIExpression *EmptyExpr, ParamSet &Params,
+                           bool ShouldTryEmitEntryVals,
+                           FwdRegWorklist &NewWorklistItems) {
 
-  auto *MBB = CallMI->getParent();
+  auto *MF = CurMI->getMF();
   const auto &TRI = MF->getSubtarget().getRegisterInfo();
   const auto &TII = MF->getSubtarget().getInstrInfo();
   const auto &TLI = MF->getSubtarget().getTargetLowering();
 
-  // Skip the call instruction.
-  auto I = std::next(CallMI->getReverseIterator());
-
-  FwdRegWorklist ForwardedRegWorklist;
-
-  // If an instruction defines more than one item in the worklist, we may run
-  // into situations where a worklist register's value is (potentially)
-  // described by the previous value of another register that is also defined
-  // by that instruction.
-  //
-  // This can for example occur in cases like this:
-  //
-  //   $r1 = mov 123
-  //   $r0, $r1 = mvrr $r1, 456
-  //   call @foo, $r0, $r1
-  //
-  // When describing $r1's value for the mvrr instruction, we need to make sure
-  // that we don't finalize an entry value for $r0, as that is dependent on the
-  // previous value of $r1 (123 rather than 456).
-  //
-  // In order to not have to distinguish between those cases when finalizing
-  // entry values, we simply postpone adding new parameter registers to the
-  // worklist, by first keeping them in this temporary container until the
-  // instruction has been handled.
-  FwdRegWorklist NewWorklistItems;
-
-  const DIExpression *EmptyExpr =
-      DIExpression::get(MF->getFunction().getContext(), {});
-
-  // Add all the forwarding registers into the ForwardedRegWorklist.
-  for (auto ArgReg : CallFwdRegsInfo->second) {
-    bool InsertedReg =
-        ForwardedRegWorklist.insert({ArgReg.Reg, {{ArgReg.Reg, EmptyExpr}}})
-            .second;
-    assert(InsertedReg && "Single register used to forward two arguments?");
-    (void)InsertedReg;
-  }
-
-  // We erase, from the ForwardedRegWorklist, those forwarding registers for
-  // which we successfully describe a loaded value (by using
-  // the describeLoadedValue()). For those remaining arguments in the working
-  // list, for which we do not describe a loaded value by
-  // the describeLoadedValue(), we try to generate an entry value expression
-  // for their call site value description, if the call is within the entry MBB.
-  // TODO: Handle situations when call site parameter value can be described
-  // as the entry value within basic blocks other than the first one.
-  bool ShouldTryEmitEntryVals = MBB->getIterator() == MF->begin();
-
   // If the MI is an instruction defining one or more parameters' forwarding
   // registers, add those defines.
   auto getForwardingRegsDefinedByMI = [&](const MachineInstr &MI,
                                           SmallSetVector<unsigned, 4> &Defs) {
     if (MI.isDebugInstr())
       return;
 
     for (const MachineOperand &MO : MI.operands()) {
       if (MO.isReg() && MO.isDef() &&
           Register::isPhysicalRegister(MO.getReg())) {
         for (auto FwdReg : ForwardedRegWorklist)
           if (TRI->regsOverlap(FwdReg.first, MO.getReg()))
             Defs.insert(FwdReg.first);
       }
     }
   };
 
-  // Search for a loading value in forwarding registers.
-  for (; I != MBB->rend(); ++I) {
-    // Skip bundle headers.
-    if (I->isBundle())
-      continue;
+  if (CurMI->isBundle())
+    return false;
 
-    // If the next instruction is a call we can not interpret parameter's
-    // forwarding registers or we finished the interpretation of all parameters.
-    if (I->isCall())
-      return;
+  if (CurMI->isCall())
+    return true;
 
-    if (ForwardedRegWorklist.empty())
-      return;
+    return true;
 
-    // Set of worklist registers that are defined by this instruction.
-    SmallSetVector<unsigned, 4> FwdRegDefs;
 
-    getForwardingRegsDefinedByMI(*I, FwdRegDefs);
-    if (FwdRegDefs.empty())
-      continue;
+  getForwardingRegsDefinedByMI(*CurMI, FwdRegDefs);
+    return false;
 
-    for (auto ParamFwdReg : FwdRegDefs) {
-      if (auto ParamValue = TII->describeLoadedValue(*I, ParamFwdReg)) {
+    if (auto ParamValue = TII->describeLoadedValue(*CurMI, ParamFwdReg)) {
         if (ParamValue->first.isImm()) {
           int64_t Val = ParamValue->first.getImm();
           finishCallSiteParams(Val, ParamValue->second,
                                ForwardedRegWorklist[ParamFwdReg], Params);
         } else if (ParamValue->first.isReg()) {
           Register RegLoc = ParamValue->first.getReg();
           unsigned SP = TLI->getStackPointerRegisterToSaveRestore();
           Register FP = TRI->getFrameRegister(*MF);
           bool IsSPorFP = (RegLoc == SP) || (RegLoc == FP);
           if (TRI->isCalleeSavedPhysReg(RegLoc, *MF) || IsSPorFP) {
             MachineLocation MLoc(RegLoc, /*IsIndirect=*/IsSPorFP);
             finishCallSiteParams(MLoc, ParamValue->second,
                                  ForwardedRegWorklist[ParamFwdReg], Params);
-          } else {
             // ParamFwdReg was described by the non-callee saved register
             // RegLoc. Mark that the call site values for the parameters are
             // dependent on that register instead of ParamFwdReg. Since RegLoc
             // may be a register that will be handled in this iteration, we
             // postpone adding the items to the worklist, and instead keep them
             // in a temporary container.
             addToFwdRegWorklist(NewWorklistItems, RegLoc, ParamValue->second,
                                 ForwardedRegWorklist[ParamFwdReg]);
-          }
-        }
-      }
-    }
 
-    // Remove all registers that this instruction defines from the worklist.
-    for (auto ParamFwdReg : FwdRegDefs)
-      ForwardedRegWorklist.erase(ParamFwdReg);
 
-    // Now that we are done handling this instruction, add items from the
-    // temporary worklist to the real one.
-    for (auto New : NewWorklistItems)
-      addToFwdRegWorklist(ForwardedRegWorklist, New.first, EmptyExpr,
-                          New.second);
-    NewWorklistItems.clear();
+
+  return false;
+}
+
+/// Try to interpret values loaded into registers that forward parameters
+/// for \p CallMI. Store parameters with interpreted value into \p Params.
+static void collectCallSiteParameters(const MachineInstr *CallMI,
+                                      ParamSet &Params) {
+  auto *MF = CallMI->getMF();
+  auto CalleesMap = MF->getCallSitesInfo();
+  auto CallFwdRegsInfo = CalleesMap.find(CallMI);
+
+  // There is no information for the call instruction.
+  if (CallFwdRegsInfo == CalleesMap.end())
+    return;
+
+  auto *MBB = CallMI->getParent();
+
+  // Skip the call instruction.
+  auto I = std::next(CallMI->getReverseIterator());
+
+  FwdRegWorklist ForwardedRegWorklist;
+
+  // If an instruction defines more than one item in the worklist, we may run
+  // into situations where a worklist register's value is (potentially)
+  // described by the previous value of another register that is also defined
+  // by that instruction.
+  //
+  // This can for example occur in cases like this:
+  //
+  //   $r1 = mov 123
+  //   $r0, $r1 = mvrr $r1, 456
+  //   call @foo, $r0, $r1
+  //
+  // When describing $r1's value for the mvrr instruction, we need to make sure
+  // that we don't finalize an entry value for $r0, as that is dependent on the
+  // previous value of $r1 (123 rather than 456).
+  //
+  // In order to not have to distinguish between those cases when finalizing
+  // entry values, we simply postpone adding new parameter registers to the
+  // worklist, by first keeping them in this temporary container until the
+  // instruction has been handled.
+  FwdRegWorklist NewWorklistItems;
+
+  const DIExpression *EmptyExpr =
+      DIExpression::get(MF->getFunction().getContext(), {});
+
+  // Add all the forwarding registers into the ForwardedRegWorklist.
+  for (auto ArgReg : CallFwdRegsInfo->second) {
+    bool InsertedReg =
+        ForwardedRegWorklist.insert({ArgReg.Reg, {{ArgReg.Reg, EmptyExpr}}})
+            .second;
+    assert(InsertedReg && "Single register used to forward two arguments?");
+    (void)InsertedReg;
+  }
+
+  // We erase, from the ForwardedRegWorklist, those forwarding registers for
+  // which we successfully describe a loaded value (by using
+  // the describeLoadedValue()). For those remaining arguments in the working
+  // list, for which we do not describe a loaded value by
+  // the describeLoadedValue(), we try to generate an entry value expression
+  // for their call site value description, if the call is within the entry MBB.
+  // TODO: Handle situations when call site parameter value can be described
+  // as the entry value within basic blocks other than the first one.
+  bool ShouldTryEmitEntryVals = MBB->getIterator() == MF->begin();
+
+  // Search for a loading value in forwarding registers.
+  for (; I != MBB->rend(); ++I) {
+    if (interpretValue(&(*I), ForwardedRegWorklist, EmptyExpr, Params,
+                       ShouldTryEmitEntryVals, NewWorklistItems))
+      return;
   }
 
   // Emit the call site parameter's value as an entry value.
   if (ShouldTryEmitEntryVals) {
     // Create an expression where the register's entry value is used.
     DIExpression *EntryExpr = DIExpression::get(
         MF->getFunction().getContext(), {dwarf::DW_OP_LLVM_entry_value, 1});
     for (auto RegEntry : ForwardedRegWorklist) {
       MachineLocation MLoc(RegEntry.first);
       finishCallSiteParams(MLoc, EntryExpr, RegEntry.second, Params);
     }
   }
 }
 
 void DwarfDebug::constructCallSiteEntryDIEs(const DISubprogram &SP,
for (const MachineBasicBlock &MBB : MF) {
     for (const MachineInstr &MI : MBB.instrs()) {
       // Bundles with call in them will pass the isCall() test below but do not
       // have callee operand information so skip them here. Iterator will
       // eventually reach the call MI.
       if (MI.isBundle())
         continue;
 
       // Skip instructions which aren't calls. Both calls and tail-calling jump
       // instructions (e.g TAILJMPd64) are classified correctly here.
       if (!MI.isCandidateForCallSiteEntry())
         continue;
 
       // Skip instructions marked as frame setup, as they are not interesting to
       // the user.
       if (MI.getFlag(MachineInstr::FrameSetup))
         continue;
 
       // TODO: Add support for targets with delay slots (see: beginInstruction).
       if (MI.hasDelaySlot())
         return;
 
       // If this is a direct call, find the callee's subprogram.
       // In the case of an indirect call find the register that holds
       // the callee.
       const MachineOperand &CalleeOp = MI.getOperand(0);