[PATCH] D30052: [X86] Emit fewer instructions to allocate >16GB stack frames

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:293-311
+      unsigned PushOpc = Is64Bit ? X86::PUSH64r : X86::PUSH32r;
+      unsigned LoadOpc = Is64Bit ? X86::MOV64rm : X86::MOV32rm;
+      unsigned XchgOpc = Is64Bit ? X86::XCHG64rm : X86::XCHG32rm;
+      BuildMI(MBB, MBBI, DL, TII.get(PushOpc))
+          .addReg(Rax, RegState::Kill)
+          .setMIFlag(Flag);
+      BuildMI(MBB, MBBI, DL, TII.get(MovRIOpc), Rax)
----------------
sdardis wrote:
> Given the assert(Is64Bit..) above this hunk, can't PushOpc be replaced with PUSH64r directly? Likewise for LoadOpc, XchgOpc.
I kind of waffled on that assertion. I added it at the end after deciding on the 16GB stack frame threshold. I guess I'll do the simplification.

Also, it's easy to hit the assertion by running the new test targeting i686. Nothing else in our pass pipeline validates that stack objects are small enough to fit in the address space, which is pretty silly. This doesn't feel like the right place to report that, though.


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:300
+      BuildMI(MBB, MBBI, DL, TII.get(MovRIOpc), Rax)
+          .addImm(Offset)
+          .setMIFlag(Flag);
----------------
sdardis wrote:
> In the epilogue case, this needs to be Offset + SlotSize, as we need to account for the spill we just performed.
Thanks, nice catch!

I went ahead and actually did a manual execution test for this by tweaking the conditions to come here in the common case, and this executes correctly now. I also found a bug where x86's two operand SUB instruction was doing the wrong thing. I want to subtract RAX from RSP and store the result in RAX, which is not possible. Instead, I've negated the offset and used ADD.


https://reviews.llvm.org/D30052