X86RegisterInfo: eliminateFrameIndex: Force SP in AfterFPPop case; NFC

[MatzeB]

Summary

AfterFPPop is used for tailcall/tailjump instructions. We shouldn't ever
have frame-pointer/base-pointer relative addressing for those. After all
the frame/base pointer should already be restored to their previous
values at the return.

Make this fact explicit in preparation for an upcoming refactoring.

[rnk]

lib/Target/X86/X86RegisterInfo.cpp

679 ↗

(On Diff #95662)

Why does this assert hold true? Do we not support tail calls when doing stack realignment?

[MatzeB]

lib/Target/X86/X86RegisterInfo.cpp

679 ↗

(On Diff #95662)

I hope we do not do create tail calls that load their target address from the stack in the realignment cases. If we do, then there is a high chance the existing code would miscompile as the frame/base pointer are already reset at the pointer of the tailcall I think.

May be worth some experimentation though, so I can answer this with more confidence...

[rnk]

lib/Target/X86/X86RegisterInfo.cpp

679 ↗

(On Diff #95662)

It seems plausible that we might tail call in this kind of code:

void useit(int*, int);
void f(void (*fp)(), int n) {
  int vla[n] __attribute__((aligned(64)));
  useit(&vla[0], n);
  fp(); // suppose 'fp' lives in a fixed stack object
}

We probably aren't clever enough to do this today, but maybe a better assert would be to check that FrameIndex is fixed?

[MatzeB]

lib/Target/X86/X86RegisterInfo.cpp

679 ↗

(On Diff #95662)

I played a bit more with this example. The Tail Call Elimination pass doesn't trigger on it, because the address to that local variable escapes. Fixing that we are running into

X86ISelLowering.cpp:4086:

// Can't do sibcall if stack needs to be dynamically re-aligned. PEI needs to
// emit a special epilogue.

So at least the needsStackRealignment() cause wouldn't work today.

Either way your suggestion to just assert on FrameIndex < 0 makes a lot of sense to me and I'll go for that.

[MatzeB]

Update assert as suggested.

[rnk]

Looks good, sorry about the pedantry 😀

[MatzeB]

In D32205#732124, @rnk wrote:

Looks good, sorry about the pedantry 😀

Thanks for the review. I explicitely split this change up from the rest to get some in-depth look at it!

[MatzeB]

As I feared this broke. One of the bots was compiling this pearl (from clang/lib/Lex/Pragma.cpp:992)

static void DebugOverflowStack() {
  void (*volatile Self)() = DebugOverflowStack;
  Self();
}

for which amazingly use an x86 memory operand on the tail call instruction. Seems like someone tried to trick the compiler into deliberately producing code that overflows the stack and we managed to optimize it into a sibling call without using extra stack (the red-zone is saving us from stack allocations).

[rnk]

In D32205#735651, @MatzeB wrote:

As I feared this broke. One of the bots was compiling this pearl (from clang/lib/Lex/Pragma.cpp:992)

static void DebugOverflowStack() {
  void (*volatile Self)() = DebugOverflowStack;
  Self();
}

for which amazingly use an x86 memory operand on the tail call instruction. Seems like someone tried to trick the compiler into deliberately producing code that overflows the stack and we managed to optimize it into a sibling call without using extra stack (the red-zone is saving us from stack allocations).

That's brilliant, because if we tail call, then this function won't cause a stack overflow! We might want to try harder by escaping Self to the call.

Regardless, I guess the assertion isn't that important. We could go back to asserting that no dynamic stack realignment occurs.

[MatzeB]

In D32205#735670, @rnk wrote:

In D32205#735651, @MatzeB wrote:

As I feared this broke. One of the bots was compiling this pearl (from clang/lib/Lex/Pragma.cpp:992)

static void DebugOverflowStack() {
  void (*volatile Self)() = DebugOverflowStack;
  Self();
}

for which amazingly use an x86 memory operand on the tail call instruction. Seems like someone tried to trick the compiler into deliberately producing code that overflows the stack and we managed to optimize it into a sibling call without using extra stack (the red-zone is saving us from stack allocations).

That's brilliant, because if we tail call, then this function won't cause a stack overflow! We might want to try harder by escaping Self to the call.

Regardless, I guess the assertion isn't that important. We could go back to asserting that no dynamic stack realignment occurs.

Guess I'll try with assert(!needsStackRealignment(MF) || MF.getFrameInfo().isFixedObjectIndex(FrameIndex)) this time.

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

X86RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II,
   const X86FrameLowering *TFI = getFrameLowering(MF);
   int FrameIndex = MI.getOperand(FIOperandNum).getIndex();
   unsigned BasePtr;
 
   unsigned Opc = MI.getOpcode();
   bool AfterFPPop = Opc == X86::TAILJMPm64 || Opc == X86::TAILJMPm ||
                     Opc == X86::TCRETURNmi || Opc == X86::TCRETURNmi64;
 
-  if (hasBasePointer(MF))
+  if (AfterFPPop) {
+    assert(FrameIndex < 0 && "Should only reference fixed stack objects here");
+    BasePtr = StackPtr;
+  } else if (hasBasePointer(MF))
     BasePtr = (FrameIndex < 0 ? FramePtr : getBaseRegister());
   else if (needsStackRealignment(MF))
     BasePtr = (FrameIndex < 0 ? FramePtr : StackPtr);
-  else if (AfterFPPop)
-    BasePtr = StackPtr;
   else
     BasePtr = (TFI->hasFP(MF) ? FramePtr : StackPtr);
 
   // LOCAL_ESCAPE uses a single offset, with no register. It only works in the
   // simple FP case, and doesn't work with stack realignment. On 32-bit, the
   // offset is from the traditional base pointer location.  On 64-bit, the
   // offset is from the SP at the end of the prologue, not the FP location. This
   // matches the behavior of llvm.frameaddress.