Follow-up-fix 2 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"

Message ID 201902110109.x1B19UWh021718@ignucius.se.axis.com
State New
Headers show
Series
  • Follow-up-fix 2 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"
Related show

Commit Message

Hans-Peter Nilsson Feb. 11, 2019, 1:09 a.m.
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.


brgds, H-P

Comments

Richard Biener Feb. 11, 2019, 6:38 a.m. | #1
On February 11, 2019 2:09:30 AM GMT+01:00, Hans-Peter Nilsson <hans-peter.nilsson@axis.com> 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
Jakub Jelinek Feb. 11, 2019, 9:22 a.m. | #2
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 <richard.guenther@gmail.com>

> 

> > >+      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.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);


	Jakub
Jeff Law April 30, 2019, 5:37 p.m. | #3
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

Patch

--- 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;
+
+      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);