diff mbox

[rs6000] Fix aggregate alignment ABI issue

Message ID 201407091604.s69G4qnj020440@d06av02.portsmouth.uk.ibm.com
State New
Headers show

Commit Message

Ulrich Weigand July 9, 2014, 4:04 p.m. UTC
Hello,

last year, Bill added a patch to address PR 57949 by aligning aggregates
requiring at least 128-bit alignment at a quadword boundary in the
parameter save area:
https://gcc.gnu.org/ml/gcc-patches/2013-08/msg00803.html

Unfortunately, to implement this check, Bill's patch used a pre-existing
piece of code originally used only on Darwin, which uses a "mode == BLKmode"
check to test for aggregate types.

However, GCC may sometimes choose non-BLKmode modes to represent aggregate
types.  One case the single-element float/vector aggregates; that's OK,
because those are handled separately by the ABI anyway.

But there are more cases: many structures get some integer mode simply
because their size happen to be 1, 2, 4, 8, or 16 bytes.  The precise
rules *which* aggregates GCC uses such a mode for are intricate, and
even differ slightly between types created by the C vs. C++ front-ends.

This normally doesn't matter since the mode used to back an aggregate
type only matters for internal code generation (basically, whether GCC
may use a register to hold a local variable of that type, or whether
they must all go to memory).

Due to this check in rs6000_function_arg_boundary, however, those GCC
internal details have now leaked into the public ABI.  We have thought
of simply accepting that ABI as the de-facto ABI now and documenting
it, but that turned out to be too fragile; it is hard to precisely
describe the mode selection in a way that it can be reliably implemented
by another (non-GCC based) compiler.

After various off-line discussions, we came to the conclusion that the
best way is to fix the GCC implementation to actually align *all* aggregate
types, not just those backed by BLKmode.  [ The exception remain single-
element (or ELFv2 homogeneous) float/vector aggregates, which are handled
as before: float is doubleword aligned, vector is quadword aligned. ]

This change does break the ABI in certain cases.  However, we hope that
this is acceptable because:

- The change only affects rare cases: passing a struct by value that is
   * not a float/vector special case, and
   * has a size of 1, 2, 4, 8, or 16 bytes, and
   * has an alignment requirement of 16 bytes or more
  [ Not *all* these cases will see change, but all cases that change
  will share these properties.  ]

- This aspect of the ABI already changed recently with Bill's patch,
  and the current version hasn't seen very widespread use yet.

Note that patch below only changes the ABI for the AIX/ELFv1 and ELFv2
cases; the Darwin ABI (which shared the same problem) is left as-is.
It's up to the Darwin maintainers whether they prefer to change as well
or rather keep everything as it has been on Darwin for a long time.

Tested on powerpc64-linux and powerpc64le-linux.

OK for mainline?
[ The patch should then also go into the 4.8 and 4.9 branches for
consistency. ]

Bye,
Ulrich


ChangeLog:

	* config/rs6000/rs6000.c (rs6000_function_arg_boundary): In the AIX
	and ELFv2 ABI, do not use the "mode == BLKmode" check to test for
	aggregate types.  Instead, *all* aggregate types, except for single-
	element or homogeneous float/vector aggregates, are quadword-aligned
	if required by their type alignment.

Comments

