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

[Dominik Vogt <vogt at linux dot vnet dot ibm dot com>]

On Wed, Nov 25, 2015 at 02:31:38PM +0100, Bernd Schmidt wrote:
> 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?

Without looking into the details, I believe it's an optimization
to have certain frequently used members of the struct always on
the same cache line.

> >	* 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.

Our hope, was to justify this change with "does nothing except on
S/390 for now".  Most of the diff in explow.c is just putting
common parts of allocate_dynamic_stack_space and
get_dynamic_stack_base into subfunctions.  It is possible to leave
the existing function allocate_... untouched and just duplicate
some more code in get_... and clean up the code duplication after
Gcc6.

> 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.

All right.

> >+	      /* 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?

I thought it was when I wrote the comment, but for S/390 it's
actually placed what I'd call "after" the other stack variables
(at a lower address).  The comment is misleading.  It does not
matter whether the area ends up at the beginning or end.

> >/* 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?

Yeah.  See below.

> The new function get_dynamic_stack base looks like a shrunk-down
> version of allocate_dynamic_stack_space.

It is.

> 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)?

Actually this whole calculation with size, size_align and extra is
bogus code.  The calculated values are not used.  This makes
adjust_size_align superfluous (and thereby "fixes" the comment
documenting that function).

I have two more questions regarding code copied frpm
allocate_dynamic_stack_space.

1. Is this really necessary in get_dynamic_stack_base?

> +  if (crtl->preferred_stack_boundary < PREFERRED_STACK_BOUNDARY)
> +    crtl->preferred_stack_boundary = PREFERRED_STACK_BOUNDARY;

2. What is the effect of OPTAB_LIB_WIDEN below?  I've copied it
from the existing "plus" without actually understanding it.

> +  target = expand_binop (Pmode, add_optab, target,
> +                        gen_int_mode (offset, Pmode),
> +                        NULL_RTX, 1, OPTAB_LIB_WIDEN);

I'll send an updated patch tomorrow.  Thanks for your comments and
your help.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany