diff mbox series

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

Message ID 20190110223222.GC30353@tucnak
State New
Headers show
Series Don't use align > MAX_SUPPORTED_STACK_ALIGNMENT in assign_stack_temp_for_type (PR bootstrap/88450) | expand

Commit Message

Jakub Jelinek Jan. 10, 2019, 10:32 p.m. UTC 
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.



	Jakub

Comments

H.J. Lu Jan. 10, 2019, 11:11 p.m. UTC | #1 
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.
Jakub Jelinek Jan. 10, 2019, 11:18 p.m. UTC | #2 
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
Eric Botcazou Jan. 21, 2019, 8:17 a.m. UTC | #3 
> 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.
diff mbox series

Patch

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