Page MenuHomePhabricator

[X86] ESP should not be in the regcall CSR list
ClosedPublic

Authored by rnk on Jan 5 2021, 1:48 PM.

Details

Summary

All calling conventions should manage ESP explicitly. Typically, they
either preserve it, or use a callee-cleanup convention. There is no need
to push and pop ESP directly. Its inclusion appears to have been an
oversight.

Diff Detail

Event Timeline

rnk created this revision.Jan 5 2021, 1:48 PM
rnk requested review of this revision.Jan 5 2021, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2021, 1:48 PM

I saw the document mentions ESP/RSP should be preserve for regcall. But I cannot think out a situation that ESP/RSP need to preserve either.

llvm/lib/Target/X86/X86CallingConv.td
1168

Why do we still preserve RSP for 64 bit?

rnk updated this revision to Diff 314779.Jan 5 2021, 7:19 PM
  • Remove RSP from CSR lists too
rnk added a comment.Jan 5 2021, 7:22 PM

I saw the document mentions ESP/RSP should be preserve for regcall. But I cannot think out a situation that ESP/RSP need to preserve either.

The document is correct, SP is preserved, but it's done by the standard SP adjustments, not by explicitly pushing and popping the SP register.

llvm/lib/Target/X86/X86CallingConv.td
1168

Looks like that shows up in codegen as well.

pengfei added inline comments.Jan 5 2021, 7:30 PM
llvm/lib/Target/X86/X86CallingConv.td
1103

except r11 and rsp?

1107

The same here.

rnk added inline comments.Jan 5 2021, 7:33 PM
llvm/lib/Target/X86/X86CallingConv.td
1103

I don't think that adds any clarification. RSP is preserved, just like you would expect of any regular, non callee-cleanup convention.

pengfei accepted this revision.Jan 5 2021, 7:49 PM

LGTM.

llvm/lib/Target/X86/X86CallingConv.td
1103

Yeah, you are right. I don't know what are these definition used for. Just saw it says all GPRs :-)

This revision is now accepted and ready to land.Jan 5 2021, 7:49 PM
This revision was automatically updated to reflect the committed changes.