<div class="gmail_quote"><div>Dan,</div><div><br></div><div>Thanks for taking a look.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>Grepping for <stdarg.h> in the testsuite and running the tests</div>
that finds would be good.</blockquote><div><br></div><div>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.</div>
<div><br></div><div>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?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
> 3) Whether the default expansion of VAARG is actually suitable for Win64.<br>
> 4) Whether Darwin and Cygwin follow the same ABI.<br>
<br>
</div>Darwin does (in this area). I don't know about Cygwin or Win64.<br></blockquote><div><br></div><div>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?</div>
<div>(this isn't any worse than the current code state. Right now it asserts for all x86-64 targets)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> --- lib/Target/X86/X86ISelLowering.cpp (revision 115912)<br>
> +++ lib/Target/X86/X86ISelLowering.cpp (working copy)<br>
<br>
> - getPointerTy());<br>
> + MVT::i64);<br>
<br>
Is there any special significance to these edits, or are they just<br>
simplifications?<br></blockquote><div><br></div><div>These snuck in, they are not critical for this patch. But there is some</div><div>significance. I'm working on Native Client code generation at Google,</div><div>which has the unusual condition that is64Bit() == true and</div>
<div>getPointerTy() == MVT::i32. This change was needed to keep the</div><div>va_list structure from changing. </div><div><br></div><div>(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)</div>
<div><br></div><div>Would it be better if I removed it from this change?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + if (ArgVT == MVT::f64 || ArgVT == MVT::f128) {<br>
<br>
This should also check for f32. Even though f32 can't happen from<br>
C, it can happen in LLVM IR. Also, this code should check for the<br>
various XMM vector types -- v4f32 and friends.<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Also, this code shouldn't check for f128, since that type isn't<br>
legal on x86-64.<br></blockquote><div><br></div><div>I will add f32.</div><div><br></div><div>The gcc on my system supports __float128, and it passes this value using xmm registers. </div><div>I'm not sure what standard this is part of (GNU?), but is there any harm in supporting it ?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + // Add the offset to the reg_save_area to get the final address.<br>
> + BuildMI(offsetMBB, DL, TII->get(X86::ADD64rr), OffsetDestReg)<br>
> + .addReg(RegSaveReg)<br>
> + .addReg(OffsetReg64);<br><br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Since this is RegSaveReg's last use, it'd be slightly nicer to swap it<br>
and OffsetReg64 here, to save TwoAddressLowering the trouble.<br></blockquote><div><br></div><div>I'm not sure I understand what you mean here. Are you suggesting this instead?</div><div><br></div><div> BuildMI(offsetMBB, DL, TII->get(X86::ADD64rr), OffsetDestReg)</div>
<div>
.addReg(OffsetReg64)</div><div> .addReg(RegSaveReg);</div><div><br></div><div>Thanks,</div><div>- David M </div></div><br>