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

[Dan Gohman]

On Oct 11, 2010, at 8:13 AM, David Meyer wrote:

> Dan,
> 
> Thanks for taking a look.
> 
> Grepping for <stdarg.h> in the testsuite and running the tests
> that finds would be good.
> 
> I just submitted separately (to llvm-commits) a patch for llvm-gcc which adds the option -fuse-llvm-va-arg. This causes llvm-gcc to always use the va_arg instruction. I will also submit a patch to Clang for the same thing when the first patch goes in.
> 
> It seems reasonable that I should add a new pass to the test-suite which runs the va_arg() tests with -fuse-llvm-va-arg enabled. Would you prefer I make that change and add it to this patch, or should I submit those separately?

I'm not sure what you mean by adding a new pass to the test-suite.  I
was thinking you'd turn on va_arg in your own tree and run some tests. If
things look good, then go ahead and commit a change which enables it
unconditionally.

In any case, it's fine to submit just this backend patch by itself first.
No one is using va_arg on x86-64 right now, so this patch won't break
anything.

> 
> 
> > 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.
> 
> Just to be on the safe side, do you think I should leave an assertion in place if va_arg is invoked for Cygwin, Win64, or MinGW?
> (this isn't any worse than the current code state. Right now it asserts for all x86-64 targets)

Yes; leaving an assertion in place seems best.

>  
> 
> > --- 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?
> 
> These snuck in, they are not critical for this patch. But there is some
> significance. I'm working on Native Client code generation at Google,
> which has the unusual condition that is64Bit() == true and
> getPointerTy() == MVT::i32. This change was needed to keep the
> va_list structure from changing. 

> (Eventually the entire Native Client target patch will be sent up, but for now, I am starting with the general features, e.g., va_arg)
> 
> Would it be better if I removed it from this change?

Yes; it'd be good to separate those kinds of changes into a
separate patch.

> 
> 
> > +  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.
> 
> I will add f32.
> 
> The gcc on my system supports __float128, and it passes this value using xmm registers. 
> I'm not sure what standard this is part of (GNU?), but is there any harm in supporting it ?

Apparently __float128 is in the x86-64.org ABI, so I guess it's fine to
leave the f128 check in.

>  
> 
> > +    // 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.
> 
> I'm not sure I understand what you mean here. Are you suggesting this instead?
> 
>   BuildMI(offsetMBB, DL, TII->get(X86::ADD64rr), OffsetDestReg)
>   .addReg(OffsetReg64)
>   .addReg(RegSaveReg);

Yes. It's not super important though.

Thanks,

Dan