[Patch Darwin 2/2] fix PPC64 ABI

[Patch Darwin 2/2] fix PPC64 ABI

[IainS]

Hi,

We need to deal with the fact that the darwin ppc64 ABI is defined by an
earlier version of gcc, with the property that it always applied  
alignment
adjustments to the va-args (even for zero-sized types).  The cheapest  
way
to deal with this is to replicate the effect of the part of  
std_gimplify_va_arg_expr ()
that carries out the align adjust, for the case  of relevance.

I've made it such that there is no impact at all for non-darwin.
The impact for darwin is restricted to the single m64 ABI case.

OK for trunk and 4.5 when it re-opens?
Iain

gcc/

	PR target/29090
	* config/rs6000/rs6000.c (rs6000_gimplify_va_arg): Special-case the
	Darwin64 ABI, for zero-sized objects.


Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 162457)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -8931,6 +8995,51 @@ rs6000_gimplify_va_arg (tree valist, tree type,  
gi
        return build_va_arg_indirect_ref (t);
      }

+#if TARGET_MACHO
+  /* We need to deal with the fact that the darwin ppc64 ABI is  
defined by an
+     earlier version of gcc, with the property that it always applied  
alignment
+     adjustments to the va-args (even for zero-sized types).  The  
cheapest way
+     to deal with this is to replicate the effect of the part of
+     std_gimplify_va_arg_expr that carries out the align adjust, for  
the case
+     of relevance.
+     We don't need to check for pass-by-reference because of the test  
above.
+     We can return a simplifed answer, since we know there's no  
offset to add.  */
+
+  if (rs6000_darwin64_abi
+      && integer_zerop (TYPE_SIZE (type)))
+    {
+      unsigned HOST_WIDE_INT align, boundary;
+      tree valist_tmp = get_initialized_tmp_var (valist, pre_p, NULL);
+      align = PARM_BOUNDARY / BITS_PER_UNIT;
+      boundary = FUNCTION_ARG_BOUNDARY (TYPE_MODE (type), type);
+      if (boundary > MAX_SUPPORTED_STACK_ALIGNMENT)
+	boundary = MAX_SUPPORTED_STACK_ALIGNMENT;
+      boundary /= BITS_PER_UNIT;
+      if (boundary > align)
+	{
+	  tree t ;
+	  /* This updates arg ptr by the amount that would be necessary
+	     to align the zero-sized (but not zero-alignment) item.  */
+	  t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp,
+		  fold_build2 (POINTER_PLUS_EXPR,
+			       TREE_TYPE (valist),
+			       valist_tmp, size_int (boundary - 1)));
+	  gimplify_and_add (t, pre_p);
+
+	  t = fold_convert (sizetype, valist_tmp);
+	  t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp,
+		  fold_convert (TREE_TYPE (valist),
+				fold_build2 (BIT_AND_EXPR, sizetype, t,
+					     size_int (-boundary))));
+	  t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist, t);
+	  gimplify_and_add (t, pre_p);
+	}
+      /* Since it is zero-sized there's no increment for the item  
itself. */
+      valist_tmp = fold_convert (build_pointer_type (type),  
valist_tmp);
+      return build_va_arg_indirect_ref (valist_tmp);
+    }
+#endif
+
    if (DEFAULT_ABI != ABI_V4)
      {
        if (targetm.calls.split_complex_arg && TREE_CODE (type) ==  
COMPLEX_TYPE)

Re: [Patch Darwin 2/2] fix PPC64 ABI

[Mike Stump]

On Jul 24, 2010, at 1:36 AM, IainS wrote:
> We need to deal with the fact that the darwin ppc64 ABI is defined by an
> earlier version of gcc, with the property that it always applied alignment
> adjustments to the va-args (even for zero-sized types).

> OK for trunk and 4.5 when it re-opens?

I'm fine with this, David, any objections?


Also, as memory serves there is one more fix from last year that fixes the abi so that it is self consistent from last year.  I think it fixes a failing struct abi testcase as I recall.

Re: [Patch Darwin 2/2] fix PPC64 ABI

[IainS]

On 24 Jul 2010, at 10:45, Mike Stump wrote:

> On Jul 24, 2010, at 1:36 AM, IainS wrote:
>> We need to deal with the fact that the darwin ppc64 ABI is defined  
>> by an
>> earlier version of gcc, with the property that it always applied  
>> alignment
>> adjustments to the va-args (even for zero-sized types).
>
>> OK for trunk and 4.5 when it re-opens?
>
> I'm fine with this, David, any objections?
>
>
> Also, as memory serves there is one more fix from last year that  
> fixes the abi so that it is self consistent from last year.  I think  
> it fixes a failing struct abi testcase as I recall.

hm.
  All the test-cases from the PRs and gxx.dg/compat and gxx.dg/struct- 
layout-1
now pass ...  (less breakage to struct-by-value-1.c caused last month,  
which I'm trying to track down).

The "RS6000_STRUCT_HACK" no longer seems necessary (but I might be  
mistaken) ..

can you identify the PR, or Radar?
cheers,
Iain

Re: [Patch Darwin 2/2] fix PPC64 ABI

[Nathan Froyd]

On Sat, Jul 24, 2010 at 09:36:14AM +0100, IainS wrote:
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c	(revision 162457)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -8931,6 +8995,51 @@ rs6000_gimplify_va_arg (tree valist, tree type,  
> gi
>        return build_va_arg_indirect_ref (t);
>      }
>
> +#if TARGET_MACHO

Could we avoid introducing more #if TARGET_MACHO where it's not
necessary?  It should be sufficient to say something like:

  if (TARGET_MACHO
      && rs6000_darwin_abi
      && integer_zerop (TYPE_SIZE (type)))
    ...

and the compiler will DTRT and fold out that code for non-Darwin
targets.  (This comment applies equally to your other darwin64 patch.)

-Nathan

Re: [Patch Darwin 2/2] fix PPC64 ABI

[IainS]

On 24 Jul 2010, at 11:13, Nathan Froyd wrote:

> On Sat, Jul 24, 2010 at 09:36:14AM +0100, IainS wrote:
>> Index: gcc/config/rs6000/rs6000.c
>> ===================================================================
>> --- gcc/config/rs6000/rs6000.c	(revision 162457)
>> +++ gcc/config/rs6000/rs6000.c	(working copy)
>> @@ -8931,6 +8995,51 @@ rs6000_gimplify_va_arg (tree valist, tree  
>> type,
>> gi
>>       return build_va_arg_indirect_ref (t);
>>     }
>>
>> +#if TARGET_MACHO
>
> Could we avoid introducing more #if TARGET_MACHO where it's not
> necessary?  It should be sufficient to say something like:
>
>  if (TARGET_MACHO
>      && rs6000_darwin_abi
>      && integer_zerop (TYPE_SIZE (type)))
>    ...
>
> and the compiler will DTRT and fold out that code for non-Darwin
> targets.  (This comment applies equally to your other darwin64 patch.)

sure, if that's the preference, no problem.
I guess I was thinking that it would be better to eliminate the code   
at pre-processing
cheers,
Iain

Re: [Patch Darwin 2/2] fix PPC64 ABI

[IainS]

Mike, David,

On 24 Jul 2010, at 10:45, Mike Stump wrote:
> Also, as memory serves there is one more fix from last year that  
> fixes the abi so that it is self consistent from last year.  I think  
> it fixes a failing struct abi testcase as I recall.

OK, I found it (the struct hack is needed after all, in fact).

FWIW, The fail of  gcc.target/powerpc/ppc64-abi-1.c is misleading - I  
think that is a situation where the test-case needs some tweaking.

The darwin64-abi.c check from 4.2.1 (or LLVM) is the right thing to  
use...
...  but  I have no idea what license it is covered by and, therefore,  
whether we can import it... so for now I'm using it outside the gcc  
tree.

====

There will follow a part #3 (I can hack a fix... now I need to find  
the Right Way).

Part 3 will apply on top of parts 1 & 2 .. so those patches stand.

cheers
Iain

Re: [Patch Darwin 2/2] fix PPC64 ABI

[David Edelsohn]

On Sat, Jul 24, 2010 at 11:06 AM, IainS
<developer@sandoe-acoustics.co.uk> wrote:
> Mike, David,
>
> On 24 Jul 2010, at 10:45, Mike Stump wrote:
>>
>> Also, as memory serves there is one more fix from last year that fixes the
>> abi so that it is self consistent from last year.  I think it fixes a
>> failing struct abi testcase as I recall.
>
> OK, I found it (the struct hack is needed after all, in fact).
>
> FWIW, The fail of  gcc.target/powerpc/ppc64-abi-1.c is misleading - I think
> that is a situation where the test-case needs some tweaking.
>
> The darwin64-abi.c check from 4.2.1 (or LLVM) is the right thing to use...
> ...  but  I have no idea what license it is covered by and, therefore,
> whether we can import it... so for now I'm using it outside the gcc tree.
>
> ====
>
> There will follow a part #3 (I can hack a fix... now I need to find the
> Right Way).
>
> Part 3 will apply on top of parts 1 & 2 .. so those patches stand.

