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

[Bernd Schmidt <bschmidt@redhat.com>]

On 11/25/2015 01:56 PM, 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
>
> in the backend; otherwise the old logic is used.
>
> The kernel uses runtime alignment for the page structure (aligned
> to 16 bytes),

Just curious, is that necessary or is it an optimization for statically 
allocated page structures?

> 	* cfgexpand.c (expand_stack_vars): Implement
> 	ALLOCATE_DYNAMIC_STACK_SPACE_IN_PROLOGUE.
> 	* explow.c (get_dynamic_stack_base): New function to return an address
> 	expression for the dynamic stack base when using
> 	ALLOCATE_DYNAMIC_STACK_SPACE_IN_PROLOGUE.
> 	(allocate_dynamic_stack_space): Use new functions.
> 	(align_dynamic_address, adjust_size_align): Move some code
> 	from allocate_dynamic_stack_space to new functions.
> 	* explow.h (get_dynamic_stack_base): Export.
> 	* doc/tm.texi (ALLOCATE_DYNAMIC_STACK_SPACE_IN_PROLOGUE): Document.
> 	* config/s390/s390.h (ALLOCATE_DYNAMIC_STACK_SPACE_IN_PROLOGUE): Define.
> 	* defaults.h (ALLOCATE_DYNAMIC_STACK_SPACE_IN_PROLOGUE): Define by
> 	default.

I think the approach is quite reasonable. Not sure whether this is 
appropriate for stage3 - it does look slightly risky and may not be 
worth it at this point for just fixing a warning.

However, I don't think this should be a target-controlled thing, just 
make it use the new behaviour unconditionally. Also, in the future, when 
making something target-controlled, use a hook, not a macro.

> +	      /* Allocate space in the prologue, at the beginning of the virtual
> +		 stack vars area.  */

Is this really at the beginning? What if FRAME_GROWS_DOWNWARD?

> /* Common code used by allocate_dynamic_stack_space and get_dynamic_stack_base.
> +   Adjust SIZE_ALIGN for SIZE, if needed, and returns the updated value.  */

The comment is meaningless. Adjust how?

The new function get_dynamic_stack base looks like a shrunk-down version 
of allocate_dynamic_stack_space. What I'm worried about is that it makes 
various adjustments to the size, and these are not mirrored in 
expand_stack_vars. That function already has (after your patch)

+	      size = large_size + large_align / BITS_PER_UNIT;

So no further adjustment should be necessary. Right?

> +  extra = (required_align - BITS_PER_UNIT) / BITS_PER_UNIT;

allocate_dynamic_stack_space has extra_align here instead of the first 
BITS_PER_UNIT. Why isn't this retained (or, as pointed out above, why is 
any of this code here in the first place)?


Bernd