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