Darwin-specific patches approved by Mike are fine with me.

Thanks, David

Re: [Patch Darwin 2/2] fix PPC64 ABI

[Nathan Froyd]

On Sat, Jul 24, 2010 at 12:43:52PM +0100, IainS wrote:
> On 24 Jul 2010, at 11:13, Nathan Froyd wrote:
>> Could we avoid introducing more #if TARGET_MACHO where it's not
>> necessary?  It should be sufficient to say something like:
>>
>>  if (TARGET_MACHO
>>      && rs6000_darwin_abi
>>      && integer_zerop (TYPE_SIZE (type)))
>>    ...
>>
>> and the compiler will DTRT and fold out that code for non-Darwin
>> targets.  (This comment applies equally to your other darwin64 patch.)
>
> sure, if that's the preference, no problem.
> I guess I was thinking that it would be better to eliminate the code  at 
> pre-processing

It does slightly speed things up (I doubt that you'd notice the speedup,
really).  The rationale for using:

  if (TARGET_MACHO)

versus

  #if TARGET_MACHO

is that with the former, the compiler will validate that the code
syntax-checks and type-checks even when compiling for non-Darwin
targets.  This extra checking makes it slightly harder to inadvertently
break things.

-Nathan

Re: [Patch Darwin 2/2] fix PPC64 ABI