David Edelsohn July 9, 2014, 6:25 p.m. UTC | #1
On Wed, Jul 9, 2014 at 12:04 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Hello,
>
> last year, Bill added a patch to address PR 57949 by aligning aggregates
> requiring at least 128-bit alignment at a quadword boundary in the
> parameter save area:
> https://gcc.gnu.org/ml/gcc-patches/2013-08/msg00803.html
>
> Unfortunately, to implement this check, Bill's patch used a pre-existing
> piece of code originally used only on Darwin, which uses a "mode == BLKmode"
> check to test for aggregate types.
>
> However, GCC may sometimes choose non-BLKmode modes to represent aggregate
> types.  One case the single-element float/vector aggregates; that's OK,
> because those are handled separately by the ABI anyway.
>
> But there are more cases: many structures get some integer mode simply
> because their size happen to be 1, 2, 4, 8, or 16 bytes.  The precise
> rules *which* aggregates GCC uses such a mode for are intricate, and
> even differ slightly between types created by the C vs. C++ front-ends.
>
> This normally doesn't matter since the mode used to back an aggregate
> type only matters for internal code generation (basically, whether GCC
> may use a register to hold a local variable of that type, or whether
> they must all go to memory).
>
> Due to this check in rs6000_function_arg_boundary, however, those GCC
> internal details have now leaked into the public ABI.  We have thought
> of simply accepting that ABI as the de-facto ABI now and documenting
> it, but that turned out to be too fragile; it is hard to precisely
> describe the mode selection in a way that it can be reliably implemented
> by another (non-GCC based) compiler.
>
> After various off-line discussions, we came to the conclusion that the
> best way is to fix the GCC implementation to actually align *all* aggregate
> types, not just those backed by BLKmode.  [ The exception remain single-
> element (or ELFv2 homogeneous) float/vector aggregates, which are handled
> as before: float is doubleword aligned, vector is quadword aligned. ]
>
> This change does break the ABI in certain cases.  However, we hope that
> this is acceptable because:
>
> - The change only affects rare cases: passing a struct by value that is
>    * not a float/vector special case, and
>    * has a size of 1, 2, 4, 8, or 16 bytes, and
>    * has an alignment requirement of 16 bytes or more
>   [ Not *all* these cases will see change, but all cases that change
>   will share these properties.  ]
>
> - This aspect of the ABI already changed recently with Bill's patch,
>   and the current version hasn't seen very widespread use yet.
>
> Note that patch below only changes the ABI for the AIX/ELFv1 and ELFv2
> cases; the Darwin ABI (which shared the same problem) is left as-is.
> It's up to the Darwin maintainers whether they prefer to change as well
> or rather keep everything as it has been on Darwin for a long time.
>
> Tested on powerpc64-linux and powerpc64le-linux.
>
> OK for mainline?
> [ The patch should then also go into the 4.8 and 4.9 branches for
> consistency. ]
>
> Bye,
> Ulrich
>
>
> ChangeLog:
>
>         * config/rs6000/rs6000.c (rs6000_function_arg_boundary): In the AIX
>         and ELFv2 ABI, do not use the "mode == BLKmode" check to test for
>         aggregate types.  Instead, *all* aggregate types, except for single-
>         element or homogeneous float/vector aggregates, are quadword-aligned
>         if required by their type alignment.

Okay.

I copied the Darwin maintainers and active testers so that they are
explicitly aware of the ABI issue. They can decide if they want to fix
the ABI alignment issue on Darwin.

