Page MenuHomePhabricator

[RISCV] Support for varargs
ClosedPublic

Authored by asb on Dec 4 2017, 2:21 PM.

Details

Summary

Includes support for expanding va_copy. Also adds support for using 'aligned' registers when necessary for vararg calls, and ensure the frame pointer always points to the bottom of the vararg spill region. This is necessary to ensure that the saved return address and stack pointer are always available at fixed known offsets of the frame pointer.

See https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md for calling convention documentation.

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.Dec 4 2017, 2:21 PM
sabuasal added inline comments.Dec 7 2017, 3:44 PM
test/CodeGen/RISCV/vararg.ll
144 ↗(On Diff #125416)

"where the first"

asb updated this revision to Diff 126151.Dec 8 2017, 7:13 AM
asb marked an inline comment as done.

Rebase patch and fix typo in test code comment.

asb updated this revision to Diff 127100.Dec 15 2017, 4:33 AM

Rebasing patch. Ping!

asb updated this revision to Diff 127323.Dec 18 2017, 3:37 AM

Update patch after r320884 (MachineFunction::getFunction now returns a reference).

xiangzhai added inline comments.Jan 3 2018, 7:35 PM
lib/Target/RISCV/RISCVISelLowering.cpp
446 ↗(On Diff #127323)

RegIdx % 2 == 1 is an odd register, not an even, but the comment is "ensure it is assigned to an even register".

asb added inline comments.Jan 4 2018, 6:51 AM
lib/Target/RISCV/RISCVISelLowering.cpp
446 ↗(On Diff #127323)

The comment and code are correct. If we would use an odd register, we 'waste' it by mark it as allocated to ensure that an even register will be used. I'll add an extra comment to make it a bit clearer - thanks for the feedback.

xiangzhai added inline comments.Jan 4 2018, 7:22 PM
lib/Target/RISCV/RISCVISelLowering.cpp
446 ↗(On Diff #127323)

Thanks for your teaching!

efriedma added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
701 ↗(On Diff #127323)

Could you clarify what "ensure even-numbered registers are always 2*XLEN-aligned" means? The varargs save area is always right next to any arguments passed on the stack, so I'm not sure what exactly you need to align.

test/CodeGen/RISCV/vararg.ll
3 ↗(On Diff #127323)

Could you generate 64-bit tests as well? (It probably isn't that different, but nice to have.)

297 ↗(On Diff #127323)

This looks a little weird; why are we loading sp+24 into a scratch register?

xiangzhai added inline comments.Jan 8 2018, 6:01 PM
lib/Target/RISCV/RISCVISelLowering.cpp
701 ↗(On Diff #127323)

Hi Eli,

I am reviewing Compiler Principle chapter 7.2 Stack Allocation, then consider about this question, because Strict Alignment? just like GCC https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00011.html Please teach me, thanks a lot!

Regards,
Leslie Zhai

efriedma added inline comments.Jan 8 2018, 6:32 PM
lib/Target/RISCV/RISCVISelLowering.cpp
701 ↗(On Diff #127323)

Yes, in general, some things need to be aligned, for a variety of reasons (including the issue you noted). I'm not questioning that part.

The question is more about the stack layout. Why do we need to allocate an extra object here to ensure alignment? What stack objects are we ensuring the alignment of?

xiangzhai added inline comments.Jan 8 2018, 7:02 PM
lib/Target/RISCV/RISCVISelLowering.cpp
701 ↗(On Diff #127323)

Perhaps I need to review Compiler Principle's chapter 7.2.4 VarArg in stack and 7.3.5 Access Link more carefully :)

asb updated this revision to Diff 129085.Jan 9 2018, 7:41 AM
asb marked 3 inline comments as done.

Update code comments to address feedback.

lib/Target/RISCV/RISCVISelLowering.cpp
701 ↗(On Diff #127323)

This is a requirement of the calling convention, see here. I believe the intention is to ensure that when the vararg registers are written to the stack they are done so with correct alignment, meaning va_arg can assume correct alignment regardless of whether a parameter was originally passed in an argument register or the stack. Assigning 'aligned' registers is no use if a2 is unaligned on the stack.

test/CodeGen/RISCV/vararg.ll
3 ↗(On Diff #127323)

RV64I codegen isn't upstream yet.

efriedma added inline comments.Jan 9 2018, 11:24 AM
lib/Target/RISCV/RISCVISelLowering.cpp
701 ↗(On Diff #127323)

I'm still not following; even-numbered registers are always 2*XLEN-aligned because the vararg save area must be adjacent to arguments passed on the stack. Like so:

stack arg
stack arg (XLEN*2 aligned)
saved a7
saved a6 (XLEN*2 aligned)
saved a5
saved a4 (XLEN*2 aligned)
saved a3
saved a2 (XLEN*2 aligned)
saved a1
saved a0 (XLEN*2 aligned)

This is the one and only layout, no matter how many argregs we save. If some of these arguments are fixed, you can skip saving them (and use the space for other stack objects, or just let the space extend beyond the stack pointer), but the layout doesn't fundamentally change. For example, if we save five registers (because there are three fixed registers), the layout is something like this:

stack arg
stack arg (XLEN*2 aligned)
saved a7
saved a6 (XLEN*2 aligned)
saved a5
saved a4 (XLEN*2 aligned)
saved a3 (va_start points here)
PADDING (XLEN*2 aligned) (frame pointer points here?)
(unused space)
(unused space) (XLEN*2 aligned)

Assuming my model is right, what purpose does the padding serve? The only effect I can see is that it changes the alignment of the frame pointer (which the comment doesn't mention at all).

asb updated this revision to Diff 129220.Jan 10 2018, 1:48 AM

Updated to clarify a comment based on Eli's feedback (thanks!).

lib/Target/RISCV/RISCVISelLowering.cpp
701 ↗(On Diff #127323)

Your analysis is correct, this code doesn't change the alignment of the data on the stack, only the aligment of the frame pointer (and hence the offsets codegen uses to access it). I've updated the comment accordingly. Thanks for your careful reading of this, I really appreciate it.

test/CodeGen/RISCV/vararg.ll
297 ↗(On Diff #127323)

That's the frame pointer.

This revision is now accepted and ready to land.Jan 10 2018, 10:56 AM
kparzysz added inline comments.Jan 10 2018, 11:19 AM
lib/Target/RISCV/RISCVISelLowering.cpp
719 ↗(On Diff #129220)

You could use MachinePointerInfo::getFixedStack instead of an empty pointer info to get more accurate aliasing.

kparzysz added inline comments.Jan 10 2018, 11:31 AM
lib/Target/RISCV/RISCVISelLowering.cpp
719 ↗(On Diff #129220)

Other than that, LGTM.

asb added inline comments.Jan 10 2018, 11:35 AM
lib/Target/RISCV/RISCVISelLowering.cpp
719 ↗(On Diff #129220)

Good spot, making that change and applying now. Thanks for the reviews everyone.

Closed by commit rL322215: [RISCV] Support for varargs (authored by asb, committed by ). · Explain WhyJan 10 2018, 11:42 AM
This revision was automatically updated to reflect the committed changes.