Re: Follow-up-fix 2 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"
On 5/12/19 10:01 PM, Hans-Peter Nilsson wrote:
>> Date: Tue, 30 Apr 2019 11:37:17 -0600
>> From: Jeff Law
>
>> On 2/10/19 6:09 PM, Hans-Peter Nilsson wrote:
>>> Here's the follow-up, getting rid of the observed
>>> alignment-padding in execute/930126-1.c: the x parameter in f
>>> spuriously being runtime-aligned to BITS_PER_WORD. I separated
>>> this change because this is an older issue, a change introduced
>>> in r94104 where BITS_PER_WORD was chosen perhaps because we
>>> expect register-sized writes into this area. Here, we instead
>>> align to a minimum of PREFERRED_STACK_BOUNDARY, but of course
>>> gated on ! STRICT_ALIGNMENT.
>>>
>>> Regtested cris-elf and x86_64-pc-linux-gnu.
>>>
>>> Ok to commit?
>>>
>>> gcc:
>>> * function.c (assign_parm_setup_block): If not STRICT_ALIGNMENT,
>>> instead of always BITS_PER_WORD, align the stacked
>>> parameter to a minimum PREFERRED_STACK_BOUNDARY.
>> Interestingly enough in the thread from 2005 Richard S suggests that he
>> could have made increasing the alignment conditional on STRICT_ALIGNMENT
>> but thought that with the size already being rounded up it wasn't worth
>> it and that we could take advantage of the increased alignment elsewhere.
>>
>> I wonder if we could just go back to that idea. Leave the alignment as
>> DECL_ALIGN for !STRICT_ALIGNMENT targets and bump it up for
>> STRICT_ALIGNMENT targets?
>>
>> So something like
>>
>> align = STRICT_ALIGNMENT ? MAX (DECL_ALIGN (parm), BITS_PER_WORD) :
>> DECL_ALIGN (parm)
>
> That'd work for me, I think. Testing in progress. Thanks.
> Almost obvious, but: is this ok; what you meant?
>
> gcc:
> * function.c (assign_parm_setup_block): Raise alignment of
> stacked parameter only for STRICT_ALIGNMENT targets.
Yea. I threw it into my tester overnight and it hasn't caused any
problems. I think this is good for the trunk.
>
> Index: gcc/function.c
> ===
> --- gcc/function.c(revision 27)
> +++ gcc/function.c(working copy)
> @@ -2912,7 +2912,11 @@ assign_parm_setup_block (struct assign_p
>size_stored = CEIL_ROUND (size, UNITS_PER_WORD);
>if (stack_parm == 0)
> {
> - SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), BITS_PER_WORD));
> + HOST_WIDE_INT parm_align
> + = (STRICT_ALIGNMENT
> +? MAX (DECL_ALIGN (parm), BITS_PER_WORD) : DECL_ALIGN (parm));
> +
> + SET_DECL_ALIGN (parm, parm_align);
>if (DECL_ALIGN (parm) > MAX_SUPPORTED_STACK_ALIGNMENT)
> {
> rtx allocsize = gen_int_mode (size_stored, Pmode);
>
> PS. A preferable solution would IMHO involve hookifying
> parameter padding and alignment as separate entities. Maybe
> later, perhaps even after PR84877 is fixed, or that bug risk
> dying from perceived misprioritized attention. (Amazing how
> easy it is to "overalign" here, and hard to align "there"!)
Yea, but at this point I suspect folks are a bit reluctant to dig into
this code :(
jeff
Re: Follow-up-fix 2 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"
> Date: Tue, 30 Apr 2019 11:37:17 -0600
> From: Jeff Law
> On 2/10/19 6:09 PM, Hans-Peter Nilsson wrote:
> > Here's the follow-up, getting rid of the observed
> > alignment-padding in execute/930126-1.c: the x parameter in f
> > spuriously being runtime-aligned to BITS_PER_WORD. I separated
> > this change because this is an older issue, a change introduced
> > in r94104 where BITS_PER_WORD was chosen perhaps because we
> > expect register-sized writes into this area. Here, we instead
> > align to a minimum of PREFERRED_STACK_BOUNDARY, but of course
> > gated on ! STRICT_ALIGNMENT.
> >
> > Regtested cris-elf and x86_64-pc-linux-gnu.
> >
> > Ok to commit?
> >
> > gcc:
> > * function.c (assign_parm_setup_block): If not STRICT_ALIGNMENT,
> > instead of always BITS_PER_WORD, align the stacked
> > parameter to a minimum PREFERRED_STACK_BOUNDARY.
> Interestingly enough in the thread from 2005 Richard S suggests that he
> could have made increasing the alignment conditional on STRICT_ALIGNMENT
> but thought that with the size already being rounded up it wasn't worth
> it and that we could take advantage of the increased alignment elsewhere.
>
> I wonder if we could just go back to that idea. Leave the alignment as
> DECL_ALIGN for !STRICT_ALIGNMENT targets and bump it up for
> STRICT_ALIGNMENT targets?
>
> So something like
>
> align = STRICT_ALIGNMENT ? MAX (DECL_ALIGN (parm), BITS_PER_WORD) :
> DECL_ALIGN (parm)
That'd work for me, I think. Testing in progress. Thanks.
Almost obvious, but: is this ok; what you meant?
gcc:
* function.c (assign_parm_setup_block): Raise alignment of
stacked parameter only for STRICT_ALIGNMENT targets.
Index: gcc/function.c
===
--- gcc/function.c (revision 27)
+++ gcc/function.c (working copy)
@@ -2912,7 +2912,11 @@ assign_parm_setup_block (struct assign_p
size_stored = CEIL_ROUND (size, UNITS_PER_WORD);
if (stack_parm == 0)
{
- SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), BITS_PER_WORD));
+ HOST_WIDE_INT parm_align
+ = (STRICT_ALIGNMENT
+ ? MAX (DECL_ALIGN (parm), BITS_PER_WORD) : DECL_ALIGN (parm));
+
+ SET_DECL_ALIGN (parm, parm_align);
if (DECL_ALIGN (parm) > MAX_SUPPORTED_STACK_ALIGNMENT)
{
rtx allocsize = gen_int_mode (size_stored, Pmode);
PS. A preferable solution would IMHO involve hookifying
parameter padding and alignment as separate entities. Maybe
later, perhaps even after PR84877 is fixed, or that bug risk
dying from perceived misprioritized attention. (Amazing how
easy it is to "overalign" here, and hard to align "there"!)
brgds, H-P
Re: Follow-up-fix 2 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"
On 2/10/19 6:09 PM, Hans-Peter Nilsson wrote: > Here's the follow-up, getting rid of the observed > alignment-padding in execute/930126-1.c: the x parameter in f > spuriously being runtime-aligned to BITS_PER_WORD. I separated > this change because this is an older issue, a change introduced > in r94104 where BITS_PER_WORD was chosen perhaps because we > expect register-sized writes into this area. Here, we instead > align to a minimum of PREFERRED_STACK_BOUNDARY, but of course > gated on ! STRICT_ALIGNMENT. > > Regtested cris-elf and x86_64-pc-linux-gnu. > > Ok to commit? > > gcc: > * function.c (assign_parm_setup_block): If not STRICT_ALIGNMENT, > instead of always BITS_PER_WORD, align the stacked > parameter to a minimum PREFERRED_STACK_BOUNDARY. Interestingly enough in the thread from 2005 Richard S suggests that he could have made increasing the alignment conditional on STRICT_ALIGNMENT but thought that with the size already being rounded up it wasn't worth it and that we could take advantage of the increased alignment elsewhere. I wonder if we could just go back to that idea. Leave the alignment as DECL_ALIGN for !STRICT_ALIGNMENT targets and bump it up for STRICT_ALIGNMENT targets? So something like align = STRICT_ALIGNMENT ? MAX (DECL_ALIGN (parm), BITS_PER_WORD) : DECL_ALIGN (parm) Jeff
Re: Follow-up-fix 2 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"
> Date: Mon, 11 Feb 2019 10:22:12 +0100 > From: Jakub Jelinek > Is PREFERRED_STACK_BOUNDARY what we want here? Shouldn't that be > STACK_BOUNDARY, or PARM_BOUNDARY? Though, PARM_BOUNDARY on cris is 32... Hm. I wish there was a better distinction both in the code and in peoples minds between boundary (or whatever you'd call the multiple to which the size of something must be rounded) and (memory/stack-)alignment. Alas, there are lots of mixups in usage and the documentation even calls this alignment. I'm not even sure it was *size* boundary when I set PARM_BOUNDARY to 32 in the first place! I may have to change that to reflect the current sources. :( Still, this isn't strictly a parameter (to pass or to be expected incoming according to an ABI), but a local copy of what was incoming in registers, so picking the preferred stack boundary as per PREFERRED_STACK_BOUNDARY seems natural. brgds, H-P
Re: Follow-up-fix 2 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"
On Mon, Feb 11, 2019 at 10:00:03AM +0100, Hans-Peter Nilsson wrote:
> > Date: Mon, 11 Feb 2019 07:38:14 +0100
> > From: Richard Biener
>
> > >+ HOST_WIDE_INT min_parm_align
> > >+ = STRICT_ALIGNMENT ? BITS_PER_WORD : PREFERRED_STACK_BOUNDARY;
> >
> > Shouldn't it be MIN (...) of BOTH?
>
> That *does* seem logical... Take 2 as follows, in testing as
> before.
>
> Ok to commit, if testing passes?
Is PREFERRED_STACK_BOUNDARY what we want here? Shouldn't that be
STACK_BOUNDARY, or PARM_BOUNDARY? Though, PARM_BOUNDARY on cris is 32...
> gcc:
> * function.c (assign_parm_setup_block): Align the
> parameter to a minimum of PREFERRED_STACK_BOUNDARY and
> BITS_PER_WORD instead of always BITS_PER_WORD.
>
> --- gcc/function.c.orig2 Sat Feb 9 00:53:17 2019
> +++ gcc/function.cMon Feb 11 09:37:28 2019
> @@ -2912,7 +2912,10 @@ assign_parm_setup_block (struct assign_p
>size_stored = CEIL_ROUND (size, UNITS_PER_WORD);
>if (stack_parm == 0)
> {
> - SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), BITS_PER_WORD));
> + HOST_WIDE_INT min_parm_align
> + = MIN (BITS_PER_WORD, PREFERRED_STACK_BOUNDARY);
> +
> + SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), min_parm_align));
>if (DECL_ALIGN (parm) > MAX_SUPPORTED_STACK_ALIGNMENT)
> {
> rtx allocsize = gen_int_mode (size_stored, Pmode);
Jakub
Re: Follow-up-fix 2 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"
> Date: Mon, 11 Feb 2019 07:38:14 +0100
> From: Richard Biener
> >+ HOST_WIDE_INT min_parm_align
> >+= STRICT_ALIGNMENT ? BITS_PER_WORD : PREFERRED_STACK_BOUNDARY;
>
> Shouldn't it be MIN (...) of BOTH?
That *does* seem logical... Take 2 as follows, in testing as
before.
Ok to commit, if testing passes?
gcc:
* function.c (assign_parm_setup_block): Align the
parameter to a minimum of PREFERRED_STACK_BOUNDARY and
BITS_PER_WORD instead of always BITS_PER_WORD.
--- gcc/function.c.orig2Sat Feb 9 00:53:17 2019
+++ gcc/function.c Mon Feb 11 09:37:28 2019
@@ -2912,7 +2912,10 @@ assign_parm_setup_block (struct assign_p
size_stored = CEIL_ROUND (size, UNITS_PER_WORD);
if (stack_parm == 0)
{
- SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), BITS_PER_WORD));
+ HOST_WIDE_INT min_parm_align
+ = MIN (BITS_PER_WORD, PREFERRED_STACK_BOUNDARY);
+
+ SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), min_parm_align));
if (DECL_ALIGN (parm) > MAX_SUPPORTED_STACK_ALIGNMENT)
{
rtx allocsize = gen_int_mode (size_stored, Pmode);
brgds, H-P
Re: Follow-up-fix 2 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"
On February 11, 2019 2:09:30 AM GMT+01:00, Hans-Peter Nilsson
wrote:
>Here's the follow-up, getting rid of the observed
>alignment-padding in execute/930126-1.c: the x parameter in f
>spuriously being runtime-aligned to BITS_PER_WORD. I separated
>this change because this is an older issue, a change introduced
>in r94104 where BITS_PER_WORD was chosen perhaps because we
>expect register-sized writes into this area. Here, we instead
>align to a minimum of PREFERRED_STACK_BOUNDARY, but of course
>gated on ! STRICT_ALIGNMENT.
>
>Regtested cris-elf and x86_64-pc-linux-gnu.
>
>Ok to commit?
>
>gcc:
> * function.c (assign_parm_setup_block): If not STRICT_ALIGNMENT,
> instead of always BITS_PER_WORD, align the stacked
> parameter to a minimum PREFERRED_STACK_BOUNDARY.
>
>--- function.c.orig2 Sat Feb 9 00:53:17 2019
>+++ function.c Sat Feb 9 23:21:35 2019
>@@ -2912,7 +2912,10 @@ assign_parm_setup_block (struct assign_p
> size_stored = CEIL_ROUND (size, UNITS_PER_WORD);
> if (stack_parm == 0)
> {
>- SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), BITS_PER_WORD));
>+ HOST_WIDE_INT min_parm_align
>+ = STRICT_ALIGNMENT ? BITS_PER_WORD : PREFERRED_STACK_BOUNDARY;
Shouldn't it be MIN (...) of BOTH?
>+
>+ SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), min_parm_align));
> if (DECL_ALIGN (parm) > MAX_SUPPORTED_STACK_ALIGNMENT)
> {
> rtx allocsize = gen_int_mode (size_stored, Pmode);
>
>brgds, H-P
Follow-up-fix 2 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"
Here's the follow-up, getting rid of the observed
alignment-padding in execute/930126-1.c: the x parameter in f
spuriously being runtime-aligned to BITS_PER_WORD. I separated
this change because this is an older issue, a change introduced
in r94104 where BITS_PER_WORD was chosen perhaps because we
expect register-sized writes into this area. Here, we instead
align to a minimum of PREFERRED_STACK_BOUNDARY, but of course
gated on ! STRICT_ALIGNMENT.
Regtested cris-elf and x86_64-pc-linux-gnu.
Ok to commit?
gcc:
* function.c (assign_parm_setup_block): If not STRICT_ALIGNMENT,
instead of always BITS_PER_WORD, align the stacked
parameter to a minimum PREFERRED_STACK_BOUNDARY.
--- function.c.orig2Sat Feb 9 00:53:17 2019
+++ function.c Sat Feb 9 23:21:35 2019
@@ -2912,7 +2912,10 @@ assign_parm_setup_block (struct assign_p
size_stored = CEIL_ROUND (size, UNITS_PER_WORD);
if (stack_parm == 0)
{
- SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), BITS_PER_WORD));
+ HOST_WIDE_INT min_parm_align
+ = STRICT_ALIGNMENT ? BITS_PER_WORD : PREFERRED_STACK_BOUNDARY;
+
+ SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), min_parm_align));
if (DECL_ALIGN (parm) > MAX_SUPPORTED_STACK_ALIGNMENT)
{
rtx allocsize = gen_int_mode (size_stored, Pmode);
brgds, H-P
