[PATCH] Allocate constant size dynamic stack space in the prologue
Bernd Schmidt
bschmidt@redhat.com
Wed Nov 25 13:33:00 GMT 2015
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
More information about the Gcc-patches
mailing list