Re: [PATCH v2] Allocate constant size dynamic stack space in the prologue

[Jeff Law <law at redhat dot com>]

On 05/06/2016 03:37 AM, Dominik Vogt wrote:
> Updated version of the patch described below.  Apart from fixing a
> bug and adding a test, the new logic is now used always, for all
> targets.  The discussion of the original patch starts here:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03052.html
>
> The new patch has been bootstrapped and regression tested on s390,
> s390x and x86_64, but please check the questions/comments in the
> follow up message.
>
> On Wed, Nov 25, 2015 at 01:56:10PM +0100, Dominik Vogt wrote:
> > > The attached patch fixes a warning during Linux kernel compilation
> > > on S/390 due to -mwarn-dynamicstack and runtime alignment of stack
> > > variables with constant size causing cfun->calls_alloca to be set
> > > (even if alloca is not used at all).  The patched code places
> > > constant size runtime aligned variables in the "virtual stack
> > > vars" area instead of creating a "virtual stack dynamic" area.
> > >
> > > This behaviour is activated by defining
> > >
> > >   #define ALLOCATE_DYNAMIC_STACK_SPACE_IN_PROLOGUE 1
Is there some reason why we don't just to this unconditionally?  ie, if 
we know the size of dynamic space, why not just always handle that in 
the prologue?  Seems like a useful optimization for a variety of reasons.

Of course if we do this unconditionally, we definitely need to find a 
way to test it better.

In reality, I don't see where/how the patch uses 
ALLOCATE_DYNAMIC_STACK_SPACE_IN_PROLOGUE anyway and it seems to be 
enabled for all targets, which is what I want :-)



> > >
> > > in the backend; otherwise the old logic is used.
> > >
> > > The kernel uses runtime alignment for the page structure (aligned
> > > to 16 bytes), and apart from triggereing the alloca warning
> > > (-mwarn-dynamicstack), the current Gcc also generates inefficient
> > > code like
> > >
> > >   aghi %r15,-160  # prologue: create stack frame
> > >   lgr %r11,%r15   # prologue: generate frame pointer
> > >   aghi %r15,-32   # space for dynamic stack
> > >
> > > which could be simplified to
> > >
> > >   aghi %r15,-192
> > >
> > > (if later optimization passes are able to get rid of the frame
> > > pointer).  Is there a specific reason why the patched behaviour
> > > shouldn't be used for all platforms?
> > >
> > > --
> > >
> > > As the placement of runtime aligned stack variables with constant
> > > size is done completely in the middleend, I don't see a way to fix
> > > this in the backend.
> Ciao
>
> Dominik ^_^  ^_^
>
> -- Dominik Vogt IBM Germany
>
>
> 0001-v2-ChangeLog
>
>
> gcc/ChangeLog
>
> 	* cfgexpand.c (expand_stack_vars): Implement synamic stack space
> 	allocation in the prologue.
> 	* explow.c (get_dynamic_stack_base): New function to return an address
> 	expression for the dynamic stack base.
> 	(get_dynamic_stack_size): New function to do the required dynamic stack
> 	space size calculations.
> 	(allocate_dynamic_stack_space): Use new functions.
> 	(align_dynamic_address): Move some code from
> 	allocate_dynamic_stack_space to new function.
> 	* explow.h (get_dynamic_stack_base, get_dynamic_stack_size): Export.
> gcc/testsuite/ChangeLog
>
> 	* gcc.target/s390/warn-dynamicstack-1.c: New test.
> 	* gcc.dg/stack-usage-2.c (foo3): Adapt expected warning.
> 	stack-layout-dynamic-1.c: New test.
>
>
> 0001-v2-Allocate-constant-size-dynamic-stack-space-in-the-pr.patch
>
>
> From e76a7e02f7862681d1b5344e64aca1b0a62cdc2c Mon Sep 17 00:00:00 2001
> From: Dominik Vogt <vogt@linux.vnet.ibm.com>
> Date: Wed, 25 Nov 2015 09:31:19 +0100
> Subject: [PATCH] Allocate constant size dynamic stack space in the
>  prologue ...
>
> ... and place it in the virtual stack vars area, if the platform supports it.
> On S/390 this saves adjusting the stack pointer twice and forcing the frame
> pointer into existence.  It also removes the warning with -mwarn-dynamicstack
> that is triggered by cfun->calls_alloca == 1.
>
> This fixes a problem with the Linux kernel which aligns the page structure to
> 16 bytes at run time using inefficient code and issuing a bogus warning.

> @@ -1186,6 +1190,18 @@ expand_stack_vars (bool (*pred) (size_t), struct stack_vars_data *data)
>  	  /* Large alignment is only processed in the last pass.  */
>  	  if (pred)
>  	    continue;
> +
> +	  if (large_allocsize && ! large_allocation_done)
> +	    {
> +	      /* Allocate space the virtual stack vars area in the prologue.
> +	       */
Line wrapping nit here.  Bring "prologue" down to the next line.

I really like this.  I think the big question is how do we test it.  I 
suspect our bootstrap and regression suite probably don't exercise this 
code is any significant way.

Jeff