llvm dev - Oct 2012 - [LLVMdev] [cfe-commits] [PATCH/RFC, PowerPC] Extend 32-bit function arguments / return values

Hi Hal,
>> Chris wrote some notes about this:
>>     http://www.nondot.org/sabre/LLVMNotes/ExtendedIntegerResults.txt
>> The plan seems sensible to me, but was blocked by not having a good
>> way of attaching the information to parameters and return values.
>> Hopefully Bill's attribute work means it is now possible.
>
> Indeed. Are you proposing that Ulrich hold off on the current fix in favor
of the new attribute scheme, or that he should fix this using the current
mechanism for now (perhaps with a FIXME to upgrade to the new scheme once it is
in place)? I would prefer for him to commit this now, and then upgrade later.
I don't have anything useful to say about Ulrich's fix since I
didn't look at
it.  I just wanted to make sure that everyone knew where we are probably going:
in the direction laid out by Chris in those notes.

Ciao, Duncan.

Duncan Sands <duncan.sands at gmail.com> wrote on 24.10.2012
08:20:07:
> Hal Finkel <hfinkel at anl.gov> wrote:
> >> Chris wrote some notes about this:
> >>    
http://www.nondot.org/sabre/LLVMNotes/ExtendedIntegerResults.txt
> >> The plan seems sensible to me, but was blocked by not having a
good
> >> way of attaching the information to parameters and return values.
> >> Hopefully Bill's attribute work means it is now possible.
> >
> > Indeed. Are you proposing that Ulrich hold off on the current fix
> in favor of the new attribute scheme, or that he should fix this
> using the current mechanism for now (perhaps with a FIXME to upgrade
> to the new scheme once it is in place)? I would prefer for him to
> commit this now, and then upgrade later.
>
> I don't have anything useful to say about Ulrich's fix since I
didn't
look at
> it.  I just wanted to make sure that everyone knew where we are
> probably going:
> in the direction laid out by Chris in those notes.

I'm wondering how to proceed on this issue for now.  Current status is
that I've checked in the testsuite patch, since there was agreement
that fixing the tests by adding target triples was the way to go.
However, the actual fix to enable the sign-extensions required by
the PowerPC64 ABI is not in, so we still generate code that violates
the ABI.

I understand that there is a long-term goal of having those extensions
done in the front end instead of in LLVM.  However, as far as I can
see the necessary infrastructure is not yet fully present (or in any
event, I don't see how to do it in the front end right now) ...

Therefore, since my patch doesn't make moving to the new scheme
any more difficult (it just does the same thing for "int" that is
already being done for "short" and "char"), and it does fix
the
ABI bug we have right now, I'd propose to check it in now (and
then move to new scheme with everyone else once it is ready).

Would this be OK?  Any other suggestions?

For reference, the patch in question is appended again.

Thanks,
Ulrich

(See attached file: diff-clang-ppc64-extend)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: diff-clang-ppc64-extend
Type: application/octet-stream
Size: 3418 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121031/130dcc79/attachment.obj>

On Wed, Oct 31, 2012 at 7:44 AM, Ulrich Weigand
<Ulrich.Weigand at de.ibm.com> wrote:
> Duncan Sands <duncan.sands at gmail.com> wrote on 24.10.2012
08:20:07:
>> Hal Finkel <hfinkel at anl.gov> wrote:
>
>> >> Chris wrote some notes about this:
>> >>    
http://www.nondot.org/sabre/LLVMNotes/ExtendedIntegerResults.txt
>> >> The plan seems sensible to me, but was blocked by not having a
good
>> >> way of attaching the information to parameters and return
values.
>> >> Hopefully Bill's attribute work means it is now possible.
>> >
>> > Indeed. Are you proposing that Ulrich hold off on the current fix
>> in favor of the new attribute scheme, or that he should fix this
>> using the current mechanism for now (perhaps with a FIXME to upgrade
>> to the new scheme once it is in place)? I would prefer for him to
>> commit this now, and then upgrade later.
>>
>> I don't have anything useful to say about Ulrich's fix since I
didn't
> look at
>> it.  I just wanted to make sure that everyone knew where we are
>> probably going:
>> in the direction laid out by Chris in those notes.
>
>
> I'm wondering how to proceed on this issue for now.  Current status is
> that I've checked in the testsuite patch, since there was agreement
> that fixing the tests by adding target triples was the way to go.
> However, the actual fix to enable the sign-extensions required by
> the PowerPC64 ABI is not in, so we still generate code that violates
> the ABI.
>
> I understand that there is a long-term goal of having those extensions
> done in the front end instead of in LLVM.  However, as far as I can
> see the necessary infrastructure is not yet fully present (or in any
> event, I don't see how to do it in the front end right now) ...
>
> Therefore, since my patch doesn't make moving to the new scheme
> any more difficult (it just does the same thing for "int" that is
> already being done for "short" and "char"), and it does
fix the
> ABI bug we have right now, I'd propose to check it in now (and
> then move to new scheme with everyone else once it is ready).
>
> Would this be OK?  Any other suggestions?
>
> For reference, the patch in question is appended again.
Please factor out the check for whether an integer type needs to be
extended into a separate method.

Please make sure there's a testcase on the LLVM side to make sure we
actually handle the attributes correctly.

Otherwise, looks fine.

-Eli