[PATCH] D144625: [X86] Fix the base pointer save/restore bug
LuoYuanke via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Feb 25 03:18:51 PST 2023
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
More information about the llvm-commits
mailing list