This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix the base pointer save/restore bug
ClosedPublic

Authored by LuoYuanke on Feb 23 2023, 1:13 AM.

Details

Summary

Previous the stack slot for spilling base pointer register is allocated
after the stack realignment. When the stack is naturally aligned the
stack slot is share with other data that allocated from stack and cause
data corrupt. Another issue is the stack slot for save/restore the
callee saved register is not fixed for each function. It depends on the
register usage of them in each function.

This patch is to recalculate the offset the stack slot for base pointer
register during the prolog/epilog insert pass, and allocate the stack
slot when spilling callee saved registers.

Diff Detail

Event Timeline

LuoYuanke created this revision.Feb 23 2023, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 1:13 AM
LuoYuanke requested review of this revision.Feb 23 2023, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 1:13 AM
LuoYuanke added inline comments.
llvm/test/CodeGen/X86/sjlj-baseptr.ll
27

The offset happen to be the same.

LuoYuanke updated this revision to Diff 499815.Feb 23 2023, 4:40 AM

update test case.

It's been a decade since I've worked with LLVM. The change looks okay to me.

It's been a decade since I've worked with LLVM. The change looks okay to me.

Thanks for review. Would you accept the patch, so that I can land it?

pengfei added inline comments.Feb 25 2023, 2:40 AM
llvm/lib/Target/X86/X86FrameLowering.cpp
2737

Should add const here rather than remove from line 2703?

2740

Why need this->?

2844–2845

ditto.

2848

ditto.

llvm/lib/Target/X86/X86MachineFunctionInfo.cpp
38–41

This can be moved to X86MachineFunctionInfo.h

llvm/test/CodeGen/X86/sjlj-baseptr.ll
25

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?

LuoYuanke added inline comments.Feb 25 2023, 3:18 AM
llvm/lib/Target/X86/X86FrameLowering.cpp
2737

Yes, it looks better.

2740

There is "const TargetRegisterInfo *TRI" passed from argument. "this->TRI" is X86TRI.

llvm/test/CodeGen/X86/sjlj-baseptr.ll
25

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.

LuoYuanke updated this revision to Diff 500407.Feb 25 2023, 3:19 AM

Address Phoebe's comments.

pengfei accepted this revision.Feb 25 2023, 4:26 AM

LGTM.

llvm/lib/Target/X86/X86MachineFunctionInfo.cpp
37

Recover the change here.

This revision is now accepted and ready to land.Feb 25 2023, 4:26 AM
This revision was landed with ongoing or failed builds.Feb 25 2023, 5:13 AM
This revision was automatically updated to reflect the committed changes.