[IainS]

On 26 Jul 2010, at 14:33, Nathan Froyd wrote:

> On Sat, Jul 24, 2010 at 12:43:52PM +0100, IainS wrote:
>> On 24 Jul 2010, at 11:13, Nathan Froyd wrote:
>>> Could we avoid introducing more #if TARGET_MACHO where it's not
>>> necessary?  It should be sufficient to say something like:
>>>
>>> if (TARGET_MACHO
>>>     && rs6000_darwin_abi
>>>     && integer_zerop (TYPE_SIZE (type)))
>>>   ...
>>>
>>> and the compiler will DTRT and fold out that code for non-Darwin
>>> targets.  (This comment applies equally to your other darwin64  
>>> patch.)
>>
>> sure, if that's the preference, no problem.
>> I guess I was thinking that it would be better to eliminate the  
>> code  at
>> pre-processing
>
> It does slightly speed things up (I doubt that you'd notice the  
> speedup,
> really).  The rationale for using:
>
>  if (TARGET_MACHO)
>
> versus
>
>  #if TARGET_MACHO
>
> is that with the former, the compiler will validate that the code
> syntax-checks and type-checks even when compiling for non-Darwin
> targets.  This extra checking makes it slightly harder to  
> inadvertently
> break things.

OK. I'll do a second pass through (after sorting out part #3 of the  
ABI fixes).

