don't force SP-relative addressing for statepoints when the offset is not statically known

[carnaval]

Summary

Prioritize addresses relative to the stack pointer in the stackmap, but fallback gracefully to other modes of addressing if the offset to the stack pointer is not a known constant.

[carnaval]

add context according to local customs

[sanjoy]

Hi,

I'm happy to review these (and thanks for the patches!) but can you please split these up into three patches?

include/llvm/Target/TargetFrameLowering.h

255 ↗

(On Diff #60443)

Why not return an Optional<int>?

[carnaval]

• split out the two small fixes at http://reviews.llvm.org/D21287 and http://reviews.llvm.org/D21286
• use optional type

[sanjoy]

include/llvm/Target/TargetFrameLowering.h

257 ↗

(On Diff #60528)

Just return None;

lib/CodeGen/AsmPrinter/WinException.cpp

304 ↗

(On Diff #60528)

No need to use braces here. Also, this indent looks weird -- is it clang-formatted?

Note: you can also use *TFI.getFrameIndexReferenceFromSP(*Asm->MF, FrameIndex, UnusedReg) instead of getValue.

lib/CodeGen/PrologEpilogInserter.cpp

1102 ↗

(On Diff #60528)

I'd suggest pushing the hasReservedCallFrame check into getFrameIndexReferenceFromSP -- getFrameIndexReferenceFromSP be returning None or a valid offset.

1105 ↗

(On Diff #60528)

Note: instead of if (X.hasValue()) you can also do if (X). Since Optional<T> does not auto-convert to T (you need to use its operator* or getValue()) there are no messy cases around e.g. an Optional<int> containing 0 as its value looking like it has no value etc.

lib/Target/X86/X86FrameLowering.cpp

1766 ↗

(On Diff #60528)

Use return None; and remove the braces.

1805 ↗

(On Diff #60528)

You don't need to explicitly create an Optional<int>, you can just return Offset + StackSize.

test/CodeGen/X86/statepoint-nosp.ll

2 ↗

(On Diff #60528)

You'll need a REQUIRES: asserts here otherwise the test will fail on product builds that don't have -debug-only.

[sanjoy]

Btw, here are our coding standards, in case that helps: http://llvm.org/docs/CodingStandards.html

[carnaval]

re: putting the hasReservedCallFrame check
I don't think it's possible since the other use site of this function (in WinException) seem to actually use it in that case. I'm assuming that in this context it's guaranteed that the offset will only be interpreted relative to the stack pointer on function entry.
I've added a comment to reflect that at the getFrameIndexReferenceFromSP declaration.

[sanjoy]

In D21259#456589, @carnaval wrote:

re: putting the hasReservedCallFrame check
I don't think it's possible since the other use site of this function (in WinException) seem to actually use it in that case. I'm assuming that in this context it's guaranteed that the offset will only be interpreted relative to the stack pointer on function entry.

@rnk -- can you confirm if the above is intentional? If it is, then lgtm else lets move hasReservedCallFrame into getFrameIndexReferenceFromSP.

include/llvm/Target/TargetFrameLowering.h

252 ↗

(On Diff #60580)

I'd change the language a little bit to say ".. except that the 'base register' will always be RSP and the offset is from the value of RSP after the function prologue"

[majnemer]

WinEH does not expect getFrameIndexReferenceFromSP to return a result which is dynamically correct from an arbitrary program point.
The result of getFrameIndexReferenceFromSP goes into data, not code. The idea is that the exception handling runtime wants to know the offset of the exception object relative to what the SP is after the prologue is set up.
The runtime uses CFI to recreate the stack pointer at prologue start; that, coupled with the offset, is utilized to initialize stuff like catch objects.

[sanjoy]

Given what @majnemer said, do you mind adding a boolean to the getFrameIndexReferenceFromSP API as bool AllowSPAdjustment with the semantic that if it is true then the offset returned is correct only based off of the SP on entry, while if it is false then the offset (if any returned) has to be correct for the SP at arbitrary program points after the prologue? Currently the API is such that other than documentation there is nothing that will force a user to carefully think about which variant they want.

[carnaval]

add AllowSPAdjustment

[sanjoy]

lgtm modulo very minor stuff

lib/CodeGen/PrologEpilogInserter.cpp

1098 ↗

(On Diff #60623)

Reflow this comment?

lib/Target/X86/X86FrameLowering.cpp

1767 ↗

(On Diff #60623)

Start with uppercase.

test/CodeGen/X86/statepoint-nosp.ll

7 ↗

(On Diff #60623)

This should probably go into test/CodeGen/X86/deopt-bundles.ll.

[majnemer]

lib/CodeGen/PrologEpilogInserter.cpp

1101 ↗

(On Diff #60623)

Please do something like /*AllowSPAdjustment=*/false so that readers can figure out what the bool parameter does.

1105–1106 ↗

(On Diff #60623)

I'd flip the sense of the comparison around.

[carnaval]

address comments
move test to an existing file

[rnk]

Is it really necessary to prefer SP-relative offsets from FP-relative offsets? Is there any reason not to standardize on getFrameIndexReference? You can always get SP-relative offsets by disabling frame pointers.

Assuming we need SP-relative offsets, I think it would be cleaner to expose an API on TargetFrameLowering that prefers returning SP-relative offsets if possible, rather than having an API that might fail. The base TargetFrameLowering implementation can delegate directly to getFrameIndexReference, and targets like X86 can implement the preference for SP-relative offsets.

[sanjoy]

In D21259#458739, @rnk wrote:

Is it really necessary to prefer SP-relative offsets from FP-relative offsets? Is there any reason not to standardize on getFrameIndexReference? You can always get SP-relative offsets by disabling frame pointers.

Assuming we need SP-relative offsets, I think it would be cleaner to expose an API on TargetFrameLowering that prefers returning SP-relative offsets if possible, rather than having an API that might fail. The base TargetFrameLowering implementation can delegate directly to getFrameIndexReference, and targets like X86 can implement the preference for SP-relative offsets.

We do want to prefer SP relative offsets, even when we're preserving frame pointers. But I like your second idea, I'll put up a patch for review soon.

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

public:
   /// 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 int getFrameIndexReferenceFromSP(const MachineFunction &MF, int FI,
-                                           unsigned &FrameReg) const {
+  virtual Optional<int>
+  getFrameIndexReferenceFromSP(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 0;
+    return None;
   }
 
   /// 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

return MCBinaryExpr::createAdd(getOffset(OffsetOf, OffsetFrom),
                                  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);
+    return *TFI.getFrameIndexReferenceFromSP(*Asm->MF, FrameIndex, UnusedReg,
+                                             /*AllowSPAdjustment*/ true);
   // 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);
-        const unsigned refOffset =
-          TFI->getFrameIndexReferenceFromSP(Fn, MI->getOperand(i).getIndex(),
-                                            Reg);
+        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);
 
         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

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;
 
-  int getFrameIndexReferenceFromSP(const MachineFunction &MF, int FI,
-                                   unsigned &FrameReg) const override;
+  Optional<int>
+  getFrameIndexReferenceFromSP(const MachineFunction &MF, int FI,
+                               unsigned &FrameReg,
+                               bool AllowSPAdjustment) 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

 // 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);
+  int Offset = *getFrameIndexReferenceFromSP(MF, Info.PSPSymFrameIdx, SPReg,
+                                             /*AllowSPAdjustment*/ true);
   assert(Offset >= 0);
   return static_cast<unsigned>(Offset);
 }
 
 unsigned
 X86FrameLowering::getWinEHFuncletFrameSize(const MachineFunction &MF) const {
   // This is the size of the pushed CSRs.
   unsigned CSSize =
if (TRI->hasBasePointer(MF)) {
     if (TailCallReturnAddrDelta < 0)
       Offset -= TailCallReturnAddrDelta;
   }
 
   return Offset + FPDelta;
 }
 
 // Simplified from getFrameIndexReference keeping only StackPointer cases
-int X86FrameLowering::getFrameIndexReferenceFromSP(const MachineFunction &MF,
-                                                   int FI,
-                                                   unsigned &FrameReg) const {
+Optional<int>
+X86FrameLowering::getFrameIndexReferenceFromSP(const MachineFunction &MF,
+                                               int FI, unsigned &FrameReg,
+                                               bool AllowSPAdjustment) const {
   const MachineFrameInfo *MFI = MF.getFrameInfo();
   // Does not include any dynamic realign.
   const uint64_t StackSize = MFI->getStackSize();
-  {
-#ifndef NDEBUG
-    // LLVM arranges the stack as follows:
-    //   ...
-    //   ARG2
-    //   ARG1
-    //   RETADDR
-    //   PUSH RBP   <-- RBP points here
-    //   PUSH CSRs
-    //   ~~~~~~~    <-- possible stack realignment (non-win64)
-    //   ...
-    //   STACK OBJECTS
-    //   ...        <-- RSP after prologue points here
-    //   ~~~~~~~    <-- possible stack realignment (win64)
-    //
-    // if (hasVarSizedObjects()):
-    //   ...        <-- "base pointer" (ESI/RBX) points here
-    //   DYNAMIC ALLOCAS
-    //   ...        <-- RSP points here
-    //
-    // Case 1: In the simple case of no stack realignment and no dynamic
-    // allocas, both "fixed" stack objects (arguments and CSRs) are addressable
-    // with fixed offsets from RSP.
-    //
-    // Case 2: In the case of stack realignment with no dynamic allocas, fixed
-    // stack objects are addressed with RBP and regular stack objects with RSP.
-    //
-    // Case 3: In the case of dynamic allocas and stack realignment, RSP is used
-    // to address stack arguments for outgoing calls and nothing else. The "base
-    // 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.
 
-    assert((!MFI->isFixedObjectIndex(FI) || !TRI->needsStackRealignment(MF) ||
-            STI.isTargetWin64()) &&
-           "offset from fixed object to SP is not static");
-
-    // We don't handle tail calls, and shouldn't be seeing them either.
-    int TailCallReturnAddrDelta =
-        MF.getInfo<X86MachineFunctionInfo>()->getTCReturnAddrDelta();
-    assert(!(TailCallReturnAddrDelta < 0) && "we don't handle this case!");
-#endif
-  }
+  if (MFI->isFixedObjectIndex(FI) && TRI->needsStackRealignment(MF) &&
+      !STI.isTargetWin64())
+    return None;
+
+  // 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;
 
   // Fill in FrameReg output argument.
   FrameReg = TRI->getStackRegister();
 
   // This is how the math works out:
   //
   //  %rsp grows (i.e. gets lower) left to right. Each box below is
   //  one word (eight bytes).  Obj0 is the stack slot we're trying to

llvm/trunk/test/CodeGen/X86/deopt-bundles.ll

catch:
   %val2 = call i32 @callee_1(i32 100) "statepoint-id"="4243" [ "funclet"(token %cp1), "deopt"(i32 55) ]
   catchret from %cp1 to label %try.cont
 
 try.cont:
   ret i32 0
 }
 
 declare i32 @__CxxFrameHandler3(...)
+
+define void @f_0(i64 %n) {
+  ; CHECK-LABEL: _f_0
+  %s = alloca i64
+  %vl = alloca i64, i64 %n
+  ; Check that the stackmap does not reference %s through
+  ; SP since the offset is not static because of %vl.
+  ; STACKMAPS: Loc 3: Direct 6
+  call void @g_0(i64* %vl) [ "deopt"(i64* %s) ]
+  ret void
+}
+
+declare void @g_0(i64* %vl)