[PATCH] D40805: [RISCV] Support for varargs

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:701
+    // If saving an odd-number of registers, create an extra stack slot to
+    // ensure even-numbered registers are always 2*XLEN-aligned.
+    if (Idx % 2) {
----------------
xiangzhai wrote:
> efriedma wrote:
> > xiangzhai wrote:
> > > efriedma wrote:
> > > > 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.
> > > Hi Eli,
> > > 
> > > I am reviewing [Compiler Principle](https://en.wikipedia.org/wiki/Compilers:_Principles,_Techniques,_and_Tools) 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 
> > 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?
> Perhaps I need to review Compiler Principle's chapter 7.2.4 `VarArg in stack` and 7.3.5 Access Link more carefully :)
This is a requirement of the calling convention, see [here](https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#integer-calling-convention). 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.


================
Comment at: test/CodeGen/RISCV/vararg.ll:3
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
+; RUN:   | FileCheck -check-prefix=RV32I %s
+
----------------
efriedma wrote:
> Could you generate 64-bit tests as well?  (It probably isn't that different, but nice to have.)
RV64I codegen isn't upstream yet.


https://reviews.llvm.org/D40805