From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 49667 invoked by alias); 25 Nov 2015 13:31:43 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 49655 invoked by uid 89); 25 Nov 2015 13:31:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.7 required=5.0 tests=AWL,BAYES_50,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 25 Nov 2015 13:31:41 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 2DC3C3B712; Wed, 25 Nov 2015 13:31:40 +0000 (UTC) Received: from localhost.localdomain (vpn1-4-132.ams2.redhat.com [10.36.4.132]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tAPDVcj5031068; Wed, 25 Nov 2015 08:31:38 -0500 Subject: Re: [PATCH] Allocate constant size dynamic stack space in the prologue To: gcc-patches@gcc.gnu.org, Andreas Krebbel , Ulrich Weigand , Andreas Arnez References: <20151125125610.GA19687@linux.vnet.ibm.com> From: Bernd Schmidt Message-ID: <5655B83A.4020406@redhat.com> Date: Wed, 25 Nov 2015 13:33:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151125125610.GA19687@linux.vnet.ibm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg03058.txt.bz2 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