Thanks, David
Eric Christopher July 9, 2014, 6:29 p.m. UTC | #2
On Wed, Jul 9, 2014 at 11:25 AM, David Edelsohn <dje.gcc@gmail.com> wrote:
> On Wed, Jul 9, 2014 at 12:04 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> Hello,
>>
>> last year, Bill added a patch to address PR 57949 by aligning aggregates
>> requiring at least 128-bit alignment at a quadword boundary in the
>> parameter save area:
>> https://gcc.gnu.org/ml/gcc-patches/2013-08/msg00803.html
>>
>> Unfortunately, to implement this check, Bill's patch used a pre-existing
>> piece of code originally used only on Darwin, which uses a "mode == BLKmode"
>> check to test for aggregate types.
>>
>> However, GCC may sometimes choose non-BLKmode modes to represent aggregate
>> types.  One case the single-element float/vector aggregates; that's OK,
>> because those are handled separately by the ABI anyway.
>>
>> But there are more cases: many structures get some integer mode simply
>> because their size happen to be 1, 2, 4, 8, or 16 bytes.  The precise
>> rules *which* aggregates GCC uses such a mode for are intricate, and
>> even differ slightly between types created by the C vs. C++ front-ends.
>>
>> This normally doesn't matter since the mode used to back an aggregate
>> type only matters for internal code generation (basically, whether GCC
>> may use a register to hold a local variable of that type, or whether
>> they must all go to memory).
>>
>> Due to this check in rs6000_function_arg_boundary, however, those GCC
>> internal details have now leaked into the public ABI.  We have thought
>> of simply accepting that ABI as the de-facto ABI now and documenting
>> it, but that turned out to be too fragile; it is hard to precisely
>> describe the mode selection in a way that it can be reliably implemented
>> by another (non-GCC based) compiler.
>>
>> After various off-line discussions, we came to the conclusion that the
>> best way is to fix the GCC implementation to actually align *all* aggregate
>> types, not just those backed by BLKmode.  [ The exception remain single-
>> element (or ELFv2 homogeneous) float/vector aggregates, which are handled
>> as before: float is doubleword aligned, vector is quadword aligned. ]
>>
>> This change does break the ABI in certain cases.  However, we hope that
>> this is acceptable because:
>>
>> - The change only affects rare cases: passing a struct by value that is
>>    * not a float/vector special case, and
>>    * has a size of 1, 2, 4, 8, or 16 bytes, and
>>    * has an alignment requirement of 16 bytes or more
>>   [ Not *all* these cases will see change, but all cases that change
>>   will share these properties.  ]
>>
>> - This aspect of the ABI already changed recently with Bill's patch,
>>   and the current version hasn't seen very widespread use yet.
>>
>> Note that patch below only changes the ABI for the AIX/ELFv1 and ELFv2
>> cases; the Darwin ABI (which shared the same problem) is left as-is.
>> It's up to the Darwin maintainers whether they prefer to change as well
>> or rather keep everything as it has been on Darwin for a long time.
>>
>> Tested on powerpc64-linux and powerpc64le-linux.
>>
>> OK for mainline?
>> [ The patch should then also go into the 4.8 and 4.9 branches for
>> consistency. ]
>>
>> Bye,
>> Ulrich
>>
>>
>> ChangeLog:
>>
>>         * config/rs6000/rs6000.c (rs6000_function_arg_boundary): In the AIX
>>         and ELFv2 ABI, do not use the "mode == BLKmode" check to test for
>>         aggregate types.  Instead, *all* aggregate types, except for single-
>>         element or homogeneous float/vector aggregates, are quadword-aligned
>>         if required by their type alignment.
>
> Okay.
>
> I copied the Darwin maintainers and active testers so that they are
> explicitly aware of the ABI issue. They can decide if they want to fix
> the ABI alignment issue on Darwin.
>

Thanks David, In general I'd personally prefer to fix the ABI issues,
but PPC darwin is beyond EoL by the original company and I don't have
any hardware for it myself - in which case I'll leave it up to our
more active testers or someone with hardware. (Mike? Have old ppc
hardware sitting around?)

-eric
Mike Stump July 9, 2014, 7:01 p.m. UTC | #3
On Jul 9, 2014, at 11:29 AM, Eric Christopher <echristo@gmail.com> wrote:
>>> - The change only affects rare cases: passing a struct by value that is
>>>   * not a float/vector special case, and
>>>   * has a size of 1, 2, 4, 8, or 16 bytes, and
>>>   * has an alignment requirement of 16 bytes or more

>> I copied the Darwin maintainers and active testers so that they are
>> explicitly aware of the ABI issue. They can decide if they want to fix
>> the ABI alignment issue on Darwin.

> Thanks David, In general I'd personally prefer to fix the ABI issues,
> but PPC darwin is beyond EoL by the original company and I don't have
> any hardware for it myself - in which case I'll leave it up to our
> more active testers or someone with hardware. (Mike? Have old ppc
> hardware sitting around?)

