[X86] Fix the incorrect displacement for prolog/epilog

[LuoYuanke]

Summary

The bug is introduced in rGe4ceb5a7bb9b which set the wrong offset from
the stack base. This patch is to fix the bug.

[pengfei]

llvm/lib/Target/X86/X86FrameLowering.cpp

1590

Curious why using 2 before. I was thinking it's related to stack aligement. AFAIK, Windows requires the stack always aligned to 16B on 64-bits.

Is better to directly int64_t Offset = -SlotSize?

2327

ditto.

llvm/test/CodeGen/X86/i386-baseptr.ll

77

The dwarf doesn't need to update together?

[LuoYuanke]

llvm/lib/Target/X86/X86FrameLowering.cpp

1590

Because my previous version is to push a callee saved register push %rbx as well, and then I realized that it is more complex on unwind support, and I turn to a scratch register which doesn't need to be saved and restore. But I forget to modify the offset. I'm developing a runtime test case to cover this issue.

llvm/test/CodeGen/X86/i386-baseptr.ll

77

The dwarf seems correct. The ecx pointer to the base stack, and esp = ecx - 4 pointer to the offset 4 which is the function return address.

[pengfei]

LGTM.

[LuoYuanke]

llvm/lib/Target/X86/X86FrameLowering.cpp

1590

I got regression with below change. It is wired to me. Maybe it is due to the type of SlotSize is unsigned.

diff --git a/llvm/lib/Target/X86/X86FrameLowering.cpp b/llvm/lib/Target/X86/X86FrameLowering.cpp
index 15f39a509e1a..f3f21f0ac46d 100644
--- a/llvm/lib/Target/X86/X86FrameLowering.cpp
+++ b/llvm/lib/Target/X86/X86FrameLowering.cpp
@@ -1587,7 +1587,7 @@ void X86FrameLowering::emitPrologue(MachineFunction &MF,
                MachineInstr::FrameSetup);
     }
     BuildStackAlignAND(MBB, MBBI, DL, StackPtr, MaxAlign);
-    int64_t Offset = -1 * (int64_t)SlotSize;
+    int64_t Offset = -SlotSize;
     BuildMI(MBB, MBBI, DL, TII.get(Is64Bit ? X86::PUSH64rmm: X86::PUSH32rmm))
         .addReg(ArgBaseReg)
         .addImm(1)
@@ -2324,6 +2324,7 @@ void X86FrameLowering::emitEpilogue(MachineFunction &MF,
         .addUse(ArgBaseReg)
         .addImm(1)
         .addUse(X86::NoRegister)
+        // .addImm((int64_t)-SlotSize)
         .addImm((int64_t)SlotSize * -1)
         .addUse(X86::NoRegister)
         .setMIFlag(MachineInstr::FrameDestroy);

[pengfei]

llvm/lib/Target/X86/X86FrameLowering.cpp

1590

How about -(int64_t)SlotSize?

[LuoYuanke]

llvm/lib/Target/X86/X86FrameLowering.cpp

1590

It works. :) I'll adopt this version.

[LuoYuanke]

Address Phoebe's comments.

[rupprecht]

Test are passing again after this patch, so I think we're all good now. Thanks!

llvm/lib/Target/X86/X86FrameLowering.cpp

if (auto *MI = X86FI->getStackPtrSaveMI()) {
     if (NeedsDwarfCFI) {
       // .cfi_def_cfa %basereg, 0
       unsigned DwarfStackPtr = TRI->getDwarfRegNum(ArgBaseReg, true);
       BuildCFI(MBB, MBBI, DL,
                MCCFIInstruction::cfiDefCfa(nullptr, DwarfStackPtr, 0),
                MachineInstr::FrameSetup);
     }
     BuildStackAlignAND(MBB, MBBI, DL, StackPtr, MaxAlign);
-    int64_t Offset = Is64Bit ? -2 * (int64_t)SlotSize : -1 * (int64_t)SlotSize;
+    int64_t Offset = -(int64_t)SlotSize;
     BuildMI(MBB, MBBI, DL, TII.get(Is64Bit ? X86::PUSH64rmm: X86::PUSH32rmm))
         .addReg(ArgBaseReg)
         .addImm(1)
         .addReg(X86::NoRegister)
         .addImm(Offset)
         .addReg(X86::NoRegister)
         .setMIFlag(MachineInstr::FrameSetup);
   }
