Patchwork Fix DECL_ALIGN during expand with dynamic stack realignment (PR target/44542)

login
register
mail settings
Submitter Jakub Jelinek
Date June 17, 2010, 12:20 p.m.
Message ID <20100617122054.GW7811@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/56027/
State New
Headers show

Comments

Jakub Jelinek - June 17, 2010, 12:20 p.m.
Hi!

As discussed in the PR, with dynamic stack realignment (== x86_64/i686
right now) expand_one_stack_var_at sometimes sets DECL_ALIGN to a too high
value, bigger than what the stack will be actually aligned.

The purpose of the expand_one_stack_var_at munging of DECL_ALIGN is likely
that if a decl with small requested alignment is given some very nicely
aligned offset  we shouldn't hide that fact from the expanders.
The alignment is computed just from the offset though, so say for
offset 64 it sets DECL_ALIGN to 512 bits.  It needs to
take into account alignment of the stack as well though, the decl
is only aligned to 512 bits in that case if the base is at least that much
aligned.  Without dynamic stack realignment we should know that already
(and, the patch shouldn't change anything for those), with dynamic stack
realignment we unfortunately don't know the final alignment yet, but we should
know some lower boundary for it.

The last hunk is just to make sure the so computed DECL_ALIGN isn't later on
considered again for stack realignment purposes.  On the 4.4 branch
expand_one_var could be called for the same variable more than once, on the
trunk it is unclear whether that can happen and is just a latent issue.
I'm aware that Michael suggested doing the adjustments only for
!really_expand, but we never call it with !really_expand at -O0 and for -O1+
we call it too early (during inlining etc.), rather than right before the
actual expansion.

Bootstrapped/regtested on x86_64-linux and i686-linux.  Ok for trunk?

2010-06-17  Jakub Jelinek  <jakub@redhat.com>

	PR target/44542
	* cfgexpand.c (expand_one_stack_var_at): Limit align to maximum
	of max_used_stack_slot_alignment and PREFERRED_STACK_BOUNDARY
	instead of MAX_SUPPORTED_STACK_ALIGNMENT.
	(expand_one_var): Don't consider DECL_ALIGN for variables for
	which expand_one_stack_var_at has been already called.


	Jakub
Michael Matz - June 17, 2010, 2:52 p.m.
Hi,

On Thu, 17 Jun 2010, Jakub Jelinek wrote:

> The last hunk is just to make sure the so computed DECL_ALIGN isn't later on
> considered again for stack realignment purposes.  On the 4.4 branch
> expand_one_var could be called for the same variable more than once, on the
> trunk it is unclear whether that can happen and is just a latent issue.
> I'm aware that Michael suggested doing the adjustments only for
> !really_expand, but we never call it with !really_expand at -O0 and for -O1+
> we call it too early (during inlining etc.), rather than right before the
> actual expansion.

Yeah, I misremembered the flow of events when making this inquiry.  Still 
...

> +      else if (DECL_HAS_VALUE_EXPR_P (var)
> +	       || (DECL_RTL_SET_P (var)
> +		   && MEM_P (DECL_RTL (var))
> +		   && (XEXP (DECL_RTL (var), 0) == virtual_stack_vars_rtx
> +		       || (GET_CODE (XEXP (DECL_RTL (var), 0)) == PLUS
> +			   && XEXP (XEXP (DECL_RTL (var), 0), 0)
> +			      == virtual_stack_vars_rtx
> +			   && CONST_INT_P (XEXP (XEXP (DECL_RTL (var), 0), 1))))))

... this reads uneasy to me (that's the reason I bothered with the 
really_expand suggestion).  Why do you have to test the form of the MEM, 
shouldn't "DECL_RTL_SET_P (var) && MEM_P (DECL_RTL (var))" be enough, or 
even without testing for MEM_P?


Ciao,
Michael.
Jakub Jelinek - June 17, 2010, 2:57 p.m.
On Thu, Jun 17, 2010 at 04:52:38PM +0200, Michael Matz wrote:
> Yeah, I misremembered the flow of events when making this inquiry.  Still 
> ...
> 
> > +      else if (DECL_HAS_VALUE_EXPR_P (var)
> > +	       || (DECL_RTL_SET_P (var)
> > +		   && MEM_P (DECL_RTL (var))
> > +		   && (XEXP (DECL_RTL (var), 0) == virtual_stack_vars_rtx
> > +		       || (GET_CODE (XEXP (DECL_RTL (var), 0)) == PLUS
> > +			   && XEXP (XEXP (DECL_RTL (var), 0), 0)
> > +			      == virtual_stack_vars_rtx
> > +			   && CONST_INT_P (XEXP (XEXP (DECL_RTL (var), 0), 1))))))
> 
> ... this reads uneasy to me (that's the reason I bothered with the 
> really_expand suggestion).  Why do you have to test the form of the MEM, 
> shouldn't "DECL_RTL_SET_P (var) && MEM_P (DECL_RTL (var))" be enough, or 
> even without testing for MEM_P?

It probably is enough, TREE_STATIC || DECL_EXTERNAL is earlier, so the var
should be automatic then and I hope nothing else sets DECL_RTL to MEM for
those earlier.  Will bootstrap/regtest with everything after the MEM_P test
in an gcc_assert.

	Jakub
H.J. Lu - June 17, 2010, 3:29 p.m.
On Thu, Jun 17, 2010 at 5:20 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> As discussed in the PR, with dynamic stack realignment (== x86_64/i686
> right now) expand_one_stack_var_at sometimes sets DECL_ALIGN to a too high
> value, bigger than what the stack will be actually aligned.
>
> The purpose of the expand_one_stack_var_at munging of DECL_ALIGN is likely
> that if a decl with small requested alignment is given some very nicely
> aligned offset  we shouldn't hide that fact from the expanders.
> The alignment is computed just from the offset though, so say for
> offset 64 it sets DECL_ALIGN to 512 bits.  It needs to
> take into account alignment of the stack as well though, the decl
> is only aligned to 512 bits in that case if the base is at least that much
> aligned.  Without dynamic stack realignment we should know that already
> (and, the patch shouldn't change anything for those), with dynamic stack
> realignment we unfortunately don't know the final alignment yet, but we should
> know some lower boundary for it.
>
> The last hunk is just to make sure the so computed DECL_ALIGN isn't later on
> considered again for stack realignment purposes.  On the 4.4 branch
> expand_one_var could be called for the same variable more than once, on the
> trunk it is unclear whether that can happen and is just a latent issue.
> I'm aware that Michael suggested doing the adjustments only for
> !really_expand, but we never call it with !really_expand at -O0 and for -O1+
> we call it too early (during inlining etc.), rather than right before the
> actual expansion.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux.  Ok for trunk?
>
> 2010-06-17  Jakub Jelinek  <jakub@redhat.com>
>
>        PR target/44542
>        * cfgexpand.c (expand_one_stack_var_at): Limit align to maximum
>        of max_used_stack_slot_alignment and PREFERRED_STACK_BOUNDARY
>        instead of MAX_SUPPORTED_STACK_ALIGNMENT.
>        (expand_one_var): Don't consider DECL_ALIGN for variables for
>        which expand_one_stack_var_at has been already called.
>
> --- gcc/cfgexpand.c.jj  2010-06-17 08:16:56.000000000 +0200
> +++ gcc/cfgexpand.c     2010-06-17 10:16:35.000000000 +0200
> @@ -706,7 +706,7 @@ static void
>  expand_one_stack_var_at (tree decl, HOST_WIDE_INT offset)
>  {
>   /* Alignment is unsigned.   */
> -  unsigned HOST_WIDE_INT align;
> +  unsigned HOST_WIDE_INT align, max_align;
>   rtx x;
>
>   /* If this fails, we've overflowed the stack frame.  Error nicely?  */
> @@ -723,10 +723,12 @@ expand_one_stack_var_at (tree decl, HOST
>       offset -= frame_phase;
>       align = offset & -offset;
>       align *= BITS_PER_UNIT;
> +      max_align = MAX (crtl->max_used_stack_slot_alignment,
> +                      PREFERRED_STACK_BOUNDARY);
>       if (align == 0)
>        align = STACK_BOUNDARY;

Will a stack variable which should be aligned at 64byte with
ALIGN == 0? Do we need to check minimum alignment?
Jakub Jelinek - June 17, 2010, 3:41 p.m.
On Thu, Jun 17, 2010 at 08:29:27AM -0700, H.J. Lu wrote:
> > --- gcc/cfgexpand.c.jj  2010-06-17 08:16:56.000000000 +0200
> > +++ gcc/cfgexpand.c     2010-06-17 10:16:35.000000000 +0200
> > @@ -706,7 +706,7 @@ static void
> >  expand_one_stack_var_at (tree decl, HOST_WIDE_INT offset)
> >  {
> >   /* Alignment is unsigned.   */
> > -  unsigned HOST_WIDE_INT align;
> > +  unsigned HOST_WIDE_INT align, max_align;
> >   rtx x;
> >
> >   /* If this fails, we've overflowed the stack frame.  Error nicely?  */
> > @@ -723,10 +723,12 @@ expand_one_stack_var_at (tree decl, HOST
> >       offset -= frame_phase;
> >       align = offset & -offset;
> >       align *= BITS_PER_UNIT;
> > +      max_align = MAX (crtl->max_used_stack_slot_alignment,
> > +                      PREFERRED_STACK_BOUNDARY);
> >       if (align == 0)
> >        align = STACK_BOUNDARY;
> 
> Will a stack variable which should be aligned at 64byte with
> ALIGN == 0? Do we need to check minimum alignment?

That align == 0 case is really weird, apparently created by your patch.
I'd say that for align == 0 we should set DECL_ALIGN also to max_align.
So
	if (align == 0 || align > max_align)
	  align = max_align;

The comments say:
/* The phase of the stack frame.  This is the known misalignment of                                                                                
   virtual_stack_vars_rtx from PREFERRED_STACK_BOUNDARY.  That is,                                                                                 
   (frame_offset+frame_phase) % PREFERRED_STACK_BOUNDARY == 0.  */
and frame_phase is subtracted from offset before doing offset & -offset,
so if offset is 0 and thus align 0, it should be PREFERRED_STACK_BOUNDARY
aligned (for targets other than ix86/x86_64, for those 2 maybe more).
crtl->max_used_stack_slot_alignment will be at most PREFERRED_STACK_BOUNDARY
except for ix86/x86_64.

	Jakub

Patch

--- gcc/cfgexpand.c.jj	2010-06-17 08:16:56.000000000 +0200
+++ gcc/cfgexpand.c	2010-06-17 10:16:35.000000000 +0200
@@ -706,7 +706,7 @@  static void
 expand_one_stack_var_at (tree decl, HOST_WIDE_INT offset)
 {
   /* Alignment is unsigned.   */
-  unsigned HOST_WIDE_INT align;
+  unsigned HOST_WIDE_INT align, max_align;
   rtx x;
 
   /* If this fails, we've overflowed the stack frame.  Error nicely?  */
@@ -723,10 +723,12 @@  expand_one_stack_var_at (tree decl, HOST
       offset -= frame_phase;
       align = offset & -offset;
       align *= BITS_PER_UNIT;
+      max_align = MAX (crtl->max_used_stack_slot_alignment,
+		       PREFERRED_STACK_BOUNDARY);
       if (align == 0)
 	align = STACK_BOUNDARY;
-      else if (align > MAX_SUPPORTED_STACK_ALIGNMENT)
-	align = MAX_SUPPORTED_STACK_ALIGNMENT;
+      else if (align > max_align)
+	align = max_align;
 
       DECL_ALIGN (decl) = align;
       DECL_USER_ALIGN (decl) = 0;
@@ -931,6 +933,19 @@  expand_one_var (tree var, bool toplevel,
 	align = MINIMUM_ALIGNMENT (TREE_TYPE (var),
 				   TYPE_MODE (TREE_TYPE (var)),
 				   TYPE_ALIGN (TREE_TYPE (var)));
+      else if (DECL_HAS_VALUE_EXPR_P (var)
+	       || (DECL_RTL_SET_P (var)
+		   && MEM_P (DECL_RTL (var))
+		   && (XEXP (DECL_RTL (var), 0) == virtual_stack_vars_rtx
+		       || (GET_CODE (XEXP (DECL_RTL (var), 0)) == PLUS
+			   && XEXP (XEXP (DECL_RTL (var), 0), 0)
+			      == virtual_stack_vars_rtx
+			   && CONST_INT_P (XEXP (XEXP (DECL_RTL (var), 0), 1))))))
+	/* Don't consider debug only variables with DECL_HAS_VALUE_EXPR_P set
+	   or variables which were assigned a stack slot already by
+	   expand_one_stack_var_at - in the latter case DECL_ALIGN has been
+	   changed from the offset chosen to it.  */
+	align = crtl->stack_alignment_estimated;
       else
 	align = MINIMUM_ALIGNMENT (var, DECL_MODE (var), DECL_ALIGN (var));