Well, I think from a safety perspective I think it is ok, as the system header files and most libraries I would expect not to align to 16 bytes or more.  The problem with this line of thinking, it only takes 1 structure, in one place that is used often (stat, X11) to completely kill things.  Normally I would do a world build with the change in it, and a fink build with it in it, and if no changes occur, the change is reasonably safe.  I don’t have the machines/system to do either unfortunately.  I had been testing ppc with emulation, so I can’t do much of that anymore either.

So, I think I will punt to the users that still have G5 darwin, they know who they are…  I’d say, lets leave it as is, and if they think it is a good idea as well (that would make it 3 of 3) and can do a test suite run at least with the change, then I’d approve it.
Eric Christopher July 9, 2014, 11:30 p.m. UTC | #4
On Wed, Jul 9, 2014 at 12:01 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Jul 9, 2014, at 11:29 AM, Eric Christopher <echristo@gmail.com> wrote:
>>>> - The change only affects rare cases: passing a struct by value that is
>>>>   * not a float/vector special case, and
>>>>   * has a size of 1, 2, 4, 8, or 16 bytes, and
>>>>   * has an alignment requirement of 16 bytes or more
>
>>> I copied the Darwin maintainers and active testers so that they are
>>> explicitly aware of the ABI issue. They can decide if they want to fix
>>> the ABI alignment issue on Darwin.
>
>> Thanks David, In general I'd personally prefer to fix the ABI issues,
>> but PPC darwin is beyond EoL by the original company and I don't have
>> any hardware for it myself - in which case I'll leave it up to our
>> more active testers or someone with hardware. (Mike? Have old ppc
>> hardware sitting around?)
>
> Well, I think from a safety perspective I think it is ok, as the system header files and most libraries I would expect not to align to 16 bytes or more.  The problem with this line of thinking, it only takes 1 structure, in one place that is used often (stat, X11) to completely kill things.  Normally I would do a world build with the change in it, and a fink build with it in it, and if no changes occur, the change is reasonably safe.  I don’t have the machines/system to do either unfortunately.  I had been testing ppc with emulation, so I can’t do much of that anymore either.
>
> So, I think I will punt to the users that still have G5 darwin, they know who they are…  I’d say, lets leave it as is, and if they think it is a good idea as well (that would make it 3 of 3) and can do a test suite run at least with the change, then I’d approve it.

Completely agreed.

-eric
diff mbox

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 212147)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -9182,9 +9182,20 @@ 
 	   || (type && TREE_CODE (type) == VECTOR_TYPE
 	       && int_size_in_bytes (type) >= 16))
     return 128;
-  else if (((TARGET_MACHO && rs6000_darwin64_abi)
-	    || DEFAULT_ABI == ABI_ELFv2
-            || (DEFAULT_ABI == ABI_AIX && !rs6000_compat_align_parm))
+  /* Aggregate types that need > 8 byte alignment are quadword-aligned
+     in the parameter area in the ELFv2 ABI, and in the AIX ABI unless
+     -mcompat-align-parm is used.  This does not apply to single-element
+     (or homogeneous) float/vector aggregrates.  We already checked for
+     vector above; we still need to check for float here.  */
+  else if (((DEFAULT_ABI == ABI_AIX && !rs6000_compat_align_parm)
+            || DEFAULT_ABI == ABI_ELFv2)
+	   && type && AGGREGATE_TYPE_P (type) && TYPE_ALIGN (type) > 64
+ 	   && !SCALAR_FLOAT_MODE_P (elt_mode))
+    return 128;
+  /* Similar for the Darwin64 ABI.  Note that for historical reasons we
+     implement the "aggregate type" check as a BLKmode check here; this
+     means certain aggregate types are in fact not aligned.  */
+  else if (TARGET_MACHO && rs6000_darwin64_abi
  	   && mode == BLKmode
 	   && type && TYPE_ALIGN (type) > 64)
     return 128;