On Sun, 25 Feb 2007, Nicolas Geoffray wrote:> No problem. Plus the reviewing may have taken some time. So thx a lot
> for committing. I talked to Jim who said
> he wanted to commit his changes before mine -- I hope everything's Ok.
There is one significant bug that fell out from this, which caused some
macho function calls to be compiled with ELF ABI semantics. I checked in
this patch to fix it:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070219/045027.html
I think you have to split PPCISD::CALL into PPCISD::CALL_ELF and
PPCISD::CALL_Macho, like you do for BCTRL. There were also a couple
missing patterns, together these caused some nightly tester failures on
darwin.
>> I applied the patches after some cleanups. Please keep code within 80
>> columns, please don't use nested ?: expressions without parens, and
please
>> be careful about indentation.
> Alright, sorry I forgot that. I'll be careful next time. Btw, if i have
> some bug fixes to do, what should I do? Ask for a write access? Or just
> do an RFC and waiting for someone to commit?
You are welcome to have commit-after-approval access. I'll contact you
offline. Please familiarize yourself with the developer policy:
http://llvm.org/docs/DeveloperPolicy.html
specifically:
http://llvm.org/docs/DeveloperPolicy.html#commitaccess
>> Please verify that mainline CVS has
>> everything you think it should.
>>
> I just verified and launched some compilations. Everything seems OK.
Ok, I just broke ELF support to refix darwin, so please take a look again
:)
>> The one hunk I didn't apply was this one:
>>
>> @@ -1392,12 +1418,13 @@
>> case MVT::f32:
>> case MVT::f64:
>> - if (isVarArg && isPPC64) {
>> + if (isVarArg || isPPC64) {
>> // Float varargs need to be promoted to double.
>> if (Arg.getValueType() == MVT::f32)
>> Arg = DAG.getNode(ISD::FP_EXTEND, MVT::f64, Arg);
>>
>> This changes the darwin/ppc ABI. It's not clear to me that this
was
>> intended, so I just left it out.
>>
> Ah yes, I changed this and forgot to mention it (it deserves a different
> commit - it's, as you noticed not, directly related to the linux/ppc
> abi). I think it should be a "or" instead of an "and" :
floats are
> promoted to double in a vararg call wether you're on ppc64 or ppc32.
> Maybe i'm missing something, anyone can confirm?
I spent quite a bit of time investigating this. As it turns out, llvm-gcc
automatically does the promotion before the code generator saw this, so it
never could occur before. I changed the check to:
if (isVarArg) {
// Float varargs need to be promoted to double.
if (Arg.getValueType() == MVT::f32)
Arg = DAG.getNode(ISD::FP_EXTEND, MVT::f64, Arg);
}
which is correct (non-vararg floats passed to functions on ppc64 don't
need extension).
Nice catch.
>> Thanks a lot for these patches. How well does linux/ppc work now?
>>
>>
> Thanks to you for reviewing and committing.
> I would like to say it works great :) but I know there is one bug
> I have to found because I sometimes seem to get an infinite loop.
>
> If others have the opportunity to test it, please give us some feedback.
Ok!
-Chris
--
http://nondot.org/sabre/
http://llvm.org/