[PATCH] Drop excess size used for run time allocated stack variables.

[Dominik Vogt]

What do we do now with the two patches?  At the moment, the
functional patch depends on the changes in the cleanup patch, so
it cannot be applied on its own.  Options:

(with the requested cleanup in the functional patch)

 1) Apply both patches as they are now and do further cleanup on
    top of it.
 2) Rewrite the functional patch so that it applies without the
    cleanup patch and commit it now.
 3) Look into the suggested cleanup now and adapt the functional
    patch to it when its ready.

Actually I'd prefer (1) or (2) to just get the functional patch
off my desk.  I agree that the cleanup is very useful, but there's
not relation between the cleanup and the functional stuff except
that they touch the same code.  Having the functional patch
applied would simplify further work for me.

On Thu, Jun 09, 2016 at 02:00:21PM +0200, Bernd Schmidt wrote:
> On 05/20/2016 01:11 AM, Jeff Law wrote:
> >Let's start with clean up of dead code:
> >
> > /* We will need to ensure that the address we return is aligned to
> >     REQUIRED_ALIGN.  If STACK_DYNAMIC_OFFSET is defined, we don't
> >     always know its final value at this point in the compilation (it
> >     might depend on the size of the outgoing parameter lists, for
> >     example), so we must align the value to be returned in that case.
> >     (Note that STACK_DYNAMIC_OFFSET will have a default nonzero value if
> >     STACK_POINTER_OFFSET or ACCUMULATE_OUTGOING_ARGS are defined).
> >     We must also do an alignment operation on the returned value if
> >     the stack pointer alignment is less strict than REQUIRED_ALIGN.
> >
> >     If we have to align, we must leave space in SIZE for the hole
> >     that might result from the alignment operation.  */
> >
> >  must_align = (crtl->preferred_stack_boundary < required_align);
> >  if (must_align)
> >    {
> >      if (required_align > PREFERRED_STACK_BOUNDARY)
> >        extra_align = PREFERRED_STACK_BOUNDARY;
> >      else if (required_align > STACK_BOUNDARY)
> >        extra_align = STACK_BOUNDARY;
> >      else
> >        extra_align = BITS_PER_UNIT;
> >    }
> >
> >  /* ??? STACK_POINTER_OFFSET is always defined now.  */
> >#if defined (STACK_DYNAMIC_OFFSET) || defined (STACK_POINTER_OFFSET)
> >  must_align = true;
> >  extra_align = BITS_PER_UNIT;
> >#endif
> >
> >If we look at defaults.h, it always defines STACK_POINTER_OFFSET.  So
> >all the code above I think collapses to:
> >
> >  must_align = true;
> >  extra_align = BITS_PER_UNIT
> 
> (Cc'ing rth because portions of this seem to be his, r165240).
> 
> I kind of want to approach this from a different angle; let's look
> at extra_align. The way this is used subsequently makes it appear to
> be misnamed. It looks like it should hold the stack alignment we can
> assume:
> 
>   unsigned extra = (required_align - extra_align) / BITS_PER_UNIT;
> 
>   size = plus_constant (Pmode, size, extra);
> [...]
>   if (extra && size_align > extra_align)
>     size_align = extra_align;
> 
> (where size_align is the known alignment of the size of the block to
> be allocated). If I'm reading this right, then the first part of the
> cleanup ought to be to get the naming right.
> 
> So why BITS_PER_UNIT? Shouldn't it at least be STACK_BOUNDARY? Let's
> look at the previous block a little more closely.
> 
> >   must_align = (crtl->preferred_stack_boundary < required_align);
> 
> [ crtl->preferred_stack_boundary is initialized to STACK_BOUNDARY in
> cfgexpand and only ever increased ]
> 
> >   if (must_align)
> >     {
> 
> [ if must_align, then required_align > crtl->p_s_b >= STACK_BOUNDARY ]
> 
> >       if (required_align > PREFERRED_STACK_BOUNDARY)
> >         extra_align = PREFERRED_STACK_BOUNDARY;
> 
> [ so far so good ]
> 
> >       else if (required_align > STACK_BOUNDARY)
> >         extra_align = STACK_BOUNDARY;
> 
> [ always true, right? ]
> 
> >       else
> >         extra_align = BITS_PER_UNIT;
> >     }
> 
> [ dead code, right? ]
> 
> So we're left with the question of why extra_align is set to
> BITS_PER_UNIT for STACK_DYNAMIC_OFFSET, and I can't really see a
> reason to do that either. AFAIK the minimum alignment of the stack
> is always STACK_BOUNDARY, and it's possible we could do better.
> 
> As far as I can tell, no definition of STACK_DYNAMIC_OFFSET differs
> substantially from the default definition in function.c. Why
> couldn't we round up the outgoing_args_size to the preferred stack
> boundary (or a new value to keep track of the required alignment for
> dynamic allocations) before instantiating dynamic_offset? We then
> wouldn't have to add extra alignment for it here.
> 
> This rounding seems to happen anyway in port's frame calculations,
> e.g. here in i386:
> 
>  if (ACCUMULATE_OUTGOING_ARGS
>       && (!crtl->is_leaf || cfun->calls_alloca
>           || ix86_current_function_calls_tls_descriptor))
>     {
>       offset += crtl->outgoing_args_size;
>       frame->outgoing_arguments_size = crtl->outgoing_args_size;
>     }
>   else
>     frame->outgoing_arguments_size = 0;
> 
>   /* Align stack boundary.  Only needed if we're calling another function
>      or using alloca.  */
>   if (!crtl->is_leaf || cfun->calls_alloca
>       || ix86_current_function_calls_tls_descriptor)
>     offset = ROUND_UP (offset, preferred_alignment);
> 
> or here in rs6000:
>   info->parm_size    = RS6000_ALIGN (crtl->outgoing_args_size,
>                                          TARGET_ALTIVEC ? 16 : 8);
> 
> which could probably use the default definition of
> STACK_DYNAMIC_OFFSET instead of this, if the outgoing_args_size was
> rounded appropriately:
> #define STACK_DYNAMIC_OFFSET(FUNDECL)                                   \
>   (RS6000_ALIGN (crtl->outgoing_args_size,                              \
>                  (TARGET_ALTIVEC || TARGET_VSX) ? 16 : 8)               \
>    + (STACK_POINTER_OFFSET))

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany