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

[David Meyer]

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?


> > 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)


>
> > --- 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?


> > +  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 ?


>
> > +    // 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);

Thanks,
- David M
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101011/e9258454/attachment.html>