[PATCH] D146862: [X86] Fix the incorrect displacement for prolog/epilog

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:1590
     BuildStackAlignAND(MBB, MBBI, DL, StackPtr, MaxAlign);
-    int64_t Offset = Is64Bit ? -2 * (int64_t)SlotSize : -1 * (int64_t)SlotSize;
+    int64_t Offset = -1 * (int64_t)SlotSize;
     BuildMI(MBB, MBBI, DL, TII.get(Is64Bit ? X86::PUSH64rmm: X86::PUSH32rmm))
----------------
pengfei wrote:
> LuoYuanke wrote:
> > LuoYuanke wrote:
> > > pengfei wrote:
> > > > 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`?
> > > 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.
> > 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);
> > 
> > ```
> How about `-(int64_t)SlotSize`?
It works. :) I'll adopt this version.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146862/new/

https://reviews.llvm.org/D146862