[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)

[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)

[Jakub Jelinek]

Hi!

The r266345 change breaks mingw bootstraps for quite some time.
The problem is that the dynamic realignment is done IMHO way too low.

The biggest problem is that when assign_stack_local_1 decides to do this
dynamic_align_addr, it emits wherever it is currently expanding code about 3
instructions to do something like new_base = (base + (align - 1)) / align * align
and returns a MEM with that new_base as the XEXP (mem, 0).  Unfortunately,
one of the common callers of assign_stack_local_1,
assign_stack_temp_from_type, does temporary stack slot management and reuses
already assigned slots if they are suitable somewhere else.  This of course
doesn't work at all if the instructions that set new_base don't dominate one
or more of the other uses.

Another problem is that in way too many cases we decide to choose
BIGGEST_ALIGNMENT for stack slots, even when not strictly needed.  E.g. any
BLKmode stack slot requests that BIGGEST_ALIGNMENT, even if TYPE_ALIGN is
much smaller and assign_stack_local_1 even asserts that for BLKmode the
alignment must be BIGGEST_ALIGNMENT.  E.g. on mingw with -mavx or -mavx512f
BIGGEST_ALIGNMENT is 256 or 512 bits, but MAX_SUPPORTED_STACK_ALIGNMENT is
128 bits.  So the PR84877 change, even if it didn't cause wrong-code, causes
huge amounts of stack slots to be unnecessarily overaligned with dynamic
realignment.

The following patch reverts to the previous behavior and moves this dynamic
stack realignment to the caller that needs it, which doesn't do caching and
where we can do it solely for this overaligned DECL_ALIGN.

If there are other spots that need this, wondering about:
              else
                copy = assign_temp (type, 1, 0);
in calls.c, either it can be done by using the variable-sized object
case in the then block, or could be done using assign_stack_local +
this short realignment too.

Bootstrapped/regtested on x86_64-linux, i686-linux, powerpc64le-linux,
bootstrapped on powerpc64-linux (regtest still pending there).

Ok for trunk?

2019-01-09  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/84877
	PR bootstrap/88450
	* function.c (assign_stack_local_1): Revert the 2018-11-21 changes.
	(assign_parm_setup_block): Do the argument slot realignment here
	instead.

