[AsmPrinter] Look inside bundles and rely on FrameDestroy in calculateDbgValueHistory

[bjope]

Summary

When DbgValueHistoryCalculator is determining which registers
that are clobbered within the function body it now got the
alternative to rely on FrameDestroy attributes when finding
out if an instruction is part of the epilogue or not.
In the past it treated all instructions with the same DebugLoc
as the return instruction as epilogue. So if for example the
whole function was written on a single line everything ended up
being treated as the epilogue.
For the above to work properly a target has to mark epilogue
instructions as FrameDestroy (I do not think all targets are
doing that, so therefore this isn't enabled by default).

Another difference between the old collectChangingRegs and
the new collectChangingRegsInBody is that the new method
examines individual instructions within bundles (to support
the case where a bundle contain a mix of prolog/epilog
instructions and function body instructions. That might be
an odd case, but there is nothing today that prohibits such
bundling.

[vsk]

include/llvm/CodeGen/TargetFrameLowering.h

286

Do you know which backends don't mark up epilog instructions with FrameDestroy? Is it feasible to fix the backends? Seems like collectChangingRegsInBody is just a more precise way of doing the job of collectChangingRegs: it'd be great to just have one copy of the logic.

[bjope]

include/llvm/CodeGen/TargetFrameLowering.h

286

Yes, the goal would be to make the hook return true in the future.

All I know right know is that turning this on by default makes lots of lit-tests fail. So even if the backend is updating FrameDestroy properly, we need to examine failing lit-tests, check if the target is using FrameDestroy, and update CHECK-statements when the result is improved.

A suggested deployment could be:

1. Add this hook (this patch).
2. For each in-tree target, examine if target is using FrameDestroy, if so override the hook and check/update lit-test. If not, start using FrameDestroy and override on hook.
3. When all in-tree targets are using collectChangingRegsInBody, we can remove the old collectChangingRegs (and getFirstEpilogueInst) functions.

A possible addition to step 3 is to add an assert that isFrameSetupAndFrameDestroySupported returns true. That would give out-of-tree targets a chance to detect that they need to adapt their code somehow. And then after awhile we can remove the hook completely.

[vsk]

include/llvm/CodeGen/TargetFrameLowering.h

286

With this patch applied I just see four failing tests in check-llvm:

LLVM :: DebugInfo/AArch64/struct_by_value.ll
LLVM :: DebugInfo/X86/parameters.ll
LLVM :: DebugInfo/X86/reference-argument.ll
LLVM :: DebugInfo/X86/vla-multi.ll

It seems like the failures result from the tests being too rigid, not from really fundamental issues. It seems like it'd be less work overall to fix these now (& I'd volunteer to help with that, just fixed the vla-multi.ll test in r340068). If there are more failing tests I'm just not seeing it'd make sense to reconsider, but I'd like to avoid leaving things in a transitionary state if possible.

[bjope]

include/llvm/CodeGen/TargetFrameLowering.h

286

Ok, looks like it wasn't so many tests failing after all then. So I agree that we should try to adjust the too rigid tests first.
I'll also examine FrameLowering implementations to see if FrameDestroy is used by different targets.

It would ofcourse be nice to skip the transition state completely, but then I think we somehow should give out-of-tree targets a heads up.

include/llvm/CodeGen/TargetFrameLowering.h

public:
   virtual void processFunctionBeforeFrameFinalized(MachineFunction &MF,
                                              RegScavenger *RS = nullptr) const {
   }
 
   virtual unsigned getWinEHParentFrameOffset(const MachineFunction &MF) const {
     report_fatal_error("WinEH not implemented for this target");
   }
 
+  /// Check if the target is updating FrameSetup and FrameDestroy attributes on
+  /// machine instructions.
+  virtual bool isFrameSetupAndFrameDestroySupported() const {
+    return false;
+  }
+
   /// This method is called during prolog/epilog code insertion to eliminate
   /// call frame setup and destroy pseudo instructions (but only if the Target
   /// is using them).  It is responsible for eliminating these instructions,
   /// replacing them with concrete instructions.  This method need only be
   /// implemented if using call frame setup/destroy pseudo instructions.
   /// Returns an iterator pointing to the instruction after the replaced one.
   virtual MachineBasicBlock::iterator
   eliminateCallFramePseudoInstr(MachineFunction &MF,

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp

 #include "DbgValueHistoryCalculator.h"
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineOperand.h"
+#include "llvm/CodeGen/TargetFrameLowering.h"
 #include "llvm/CodeGen/TargetLowering.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
for (const auto &MI : MBB) {
         } else if (MO.isRegMask()) {
           Regs.setBitsNotInMask(MO.getRegMask());
         }
       }
     }
   }
 }
 
+/// \brief Collect registers that are modified in the function (ignoring
+/// FrameSetup/FrameDestroy/Return instructions). Debug variables that are
+/// mapped to such registers need to be described using debug location ranges
+/// (as they aren't valid throughout the function). Compare this to a variable
+/// that is an argument to the function, being received in a register that isn't
+/// modified. Such a variable can be printed from anywhere inside the function
+/// and no range information is needed.
+static void collectChangingRegsInBody(const MachineFunction *MF,
+                                      const TargetRegisterInfo *TRI,
+                                      BitVector &Regs) {
+  for (const auto &MBB : *MF) {
+    // A bundle might contain a mix of FrameSetup/FrameDestroy and other
+    // instructions. So we need to use the instr iterator to analyse the
+    // individual instructions.
+    for (const auto &MI : MBB.instrs()) {
+      // We look inside the bundle, so skip the BUNDLE head.
+      if (MI.isBundle())
+        continue;
+      // Avoid looking at prologue or epilogue instructions.
+      if (MI.isReturn() ||
+          MI.getFlag(MachineInstr::FrameSetup) ||
+          MI.getFlag(MachineInstr::FrameDestroy))
+        continue;
+
+      // Look for register defs and register masks. Register masks are
+      // typically on calls and they clobber everything not in the mask.
+      for (const MachineOperand &MO : MI.operands()) {
+        // Skip virtual registers since they are handled by the parent.
+        if (MO.isReg() && MO.isDef() && MO.getReg() &&
+            !TRI->isVirtualRegister(MO.getReg())) {
+          for (MCRegAliasIterator AI(MO.getReg(), TRI, true); AI.isValid();
+               ++AI)
+            Regs.set(*AI);
+        } else if (MO.isRegMask()) {
+          Regs.setBitsNotInMask(MO.getRegMask());
+        }
+      }
+    }
+  }
+}
+
 void llvm::calculateDbgValueHistory(const MachineFunction *MF,
                                     const TargetRegisterInfo *TRI,
                                     DbgValueHistoryMap &Result) {
+  const TargetFrameLowering *TFI = MF->getSubtarget().getFrameLowering();
+  bool LookInsideBundlesAndRelyOnFrameDestroy =
+    TFI->isFrameSetupAndFrameDestroySupported();
   BitVector ChangingRegs(TRI->getNumRegs());
+  if (LookInsideBundlesAndRelyOnFrameDestroy)
+    collectChangingRegsInBody(MF, TRI, ChangingRegs);
+  else
     collectChangingRegs(MF, TRI, ChangingRegs);
 
   const TargetLowering *TLI = MF->getSubtarget().getTargetLowering();
   unsigned SP = TLI->getStackPointerRegisterToSaveRestore();
   RegDescribedVarsMap RegVars;
   for (const auto &MBB : *MF) {
     for (const auto &MI : MBB) {
       if (!MI.isDebugInstr()) {
         // Not a DBG_VALUE instruction. It may clobber registers which describe