[PATCH] D32205: X86RegisterInfo: eliminateFrameIndex: Force SP in AfterFPPop case; NFC

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: lib/Target/X86/X86RegisterInfo.cpp:679
+  if (AfterFPPop) {
+    assert(!hasBasePointer(MF));
+    assert(!needsStackRealignment(MF));
----------------
rnk wrote:
> MatzeB wrote:
> > rnk wrote:
> > > Why does this assert hold true? Do we not support tail calls when doing stack realignment?
> > 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...
> 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?
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D32205