NFC; refactor getFrameIndexReferenceFromSP

[sanjoy]

Summary

... into getFrameIndexReferencePreferSP. This change folds the
fail-then-retry logic into getFrameIndexReferencePreferSP.

There is a non-functional but behaviorial change in WinException --
earlier if getFrameIndexReferenceFromSP failed we'd trip an assert,
but now we'll silently use the (wrong) offset from the base pointer. I
could not write the assert I'd like to write ("FrameReg ==
StackRegister", like I've done in X86FrameLowering) since there is no
easy way to get to the stack register from WinException (happy to be
proven wrong here). One solution to this is to add a `bool
OnlyStackPointer` parameter to getFrameIndexReferenceFromSP that
asserts if it could not satisfy its promise of returning an offset from
a stack pointer, but that seems overkill.

[rnk]

lgtm with the assert

include/llvm/Target/TargetFrameLowering.h

257 ↗

(On Diff #60948)

Can we rename "AllowSPAdjustment" to something better? As the doc comments say, it's really about getting the offset relative to SP at end of prologue. A better name might be IgnoreSPUpdates or IgnoreSPAdjustments or OffsetFromInitialSP or OffsetFromSPAfterPrologue.

lib/CodeGen/AsmPrinter/WinException.cpp

307 ↗

(On Diff #60948)

We could try this assert:

assert(UnusedReg == Asm->MF->getSubtarget().getTargetLowering()->getStackPointerRegisterToSaveRestore());

Beautiful, I know.

The only way this can end up using the FP and we fail the assert is if we try to reference a fixed object with stack realignment. In this code, FrameIndex should never refer to a fixed stack object, so we should be OK.

llvm/trunk/include/llvm/Target/TargetFrameLowering.h

public:
   virtual bool needsFrameIndexResolution(const MachineFunction &MF) const;
 
   /// getFrameIndexReference - This method should return the base register
   /// and offset used to reference a frame index location. The offset is
   /// returned directly, and the base register is returned via FrameReg.
   virtual int getFrameIndexReference(const MachineFunction &MF, int FI,
                                      unsigned &FrameReg) const;
 
-  /// Same as above, except that the 'base register' will always be RSP, not
-  /// RBP on x86. This is generally used for emitting statepoint or EH tables
-  /// that use offsets from RSP.
-  /// If AllowSPAdjustment is true, the returned offset is only guaranteed
-  /// to be valid with respect to the value of SP at the end of the prologue.
-  /// TODO: This should really be a parameterizable choice.
-  virtual Optional<int>
-  getFrameIndexReferenceFromSP(const MachineFunction &MF, int FI,
+  /// Same as \c getFrameIndexReference, except that the stack pointer (as
+  /// opposed to the frame pointer) will be the preferred value for \p
+  /// FrameReg. This is generally used for emitting statepoint or EH tables that
+  /// use offsets from RSP.  If \p IgnoreSPUpdates is true, the returned
+  /// offset is only guaranteed to be valid with respect to the value of SP at
+  /// the end of the prologue.
+  virtual int getFrameIndexReferencePreferSP(const MachineFunction &MF, int FI,
                                              unsigned &FrameReg,
-                               bool AllowSPAdjustment) const {
-    // default to calling normal version, we override this on x86 only
-    llvm_unreachable("unimplemented for non-x86");
-    return None;
+                                             bool IgnoreSPUpdates) const {
+    // Always safe to dispatch to getFrameIndexReference.
+    return getFrameIndexReference(MF, FI, FrameReg);
   }
 
   /// This method determines which of the registers reported by
   /// TargetRegisterInfo::getCalleeSavedRegs() should actually get saved.
   /// The default implementation checks populates the \p SavedRegs bitset with
   /// all registers which are modified in the function, targets may override
   /// this function to save additional registers.
   /// This method also sets up the register scavenger ensuring there is a free

llvm/trunk/lib/CodeGen/AsmPrinter/WinException.cpp

 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCWin64EH.h"
 #include "llvm/Support/COFF.h"
 #include "llvm/Support/Dwarf.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Target/TargetFrameLowering.h"
+#include "llvm/Target/TargetLowering.h"
 #include "llvm/Target/TargetLoweringObjectFile.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Target/TargetRegisterInfo.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
 using namespace llvm;
 
 WinException::WinException(AsmPrinter *A) : EHStreamer(A) {
   // MSVC's EH tables are always composed of 32-bit words.  All known 64-bit
return MCBinaryExpr::createAdd(getOffset(OffsetOf, OffsetFrom),
                                  MCConstantExpr::create(1, Asm->OutContext),
                                  Asm->OutContext);
 }
 
 int WinException::getFrameIndexOffset(int FrameIndex,
                                       const WinEHFuncInfo &FuncInfo) {
   const TargetFrameLowering &TFI = *Asm->MF->getSubtarget().getFrameLowering();
   unsigned UnusedReg;
-  if (Asm->MAI->usesWindowsCFI())
-    return *TFI.getFrameIndexReferenceFromSP(*Asm->MF, FrameIndex, UnusedReg,
-                                             /*AllowSPAdjustment*/ true);
+  if (Asm->MAI->usesWindowsCFI()) {
+    int Offset =
+        TFI.getFrameIndexReferencePreferSP(*Asm->MF, FrameIndex, UnusedReg,
+                                           /*IgnoreSPUpdates*/ true);
+    assert(UnusedReg ==
+           Asm->MF->getSubtarget()
+               .getTargetLowering()
+               ->getStackPointerRegisterToSaveRestore());
+    return Offset;
+  }
+
   // For 32-bit, offsets should be relative to the end of the EH registration
   // node. For 64-bit, it's relative to SP at the end of the prologue.
   assert(FuncInfo.EHRegNodeEndOffset != INT_MAX);
   int Offset = TFI.getFrameIndexReference(*Asm->MF, FrameIndex, UnusedReg);
   Offset += FuncInfo.EHRegNodeEndOffset;
   return Offset;
 }
 

llvm/trunk/lib/CodeGen/PrologEpilogInserter.cpp

for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
       // remaining difference is the unconditional use of the stack
       // pointer as the base register.
       if (MI->getOpcode() == TargetOpcode::STATEPOINT) {
         assert((!MI->isDebugValue() || i == 0) &&
                "Frame indicies can only appear as the first operand of a "
                "DBG_VALUE machine instruction");
         unsigned Reg;
         MachineOperand &Offset = MI->getOperand(i + 1);
-        int refOffset;
-        // First try to get an offset relative to SP. If that's not
-        // possible use whatever the target usually uses.
-        auto SPOffset = TFI->getFrameIndexReferenceFromSP(
-            Fn, MI->getOperand(i).getIndex(), Reg, /*AllowSPAdjustment*/ false);
-        if (SPOffset)
-          refOffset = *SPOffset;
-        else
-          refOffset = TFI->getFrameIndexReference(
-              Fn, MI->getOperand(i).getIndex(), Reg);
-
+        int refOffset = TFI->getFrameIndexReferencePreferSP(
+            Fn, MI->getOperand(i).getIndex(), Reg, /*IgnoreSPUpdates*/ false);
         Offset.setImm(Offset.getImm() + refOffset);
         MI->getOperand(i).ChangeToRegister(Reg, false /*isDef*/);
         continue;
       }
 
       // Some instructions (e.g. inline asm instructions) can have
       // multiple frame indices and/or cause eliminateFrameIndex
       // to insert more than one instruction. We need the register

llvm/trunk/lib/Target/X86/X86FrameLowering.h

public:
   bool hasFP(const MachineFunction &MF) const override;
   bool hasReservedCallFrame(const MachineFunction &MF) const override;
   bool canSimplifyCallFramePseudos(const MachineFunction &MF) const override;
   bool needsFrameIndexResolution(const MachineFunction &MF) const override;
 
   int getFrameIndexReference(const MachineFunction &MF, int FI,
                              unsigned &FrameReg) const override;
 
-  Optional<int>
-  getFrameIndexReferenceFromSP(const MachineFunction &MF, int FI,
+  int getFrameIndexReferencePreferSP(const MachineFunction &MF, int FI,
                                      unsigned &FrameReg,
-                               bool AllowSPAdjustment) const override;
+                                     bool IgnoreSPUpdates) const override;
 
   MachineBasicBlock::iterator
   eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
                                 MachineBasicBlock::iterator MI) const override;
 
   unsigned getWinEHParentFrameOffset(const MachineFunction &MF) const override;
 
   void processFunctionBeforeFrameFinalized(MachineFunction &MF,

llvm/trunk/lib/Target/X86/X86FrameLowering.cpp

 // funclets' frames allows each funclet to dynamically accept any ancestor
 // frame as its establisher argument (the runtime doesn't guarantee the
 // immediate parent for some reason lost to history), and also allows the GC,
 // which uses the PSPSym for some bookkeeping, to find it in any funclet's
 // frame with only a single offset reported for the entire method.
 unsigned
 X86FrameLowering::getPSPSlotOffsetFromSP(const MachineFunction &MF) const {
   const WinEHFuncInfo &Info = *MF.getWinEHFuncInfo();
-  // getFrameIndexReferenceFromSP has an out ref parameter for the stack
-  // pointer register; pass a dummy that we ignore
   unsigned SPReg;
-  int Offset = *getFrameIndexReferenceFromSP(MF, Info.PSPSymFrameIdx, SPReg,
-                                             /*AllowSPAdjustment*/ true);
-  assert(Offset >= 0);
+  int Offset = getFrameIndexReferencePreferSP(MF, Info.PSPSymFrameIdx, SPReg,
+                                              /*IgnoreSPUpdates*/ true);
+  assert(Offset >= 0 && SPReg == TRI->getStackRegister());
   return static_cast<unsigned>(Offset);
 }
 
 unsigned
 X86FrameLowering::getWinEHFuncletFrameSize(const MachineFunction &MF) const {
   // This is the size of the pushed CSRs.
   unsigned CSSize =
       MF.getInfo<X86MachineFunctionInfo>()->getCalleeSavedFrameSize();
if (TRI->hasBasePointer(MF)) {
     int TailCallReturnAddrDelta = X86FI->getTCReturnAddrDelta();
     if (TailCallReturnAddrDelta < 0)
       Offset -= TailCallReturnAddrDelta;
   }
 
   return Offset + FPDelta;
 }
 
-// Simplified from getFrameIndexReference keeping only StackPointer cases
-Optional<int>
-X86FrameLowering::getFrameIndexReferenceFromSP(const MachineFunction &MF,
+int
+X86FrameLowering::getFrameIndexReferencePreferSP(const MachineFunction &MF,
                                                  int FI, unsigned &FrameReg,
-                                               bool AllowSPAdjustment) const {
+                                                 bool IgnoreSPUpdates) const {
+
   const MachineFrameInfo *MFI = MF.getFrameInfo();
   // Does not include any dynamic realign.
   const uint64_t StackSize = MFI->getStackSize();
   // LLVM arranges the stack as follows:
   //   ...
   //   ARG2
   //   ARG1
   //   RETADDR
X86FrameLowering::getFrameIndexReferencePreferSP(const MachineFunction &MF,
   // pointer" points to local variables, and RBP points to fixed objects.
   //
   // In cases 2 and 3, we can only answer for non-fixed stack objects, and the
   // answer we give is relative to the SP after the prologue, and not the
   // SP in the middle of the function.
 
   if (MFI->isFixedObjectIndex(FI) && TRI->needsStackRealignment(MF) &&
       !STI.isTargetWin64())
-    return None;
+    return getFrameIndexReference(MF, FI, FrameReg);
 
   // If !hasReservedCallFrame the function might have SP adjustement in the
   // body.  So, even though the offset is statically known, it depends on where
   // we are in the function.
   const TargetFrameLowering *TFI = MF.getSubtarget().getFrameLowering();
-  if (!AllowSPAdjustment && !TFI->hasReservedCallFrame(MF))
-    return None;
+  if (!IgnoreSPUpdates && !TFI->hasReservedCallFrame(MF))
+    return getFrameIndexReference(MF, FI, FrameReg);
 
   // We don't handle tail calls, and shouldn't be seeing them either.
   assert(MF.getInfo<X86MachineFunctionInfo>()->getTCReturnAddrDelta() >= 0 &&
          "we don't handle this case!");
 
   // Fill in FrameReg output argument.
   FrameReg = TRI->getStackRegister();