void X86FrameLowering::emitEpilogue(MachineFunction &MF,
   if (auto *MI = X86FI->getStackPtrSaveMI()) {
     unsigned Opc = X86::LEA32r;
     Register StackReg = X86::ESP;
     ArgBaseReg = MI->getOperand(0).getReg();
     if (STI.is64Bit()) {
       Opc = X86::LEA64r;
       StackReg = X86::RSP;
     }
-    // leal    -8(%basereg), %esp
+    // leal    -4(%basereg), %esp
     // .cfi_def_cfa %esp, 4
     BuildMI(MBB, MBBI, DL, TII.get(Opc), StackReg)
         .addUse(ArgBaseReg)
         .addImm(1)
         .addUse(X86::NoRegister)
-        .addImm((int64_t)SlotSize * -2)
+        .addImm(-(int64_t)SlotSize)
         .addUse(X86::NoRegister)
         .setMIFlag(MachineInstr::FrameDestroy);
     if (NeedsDwarfCFI) {
       unsigned DwarfStackPtr = TRI->getDwarfRegNum(StackReg, true);
       BuildCFI(MBB, MBBI, DL,
                MCCFIInstruction::cfiDefCfa(nullptr, DwarfStackPtr, SlotSize),
                MachineInstr::FrameDestroy);
       --MBBI;

llvm/test/CodeGen/X86/i386-baseptr.ll

 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    movl %edx, -120(%ebp)
 ; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:    movl $0, (%ecx,%eax)
 ; CHECK-NEXT:    movl {{[-0-9]+}}(%e{{[sb]}}p), %ecx # 4-byte Reload
 ; CHECK-NEXT:    leal -4(%ebp), %esp
 ; CHECK-NEXT:    popl %esi
 ; CHECK-NEXT:    popl %ebp
-; CHECK-NEXT:    leal -8(%ecx), %esp
+; CHECK-NEXT:    leal -4(%ecx), %esp
 ; CHECK-NEXT:    .cfi_def_cfa %esp, 4
 ; CHECK-NEXT:    retl
 entry:
   %k = call i32 @helper()
   %a = alloca i32, align 128
   %b = alloca i32, i32 %k, align 4
   ; clobber base pointer register
   tail call void asm sideeffect "nop", "{si}"(i32 405)
   call void asm sideeffect "movl $0, $1", "r,*m"(i32 8, ptr elementtype(i32) %a)

llvm/test/CodeGen/X86/x86-64-baseptr.ll

 }
 
 define void @clobber_base() #0 {
 ; CHECK-LABEL: clobber_base:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    leaq {{[0-9]+}}(%rsp), %r10
 ; CHECK-NEXT:    .cfi_def_cfa %r10, 0
 ; CHECK-NEXT:    andq $-128, %rsp
-; CHECK-NEXT:    pushq -16(%r10)
+; CHECK-NEXT:    pushq -8(%r10)
 ; CHECK-NEXT:    pushq %rbp
 ; CHECK-NEXT:    movq %rsp, %rbp
 ; CHECK-NEXT:    .cfi_escape 0x10, 0x06, 0x02, 0x76, 0x00 #
 ; CHECK-NEXT:    pushq %rbx
 ; CHECK-NEXT:    subq $232, %rsp
 ; CHECK-NEXT:    movq %r10, {{[-0-9]+}}(%r{{[sb]}}p) # 8-byte Spill
 ; CHECK-NEXT:    .cfi_escape 0x10, 0x03, 0x02, 0x76, 0x78 #
 ; CHECK-NEXT:    .cfi_escape 0x0f, 0x04, 0x76, 0x88, 0x7f, 0x06 #

 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    movl %edx, -112(%rbp)
 ; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:    movl $0, (%rcx,%rax)
 ; CHECK-NEXT:    movq {{[-0-9]+}}(%r{{[sb]}}p), %r10 # 8-byte Reload
 ; CHECK-NEXT:    leaq -8(%rbp), %rsp
 ; CHECK-NEXT:    popq %rbx
 ; CHECK-NEXT:    popq %rbp
-; CHECK-NEXT:    leaq -16(%r10), %rsp
+; CHECK-NEXT:    leaq -8(%r10), %rsp
 ; CHECK-NEXT:    .cfi_def_cfa %rsp, 8
 ; CHECK-NEXT:    retq
 ;
 ; X32ABI-LABEL: clobber_base:
 ; X32ABI:       # %bb.0: # %entry
 ; X32ABI-NEXT:    pushq %rbp
 ; X32ABI-NEXT:    .cfi_def_cfa_offset 16
 ; X32ABI-NEXT:    .cfi_offset %rbp, -16

 }
 
 define x86_regcallcc void @clobber_baseptr_argptr(i32 %param1, i32 %param2, i32 %param3, i32 %param4, i32 %param5, i32 %param6, i32 %param7, i32 %param8, i32 %param9, i32 %param10, i32 %param11, i32 %param12) #0 {
 ; CHECK-LABEL: clobber_baseptr_argptr:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    leaq {{[0-9]+}}(%rsp), %r10
 ; CHECK-NEXT:    .cfi_def_cfa %r10, 0
 ; CHECK-NEXT:    andq $-128, %rsp
-; CHECK-NEXT:    pushq -16(%r10)
+; CHECK-NEXT:    pushq -8(%r10)
 ; CHECK-NEXT:    pushq %rbp
 ; CHECK-NEXT:    movq %rsp, %rbp
 ; CHECK-NEXT:    .cfi_escape 0x10, 0x06, 0x02, 0x76, 0x00 #
 ; CHECK-NEXT:    pushq %rbx
 ; CHECK-NEXT:    subq $360, %rsp # imm = 0x168
 ; CHECK-NEXT:    movaps %xmm15, {{[-0-9]+}}(%r{{[sb]}}p) # 16-byte Spill
 ; CHECK-NEXT:    movaps %xmm14, {{[-0-9]+}}(%r{{[sb]}}p) # 16-byte Spill
 ; CHECK-NEXT:    movaps %xmm13, {{[-0-9]+}}(%r{{[sb]}}p) # 16-byte Spill

 ; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm12 # 16-byte Reload
 ; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm13 # 16-byte Reload
 ; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm14 # 16-byte Reload
 ; CHECK-NEXT:    movaps {{[-0-9]+}}(%r{{[sb]}}p), %xmm15 # 16-byte Reload
 ; CHECK-NEXT:    movq {{[-0-9]+}}(%r{{[sb]}}p), %r10 # 8-byte Reload
 ; CHECK-NEXT:    leaq -8(%rbp), %rsp
 ; CHECK-NEXT:    popq %rbx
 ; CHECK-NEXT:    popq %rbp
-; CHECK-NEXT:    leaq -16(%r10), %rsp
+; CHECK-NEXT:    leaq -8(%r10), %rsp
 ; CHECK-NEXT:    .cfi_def_cfa %rsp, 8
 ; CHECK-NEXT:    retq
 ;
 ; X32ABI-LABEL: clobber_baseptr_argptr:
 ; X32ABI:       # %bb.0: # %entry
 ; X32ABI-NEXT:    pushq %rbp
 ; X32ABI-NEXT:    .cfi_def_cfa_offset 16
 ; X32ABI-NEXT:    .cfi_offset %rbp, -16