[llvm-commits] [patch] va_arg for X86-64

[Dan Gohman]

On Oct 7, 2010, at 7:49 AM, David Meyer wrote:

> Hi,
> 
> Attached is an initial implementation of va_arg for X86-64 (against revision 115912).
> 
> It should work for all basic types. I have tested it on Linux only. 
> 
> This is my first patch, I do not have commit access. I'd appreciate feedback regarding:
> 
> 1) Code correctness (of course)

At a quick readthrough, the code looks good, and it looks like it's
basically doing the right thing. I have a few comments below.

> 2) Strategies for testing this new feature

Grepping for <stdarg.h> in the testsuite and running the tests
that finds would be good.

> 3) Whether the default expansion of VAARG is actually suitable for Win64.
> 4) Whether Darwin and Cygwin follow the same ABI.

Darwin does (in this area). I don't know about Cygwin or Win64.

> --- lib/Target/X86/X86ISelLowering.cpp  (revision 115912)
> +++ lib/Target/X86/X86ISelLowering.cpp  (working copy)

> -                                    getPointerTy());
> +                                    MVT::i64);

Is there any special significance to these edits, or are they just
simplifications?

> +  if (ArgVT == MVT::f64 || ArgVT == MVT::f128) {

This should also check for f32. Even though f32 can't happen from
C, it can happen in LLVM IR. Also, this code should check for the
various XMM vector types -- v4f32 and friends.

Also, this code shouldn't check for f128, since that type isn't
legal on x86-64.

> +    // Add the offset to the reg_save_area to get the final address.
> +    BuildMI(offsetMBB, DL, TII->get(X86::ADD64rr), OffsetDestReg)
> +      .addReg(RegSaveReg)
> +      .addReg(OffsetReg64);

Since this is RegSaveReg's last use, it'd be slightly nicer to swap it
and OffsetReg64 here, to save TwoAddressLowering the trouble.

Dan