--- gcc/function.c.jj	2019-01-09 15:39:26.261153591 +0100
+++ gcc/function.c	2019-01-09 16:17:59.757704567 +0100
@@ -377,7 +377,6 @@ assign_stack_local_1 (machine_mode mode,
   poly_int64 bigend_correction = 0;
   poly_int64 slot_offset = 0, old_frame_offset;
   unsigned int alignment, alignment_in_bits;
-  bool dynamic_align_addr = false;
 
   if (align == 0)
     {
@@ -396,22 +395,14 @@ assign_stack_local_1 (machine_mode mode,
 
   alignment_in_bits = alignment * BITS_PER_UNIT;
 
+  /* Ignore alignment if it exceeds MAX_SUPPORTED_STACK_ALIGNMENT.  */
   if (alignment_in_bits > MAX_SUPPORTED_STACK_ALIGNMENT)
     {
-      /* If the required alignment exceeds MAX_SUPPORTED_STACK_ALIGNMENT and
-	 it is not OK to reduce it.  Align the slot dynamically.  */
-      if (mode == BLKmode
-	  && (kind & ASLK_REDUCE_ALIGN) == 0
-	  && currently_expanding_to_rtl)
-	dynamic_align_addr = true;
-      else
-	{
-	  alignment_in_bits = MAX_SUPPORTED_STACK_ALIGNMENT;
-	  alignment = MAX_SUPPORTED_STACK_ALIGNMENT / BITS_PER_UNIT;
-	}
+      alignment_in_bits = MAX_SUPPORTED_STACK_ALIGNMENT;
+      alignment = MAX_SUPPORTED_STACK_ALIGNMENT / BITS_PER_UNIT;
     }
 
-  if (SUPPORTS_STACK_ALIGNMENT && !dynamic_align_addr)
+  if (SUPPORTS_STACK_ALIGNMENT)
     {
       if (crtl->stack_alignment_estimated < alignment_in_bits)
 	{
@@ -441,42 +432,10 @@ assign_stack_local_1 (machine_mode mode,
 	}
     }
 
-  /* Handle overalignment here for parameter copy on the stack.
-     Reserved enough space for it and dynamically align the address.
-     No free frame_space is added here.  */
-  if (dynamic_align_addr)
-    {
-      rtx allocsize = gen_int_mode (size, Pmode);
-      get_dynamic_stack_size (&allocsize, 0, alignment_in_bits, NULL);
-
-      /* This is the size of space needed to accommodate required size of data
-	 with given alignment.  */
-      poly_int64 len = rtx_to_poly_int64 (allocsize);
-      old_frame_offset = frame_offset;
-
-      if (FRAME_GROWS_DOWNWARD)
-	{
-	  frame_offset -= len;
-	  try_fit_stack_local (frame_offset, len, len,
-			       PREFERRED_STACK_BOUNDARY / BITS_PER_UNIT,
-			       &slot_offset);
-	}
-      else
-	{
-	  frame_offset += len;
-	  try_fit_stack_local (old_frame_offset, len, len,
-			       PREFERRED_STACK_BOUNDARY / BITS_PER_UNIT,
-			       &slot_offset);
-	}
-      goto found_space;
-    }
-  else
-    {
-      if (crtl->stack_alignment_needed < alignment_in_bits)
-	crtl->stack_alignment_needed = alignment_in_bits;
-      if (crtl->max_used_stack_slot_alignment < alignment_in_bits)
-	crtl->max_used_stack_slot_alignment = alignment_in_bits;
-    }
+  if (crtl->stack_alignment_needed < alignment_in_bits)
+    crtl->stack_alignment_needed = alignment_in_bits;
+  if (crtl->max_used_stack_slot_alignment < alignment_in_bits)
+    crtl->max_used_stack_slot_alignment = alignment_in_bits;
 
   if (mode != BLKmode || maybe_ne (size, 0))
     {
@@ -563,12 +522,6 @@ assign_stack_local_1 (machine_mode mode,
 			  (slot_offset + bigend_correction,
 			   Pmode));
 
-  if (dynamic_align_addr)
-    {
-      addr = align_dynamic_address (addr, alignment_in_bits);
-      mark_reg_pointer (addr, alignment_in_bits);
-    }
-
   x = gen_rtx_MEM (mode, addr);
   set_mem_align (x, alignment_in_bits);
   MEM_NOTRAP_P (x) = 1;
@@ -2960,8 +2913,21 @@ assign_parm_setup_block (struct assign_p
   if (stack_parm == 0)
     {
       SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), BITS_PER_WORD));
-      stack_parm = assign_stack_local (BLKmode, size_stored,
-				       DECL_ALIGN (parm));
+      if (DECL_ALIGN (parm) > MAX_SUPPORTED_STACK_ALIGNMENT)
+	{
+	  rtx allocsize = gen_int_mode (size, Pmode);
+	  get_dynamic_stack_size (&allocsize, 0, DECL_ALIGN (parm), NULL);
+	  stack_parm = assign_stack_local (BLKmode, UINTVAL (allocsize),
+					   MAX_SUPPORTED_STACK_ALIGNMENT);
+	  rtx addr = align_dynamic_address (XEXP (stack_parm, 0),
+					    DECL_ALIGN (parm));
+	  mark_reg_pointer (addr, DECL_ALIGN (parm));
+	  stack_parm = gen_rtx_MEM (GET_MODE (stack_parm), addr);
+	  MEM_NOTRAP_P (stack_parm) = 1;
+	}
+      else
+	stack_parm = assign_stack_local (BLKmode, size_stored,
+					 DECL_ALIGN (parm));
       if (known_eq (GET_MODE_SIZE (GET_MODE (entry_parm)), size))
 	PUT_MODE (stack_parm, GET_MODE (entry_parm));
       set_mem_attributes (stack_parm, parm, 1);


	Jakub

Re: [PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)

[Eric Botcazou]

> Another problem is that in way too many cases we decide to choose
> BIGGEST_ALIGNMENT for stack slots, even when not strictly needed.  E.g. any
> BLKmode stack slot requests that BIGGEST_ALIGNMENT, even if TYPE_ALIGN is
> much smaller and assign_stack_local_1 even asserts that for BLKmode the
> alignment must be BIGGEST_ALIGNMENT.  E.g. on mingw with -mavx or -mavx512f
> BIGGEST_ALIGNMENT is 256 or 512 bits, but MAX_SUPPORTED_STACK_ALIGNMENT is
> 128 bits.  So the PR84877 change, even if it didn't cause wrong-code, causes
> huge amounts of stack slots to be unnecessarily overaligned with dynamic
> realignment.

Yes, I think that we need to make sure that we dynamically realign only when 
this is necessary and...

> The following patch reverts to the previous behavior and moves this dynamic
> stack realignment to the caller that needs it, which doesn't do caching and
> where we can do it solely for this overaligned DECL_ALIGN.

...this modified implementation looks much safer than the original one.

> If there are other spots that need this, wondering about:
>               else
>                 copy = assign_temp (type, 1, 0);
> in calls.c, either it can be done by using the variable-sized object
> case in the then block, or could be done using assign_stack_local +
> this short realignment too.

The latter I'd say.

> Bootstrapped/regtested on x86_64-linux, i686-linux, powerpc64le-linux,
> bootstrapped on powerpc64-linux (regtest still pending there).
> 
> Ok for trunk?
> 
> 2019-01-09  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/84877
> 	PR bootstrap/88450
> 	* function.c (assign_stack_local_1): Revert the 2018-11-21 changes.
> 	(assign_parm_setup_block): Do the argument slot realignment here
> 	instead.

FWIW this looks OK to me.

-- 
Eric Botcazou

Re: [PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)

[Richard Biener]

On January 10, 2019 4:36:35 PM GMT+01:00, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Another problem is that in way too many cases we decide to choose
>> BIGGEST_ALIGNMENT for stack slots, even when not strictly needed. 
>E.g. any
>> BLKmode stack slot requests that BIGGEST_ALIGNMENT, even if
>TYPE_ALIGN is
>> much smaller and assign_stack_local_1 even asserts that for BLKmode
>the
>> alignment must be BIGGEST_ALIGNMENT.  E.g. on mingw with -mavx or
>-mavx512f
>> BIGGEST_ALIGNMENT is 256 or 512 bits, but
>MAX_SUPPORTED_STACK_ALIGNMENT is
>> 128 bits.  So the PR84877 change, even if it didn't cause wrong-code,
>causes
>> huge amounts of stack slots to be unnecessarily overaligned with
>dynamic
>> realignment.
>
>Yes, I think that we need to make sure that we dynamically realign only
>when 
>this is necessary and...
>
>> The following patch reverts to the previous behavior and moves this
>dynamic
>> stack realignment to the caller that needs it, which doesn't do
>caching and
>> where we can do it solely for this overaligned DECL_ALIGN.
>
>...this modified implementation looks much safer than the original one.
>
>> If there are other spots that need this, wondering about:
>>               else
>>                 copy = assign_temp (type, 1, 0);
>> in calls.c, either it can be done by using the variable-sized object
>> case in the then block, or could be done using assign_stack_local +
>> this short realignment too.
>
>The latter I'd say.
>
>> Bootstrapped/regtested on x86_64-linux, i686-linux,
>powerpc64le-linux,
>> bootstrapped on powerpc64-linux (regtest still pending there).
>> 
>> Ok for trunk?
>> 
>> 2019-01-09  Jakub Jelinek  <jakub@redhat.com>
>> 
>> 	PR middle-end/84877
>> 	PR bootstrap/88450
>> 	* function.c (assign_stack_local_1): Revert the 2018-11-21 changes.
>> 	(assign_parm_setup_block): Do the argument slot realignment here
>> 	instead.
>
>FWIW this looks OK to me.

Given that, 

OK.. 
Richard. 

[PATCH] Don't use align > MAX_SUPPORTED_STACK_ALIGNMENT in assign_stack_temp_for_type (PR bootstrap/88450)

[Jakub Jelinek]

Hi!

On Thu, Jan 10, 2019 at 04:36:35PM +0100, Eric Botcazou wrote:
> > If there are other spots that need this, wondering about:
> >               else
> >                 copy = assign_temp (type, 1, 0);
> > in calls.c, either it can be done by using the variable-sized object
> > case in the then block, or could be done using assign_stack_local +
> > this short realignment too.
> 
> The latter I'd say.

Will handle that tomorrow.

But, there is another thing, while assign_stack_local_1 lowers
alignment_in_bits to MAX_SUPPORTED_STACK_ALIGNMENT if it is higher than that
and records that in the MEM it creates, the caller,
assign_stack_temp_for_type will happily count with higher alignments and
on the MEMs it creates will happily set MEM_ALIGN to the higher value.
I think we shouldn't lie here, something in the optimizers could try to take
advantage of the higher MEM_ALIGN.

Bootstrapped/regtested on x86_64-linux and i686-linux, but that doesn't mean
much, because MAX_SUPPORTED_STACK_ALIGNMENT there is 1 << 28.  Guess more
useful would be to test it on mingw where BIGGEST_ALIGNMENT is often higher
than MAX_SUPPORTED_STACK_ALIGNMENT.

Thoughts on this?

2019-01-10  Jakub Jelinek  <jakub@redhat.com>

	PR bootstrap/88450
	* function.c (assign_stack_temp_for_type): Use alignment at most
	MAX_SUPPORTED_STACK_ALIGNMENT.  Adjust assert correspondingly.

--- gcc/function.c.jj	2019-01-10 00:13:47.593688442 +0100
+++ gcc/function.c	2019-01-10 00:17:42.464890435 +0100
@@ -792,6 +792,7 @@ assign_stack_temp_for_type (machine_mode
   gcc_assert (known_size_p (size));
 
   align = get_stack_local_alignment (type, mode);
+  align = MIN (align, MAX_SUPPORTED_STACK_ALIGNMENT);
 
   /* Try to find an available, already-allocated temporary of the proper
      mode which meets the size and alignment requirements.  Choose the
@@ -872,8 +873,10 @@ assign_stack_temp_for_type (machine_mode
 
 	 So for requests which depended on the rounding of SIZE, we go ahead
 	 and round it now.  We also make sure ALIGNMENT is at least
-	 BIGGEST_ALIGNMENT.  */
-      gcc_assert (mode != BLKmode || align == BIGGEST_ALIGNMENT);
+	 minimum of BIGGEST_ALIGNMENT and MAX_SUPPORTED_STACK_ALIGNMENT.  */
+      gcc_assert (mode != BLKmode
+		  || align == MIN (BIGGEST_ALIGNMENT,
+				   MAX_SUPPORTED_STACK_ALIGNMENT));
       p->slot = assign_stack_local_1 (mode,
 				      (mode == BLKmode
 				       ? aligned_upper_bound (size,


	Jakub

Re: [PATCH] Don't use align > MAX_SUPPORTED_STACK_ALIGNMENT in assign_stack_temp_for_type (PR bootstrap/88450)

[H.J. Lu]

On Thu, Jan 10, 2019 at 2:32 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> On Thu, Jan 10, 2019 at 04:36:35PM +0100, Eric Botcazou wrote:
> > > If there are other spots that need this, wondering about:
> > >               else
> > >                 copy = assign_temp (type, 1, 0);
> > > in calls.c, either it can be done by using the variable-sized object
> > > case in the then block, or could be done using assign_stack_local +
> > > this short realignment too.
> >
> > The latter I'd say.
>
> Will handle that tomorrow.
>
> But, there is another thing, while assign_stack_local_1 lowers
> alignment_in_bits to MAX_SUPPORTED_STACK_ALIGNMENT if it is higher than that
> and records that in the MEM it creates, the caller,
> assign_stack_temp_for_type will happily count with higher alignments and
> on the MEMs it creates will happily set MEM_ALIGN to the higher value.
> I think we shouldn't lie here, something in the optimizers could try to take
> advantage of the higher MEM_ALIGN.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, but that doesn't mean
> much, because MAX_SUPPORTED_STACK_ALIGNMENT there is 1 << 28.  Guess more
> useful would be to test it on mingw where BIGGEST_ALIGNMENT is often higher
> than MAX_SUPPORTED_STACK_ALIGNMENT.

FWIW, MAX_SUPPORTED_STACK_ALIGNMENT is an arbitrary large value for
Linux/x86 since we track and align stack as needed.

-- 
H.J.

Re: [PATCH] Don't use align > MAX_SUPPORTED_STACK_ALIGNMENT in assign_stack_temp_for_type (PR bootstrap/88450)

[Jakub Jelinek]

On Thu, Jan 10, 2019 at 03:11:18PM -0800, H.J. Lu wrote:
> > Bootstrapped/regtested on x86_64-linux and i686-linux, but that doesn't mean
> > much, because MAX_SUPPORTED_STACK_ALIGNMENT there is 1 << 28.  Guess more
> > useful would be to test it on mingw where BIGGEST_ALIGNMENT is often higher
> > than MAX_SUPPORTED_STACK_ALIGNMENT.
> 
> FWIW, MAX_SUPPORTED_STACK_ALIGNMENT is an arbitrary large value for
> Linux/x86 since we track and align stack as needed.

Yes, I know.  Which is why I've said that the passed bootstrap/regtest on
{x86_64,i686}-linux isn't really meaningful, since the patch probably never
changed anything at all, and probably not really useful on powerpc*, as
MAX_SUPPORTED_STACK_ALIGNMENT is there usually 128 and BIGGEST_ALIGNMENT 128
as well.

	Jakub

Re: [PATCH] Don't use align > MAX_SUPPORTED_STACK_ALIGNMENT in assign_stack_temp_for_type (PR bootstrap/88450)

[Eric Botcazou]

> Bootstrapped/regtested on x86_64-linux and i686-linux, but that doesn't mean
> much, because MAX_SUPPORTED_STACK_ALIGNMENT there is 1 << 28.  Guess more
> useful would be to test it on mingw where BIGGEST_ALIGNMENT is often higher
> than MAX_SUPPORTED_STACK_ALIGNMENT.
> 
> Thoughts on this?
> 
> 2019-01-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR bootstrap/88450
> 	* function.c (assign_stack_temp_for_type): Use alignment at most
> 	MAX_SUPPORTED_STACK_ALIGNMENT.  Adjust assert correspondingly.

Not clear to me if this shouldn't be done in get_stack_local_alignment instead 
but I don't really have a strong opinion here.

-- 
Eric Botcazou

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

[Hans-Peter Nilsson]

> Date: Thu, 10 Jan 2019 00:06:01 +0100
> From: Jakub Jelinek <jakub@redhat.com>

> 2019-01-09  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/84877
> 	PR bootstrap/88450
> 	* function.c (assign_stack_local_1): Revert the 2018-11-21 changes.
> 	(assign_parm_setup_block): Do the argument slot realignment here
> 	instead.

When this was committed as r267812, results for cris-elf went
from regress-8 to regress-160 (now at r268749, regress-154).

<TL;DR>
I analyzed one of the simpler cases,
gcc.c-torture/execute/930126-1.c at -O0.

It looks like the code added in r267812 doesn't handle targets
where MAX_SUPPORTED_STACK_ALIGNMENT is less than BITS_PER_WORD
(by necessity, non-STRICT_ALIGNMENT targets); the bug is in not
adding necessary alignment to the allocated space.

cris-elf is a non-STRICT_ALIGNMENT target and
MAX_SUPPORTED_STACK_ALIGNMENT is defaulted from STACK_BOUNDARY,
16 (bits).

The only difference with r267812 is that before, 10 bytes
(excluding 4 for the saved frame-pointer-address) was allocated
for the stack-frame of the function f and at r267812, 8 bytes;
the literally only differences are two instructions, one when
allocating stack and one when forming the pointer to the
stack-parameter.  The sizeof the actual object is 5 bytes, but
padded to 8 bytes due to copying from incoming registers (which
is IMO a valid reason, maybe not worthwhile to improve).  Please
remember that size-padding is different from alignment-padding.

The setting of DECL_ALIGN (parm) to BITS_PER_WORD (originating
at r94104) is wrong too, but not fatal.  I'll fix that in a
follow-up change and let's pretend for the moment that there's a
valid reason for aligning "parm" in this case (for example, a
user-provided overalignment).

At function entry, after saving the frame-pointer-register, the
stack-pointer is 0x3dfffece (both versions).  The parameter-
pointer-align instructions align this to a multiple of 4 for
both versions, and this of course fails as there's no padding
with r267812.  The actual failure is due to overwriting the
saved frame-pointer-register, with the wrong value then used in
main to verify the result.
</TL;DR>

To fix the r267812 incorrectness we need to use the *stored*
size, i.e. that which will be stored into using register-sized
writes.  It's seems like the bug is just a typo, so the fix is
as simple as follows.  Note the usage of "diff -U 10" to show
that size_stored is used in the "then"-arm.

Regtested on cris-elf, where it "introduces" gcc.dg/pr84877.c
but on inspection that's a known bug with a bit of hairy churn,
reverts and all.  See the PR.  This patch has no bearing on that
PR; this is for incoming parameters, and PR84877 is for (not
doing) alignment of pass-by-reference parameters for outgoing
parameters.  Still, maybe the solution to that PR should have
code like in that the context below...which I see you already
noted, in the patch-message to which this is a reply.

Also regtested on x86_64-pc-linux-gnu (without regressions)
together with the followup-change.

Ok to commit?

gcc/ChangeLog:
	* function.c (assign_parm_setup_block): Use the stored
	size, not the passed size, when allocating stack-space,
	also for a parameter with alignment larger than
	MAX_SUPPORTED_STACK_ALIGNMENT.

both arms of the containing "if".)
--- gcc/function.c.orig	Sun Jan 20 19:20:16 2019
+++ gcc/function.c	Sat Feb  9 00:53:17 2019
@@ -2907,23 +2907,23 @@ assign_parm_setup_block (struct assign_p
 	}
       data->stack_parm = NULL;
     }
 
   size = int_size_in_bytes (data->passed_type);
   size_stored = CEIL_ROUND (size, UNITS_PER_WORD);
   if (stack_parm == 0)
     {
       SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), BITS_PER_WORD));
       if (DECL_ALIGN (parm) > MAX_SUPPORTED_STACK_ALIGNMENT)
 	{
-	  rtx allocsize = gen_int_mode (size, Pmode);
+	  rtx allocsize = gen_int_mode (size_stored, Pmode);
 	  get_dynamic_stack_size (&allocsize, 0, DECL_ALIGN (parm), NULL);
 	  stack_parm = assign_stack_local (BLKmode, UINTVAL (allocsize),
 					   MAX_SUPPORTED_STACK_ALIGNMENT);
 	  rtx addr = align_dynamic_address (XEXP (stack_parm, 0),
 					    DECL_ALIGN (parm));
 	  mark_reg_pointer (addr, DECL_ALIGN (parm));
 	  stack_parm = gen_rtx_MEM (GET_MODE (stack_parm), addr);
 	  MEM_NOTRAP_P (stack_parm) = 1;
 	}
       else
 	stack_parm = assign_stack_local (BLKmode, size_stored,

brgds, H-P

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

[Hans-Peter Nilsson]

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;
+
+      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 to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"

[Hans-Peter Nilsson]

> Date: Mon, 11 Feb 2019 02:05:11 +0100
> From: Hans-Peter Nilsson <hp@axis.com>

> Regtested on cris-elf, where it "introduces" gcc.dg/pr84877.c

Correction: "no regressions" (not introduced by this proposed
patch, I misread).

brgds, H-P

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

[Richard Biener]

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

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

[Eric Botcazou]

> To fix the r267812 incorrectness we need to use the *stored*
> size, i.e. that which will be stored into using register-sized
> writes.  It's seems like the bug is just a typo, so the fix is
> as simple as follows.  Note the usage of "diff -U 10" to show
> that size_stored is used in the "then"-arm.

Right, thanks for catching it.

> Ok to commit?
> 
> gcc/ChangeLog:
> 	* function.c (assign_parm_setup_block): Use the stored
> 	size, not the passed size, when allocating stack-space,
> 	also for a parameter with alignment larger than
> 	MAX_SUPPORTED_STACK_ALIGNMENT.

OK, thanks.

-- 
Eric Botcazou

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

[Hans-Peter Nilsson]

> 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?

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

brgds, H-P

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

[Jakub Jelinek]

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

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

[Hans-Peter Nilsson]

> Date: Mon, 11 Feb 2019 10:22:12 +0100
> From: Jakub Jelinek <jakub@redhat.com>

> 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)"

[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)


Jeff

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

[Hans-Peter Nilsson]

> Date: Tue, 30 Apr 2019 11:37:17 -0600
> From: Jeff Law <law@redhat.com>

> 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 271111)
+++ 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)"

[Jeff Law]

On 5/12/19 10:01 PM, Hans-Peter Nilsson wrote:
>> Date: Tue, 30 Apr 2019 11:37:17 -0600
>> From: Jeff Law <law@redhat.com>
> 
>> 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 271111)
> +++ 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