From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 108357 invoked by alias); 20 Jul 2016 15:09:44 -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 108340 invoked by uid 89); 20 Jul 2016 15:09:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 20 Jul 2016 15:09:31 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B8192318 for ; Wed, 20 Jul 2016 08:10:39 -0700 (PDT) Received: from e105689-lin.cambridge.arm.com (e105689-lin.cambridge.arm.com [10.2.207.32]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7B04F3F387; Wed, 20 Jul 2016 08:09:28 -0700 (PDT) Subject: Re: [AArch64][3/3] Migrate aarch64_expand_prologue/epilogue to aarch64_add_constant To: Jiong Wang References: <626096f0-957f-98bd-5efa-faa0c14eb5ab@foss.arm.com> Cc: GCC Patches From: "Richard Earnshaw (lists)" Message-ID: Date: Wed, 20 Jul 2016 15:09:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-SW-Source: 2016-07/txt/msg01273.txt.bz2 On 20/07/16 16:02, Jiong Wang wrote: > On 20/07/16 15:18, Richard Earnshaw (lists) wrote: >> On 20/07/16 14:03, Jiong Wang wrote: >>> Those stack adjustment sequences inside aarch64_expand_prologue/epilogue >>> are doing exactly what's aarch64_add_constant offered, except they also >>> need to be aware of dwarf generation. >>> >>> This patch teach existed aarch64_add_constant about dwarf generation and >>> currently SP register is supported. Whenever SP is updated, there >>> should be CFA update, we then mark these instructions as frame related, >>> and if the update is too complex for gcc to guess the adjustment, we >>> attach explicit annotation. >>> >>> Both dwarf frame info size and pro/epilogue scheduling are improved >>> after >>> this patch as aarch64_add_constant has better utilization of scratch >>> register. >>> >>> OK for trunk? >>> >>> gcc/ >>> 2016-07-20 Jiong Wang >>> >>> * config/aarch64/aarch64.c (aarch64_add_constant): Mark >>> instruction as frame related when it is. Generate CFA >>> annotation when it's necessary. >>> (aarch64_expand_prologue): Use aarch64_add_constant. >>> (aarch64_expand_epilogue): Likewise. >>> >> Are you sure using aarch64_add_constant is unconditionally safe? Stack >> adjustments need to be done very carefully to ensure that we never >> transiently deallocate part of the stack. > > Richard, > > Thanks for the review, yes, I believe using aarch64_add_constant is > unconditionally > safe here. Because we have generated a stack tie to clobber the whole > memory thus > prevent any instruction which access stack be scheduled after that. > > The access to deallocated stack issue was there and fixed by > > https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02292.html. > > aarch64_add_constant itself is generating the same instruction > sequences as the > original code, except for a few cases, it will prefer > > move scratch_reg, #imm > add sp, sp, scratch_reg > > than: > add sp, sp, #imm_part1 > add sp, sp, #imm_part2 > > > > But can you guarantee we will never get and add and a sub in a single adjustment? If not, then we need to ensure the two instructions appear in the right order. R.