[PATCH] D144625: [X86] Fix the base pointer save/restore bug

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2737
 
+  X86MachineFunctionInfo *X86FI = MF.getInfo<X86MachineFunctionInfo>();
+  if (X86FI->getRestoreBasePointer()) {
----------------
pengfei wrote:
> Should add `const` here rather than remove from line 2703?
Yes, it looks better.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2740
+    unsigned Opc = STI.is64Bit() ? X86::PUSH64r : X86::PUSH32r;
+    Register BaseReg = this->TRI->getBaseRegister();
+    BuildMI(MBB, MI, DL, TII.get(Opc))
----------------
pengfei wrote:
> Why need `this->`?
There is "const TargetRegisterInfo *TRI" passed from argument. "this->TRI" is X86TRI.


================
Comment at: llvm/test/CodeGen/X86/sjlj-baseptr.ll:25
+; X86: pushl   %esi
+; X86-NEXT: pushl   %esi
 ; X86: movl    %esp, %esi
----------------
pengfei wrote:
> No quite understand why do we need push twice. Is it because the alignment of alloca is `16`? How about align to 32?
> 
> I found previously only sjlj code calls `setRestoreBasePointer`, but the change here seems more general, should we add a general test for it?
It is not for alignment. It is used to allocate the spill stack slot for base pointer register.
There are 2 more test case in https://reviews.llvm.org/D144541, this patch focuses on the bug fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144625