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
