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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


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


================
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))
----------------
Why need `this->`?


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2844-2845
+  // Clear the stack slot for spill base pointer register.
+  MachineFunction &MF = *MBB.getParent();
+  X86MachineFunctionInfo *X86FI = MF.getInfo<X86MachineFunctionInfo>();
+  if (X86FI->getRestoreBasePointer()) {
----------------
ditto.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:2848
+    unsigned Opc = STI.is64Bit() ? X86::POP64r : X86::POP32r;
+    Register BaseReg = this->TRI->getBaseRegister();
+    BuildMI(MBB, MI, DL, TII.get(Opc), BaseReg)
----------------
ditto.


================
Comment at: llvm/lib/Target/X86/X86MachineFunctionInfo.cpp:38-41
+void X86MachineFunctionInfo::setRestoreBasePointer(
+    unsigned CalleeSavedFrameSize) {
+  RestoreBasePointerOffset = -CalleeSavedFrameSize;
+}
----------------
This can be moved to X86MachineFunctionInfo.h


================
Comment at: llvm/test/CodeGen/X86/sjlj-baseptr.ll:25
+; X86: pushl   %esi
+; X86-NEXT: pushl   %esi
 ; X86: movl    %esp, %esi
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144625