| Message ID | 20100617122054.GW7811@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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?
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
--- 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));