Apropos extending this to its logical conclusion:

I suspect that the only viable 'end-game' at present is to macro-ize  
the machopic* calls,
exposing the whole of the machopic workings would probably carry too  
much weight.

(that comment would apply equally to i386.c, which also uses that  
machopic common code).

Iain

Re: [Patch Darwin 2/2] fix PPC64 ABI

[IainS]

On 24 Jul 2010, at 16:10, David Edelsohn wrote:

> On Sat, Jul 24, 2010 at 11:06 AM, IainS
> <developer@sandoe-acoustics.co.uk> wrote:
>> Mike, David,
>>
>> On 24 Jul 2010, at 10:45, Mike Stump wrote:
>>>
>>> Also, as memory serves there is one more fix from last year that  
>>> fixes the
>>> abi so that it is self consistent from last year.  I think it  
>>> fixes a
>>> failing struct abi testcase as I recall.
>>
>> OK, I found it (the struct hack is needed after all, in fact).
>>
>> FWIW, The fail of  gcc.target/powerpc/ppc64-abi-1.c is misleading -  
>> I think
>> that is a situation where the test-case needs some tweaking.
>>
>> The darwin64-abi.c check from 4.2.1 (or LLVM) is the right thing to  
>> use...
>> ...  but  I have no idea what license it is covered by and,  
>> therefore,
>> whether we can import it... so for now I'm using it outside the gcc  
>> tree.
>>
>> ====
>>
>> There will follow a part #3 (I can hack a fix... now I need to find  
>> the
>> Right Way).
>>
>> Part 3 will apply on top of parts 1 & 2 .. so those patches stand.
>
> Darwin-specific patches approved by Mike are fine with me

part 1 is r162567
part 2 (as amended with Nathan's suggestion, copied below) is r162568  
(changelog for #2 is 162569)

cheers,
Iain


amended part 2/2

   /* We need to deal with the fact that the darwin ppc64 ABI is  
defined by an
      earlier version of gcc, with the property that it always applied  
alignment
      adjustments to the va-args (even for zero-sized types).  The  
cheapest way
      to deal with this is to replicate the effect of the part of
      std_gimplify_va_arg_expr that carries out the align adjust, for  
the case
      of relevance.
      We don't need to check for pass-by-reference because of the test  
above.
      We can return a simplifed answer, since we know there's no  
offset to add.  */

   if (TARGET_MACHO
       && rs6000_darwin64_abi
       && integer_zerop (TYPE_SIZE (type)))
     {
       unsigned HOST_WIDE_INT align, boundary;
       tree valist_tmp = get_initialized_tmp_var (valist, pre_p, NULL);
       align = PARM_BOUNDARY / BITS_PER_UNIT;
       boundary = FUNCTION_ARG_BOUNDARY (TYPE_MODE (type), type);
       if (boundary > MAX_SUPPORTED_STACK_ALIGNMENT)
	boundary = MAX_SUPPORTED_STACK_ALIGNMENT;
       boundary /= BITS_PER_UNIT;
       if (boundary > align)
	{
	  tree t ;
	  /* This updates arg ptr by the amount that would be necessary
	     to align the zero-sized (but not zero-alignment) item.  */
	  t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp,
		  fold_build2 (POINTER_PLUS_EXPR,
			       TREE_TYPE (valist),
			       valist_tmp, size_int (boundary - 1)));
	  gimplify_and_add (t, pre_p);

	  t = fold_convert (sizetype, valist_tmp);
	  t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp,
		  fold_convert (TREE_TYPE (valist),
				fold_build2 (BIT_AND_EXPR, sizetype, t,
					     size_int (-boundary))));
	  t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist, t);
	  gimplify_and_add (t, pre_p);
	}
       /* Since it is zero-sized there's no increment for the item  
itself. */
       valist_tmp = fold_convert (build_pointer_type (type),  
valist_tmp);
       return build_va_arg_indirect_ref (valist_tmp);
     }