From patchwork Tue May 31 14:15:11 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrill Tkachov X-Patchwork-Id: 628254 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3rJwW96lqzz9sds for ; Wed, 1 Jun 2016 00:15:33 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=tXEHqHOG; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding; q=dns; s= default; b=M81BO7rrpZOQMUxb8Nh8iajcnJ+WHwbeaXYKbqrAeTdinYpP0hjLF 0YPt+PQtKDq1E9GdKxz44P+QdZBQe6aaW2L7L48aN8GceVasZVfhnUFG/LAHxUo0 K2oeTVGcmf7k7uYRzfM7Wjm+QEJW1fod60sb1kXtwNaLX+ozSgy9kM= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding; s=default; bh=N26p61Tv7PDxctE+kFVXyTURZ3U=; b=tXEHqHOGdhr3DiCCt0Om+IBRP8/F YwEIA1qZFunf3IYecxz3FngmzYNJVtfWecubdL99AN2iEsKzxpSaEC/yxyLIt2Oa XHLHl9h6td7E074XuygDjYwLwFYSIyaiY7mcBUZqN9LS8AYPI3VyQnjKIS1U0dOp Ajo4bNuhQNeqgsg= Received: (qmail 72807 invoked by alias); 31 May 2016 14:15:26 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 72793 invoked by uid 89); 31 May 2016 14:15:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=stm, STM, POP, accurate 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; Tue, 31 May 2016 14:15:15 +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 2E6A0F; Tue, 31 May 2016 07:15:42 -0700 (PDT) Received: from [10.2.206.43] (e100706-lin.cambridge.arm.com [10.2.206.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C0DDB3F218; Tue, 31 May 2016 07:15:12 -0700 (PDT) Message-ID: <574D9C6F.4020306@foss.arm.com> Date: Tue, 31 May 2016 15:15:11 +0100 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Jiong Wang , GCC Patches CC: Wilco Dijkstra , Ramana Radhakrishnan Subject: Re: [Patch, ARM] PR71061, length pop* pattern in epilogue correctly References: <5735DCAD.8050808@foss.arm.com> <573D7707.5090608@foss.arm.com> In-Reply-To: <573D7707.5090608@foss.arm.com> Hi Jiong, On 19/05/16 09:19, Jiong Wang wrote: > > > On 13/05/16 14:54, Jiong Wang wrote: >> For thumb mode, this is causing wrong size calculation and may affect >> some rtl pass, for example bb-order where copy_bb_p needs accurate insn >> length info. >> >> This have eventually part of the reason for >> https://gcc.gnu.org/ml/gcc-patches/2016-05/msg00639.html where bb-order >> failed to do the bb copy. >> >> For the fix, I think we should extend arm_attr_length_push_multi to pop* >> pattern. >> >> OK for trunk? >> >> 2016-05-13 Jiong. Wang >> >> gcc/ >> PR target/71061 >> * config/arm/arm-protos.h (arm_attr_length_push_multi): Rename to >> "arm_attr_length_pp_multi". Add one parameter "first_index". >> * config/arm/arm.md (*push_multi): Use new function. >> (*load_multiple_with_writeback): Set "length" attribute. >> (*pop_multiple_with_writeback_and_return): Likewise. >> (*pop_multiple_with_return): Likewise. >> > > Wilco pointed out offline the new length function is wrong as for pop we > should check PC_REG instead of LR_REG, meanwhile these pop patterns may > allow ldm thus base register needs to be checked also. > > I updated this patch to address these issues. > > OK for trunk ? > I agree with the idea of the patch, but I have some comments inline. > gcc/ > > 2016-05-19 Jiong Wang > > PR target/71061 > * config/arm/arm-protos.h (arm_attr_length_pop_multi): New declaration. > * config/arm/arm.c (arm_attr_length_pop_multi): New function to return > length for pop patterns. > * config/arm/arm.md (*load_multiple_with_writeback): Set "length" attribute. > (*pop_multiple_with_writeback_and_return): Likewise. > (*pop_multiple_with_return): Likewise. > > + + /* Check base register for LDM. */ + if (ldm_p && REGNO_REG_CLASS (regno) == HI_REGS) + return 4; + + /* Check each register in the list. */ + for (; indx >= first_indx; indx--) + { + regno = REGNO (XEXP (XVECEXP (parallel_op, 0, indx), 0)); + if (REGNO_REG_CLASS (regno) == HI_REGS + && (regno != PC_REGNUM || ldm_p)) + return 4; + } + + return 2; +} + diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h index d8179c4..5dd3e0d 100644 --- a/gcc/config/arm/arm-protos.h +++ b/gcc/config/arm/arm-protos.h @@ -163,6 +163,7 @@ extern const char *arm_output_iwmmxt_shift_immediate (const char *, rtx *, bool) extern const char *arm_output_iwmmxt_tinsr (rtx *); extern unsigned int arm_sync_loop_insns (rtx , rtx *); extern int arm_attr_length_push_multi(rtx, rtx); +extern int arm_attr_length_pop_multi(rtx *, bool, bool); extern void arm_expand_compare_and_swap (rtx op[]); extern void arm_split_compare_and_swap (rtx op[]); extern void arm_split_atomic_op (enum rtx_code, rtx, rtx, rtx, rtx, rtx, rtx); diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index c3f74dc..4d19d3a 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -27812,6 +27812,65 @@ arm_attr_length_push_multi(rtx parallel_op, rtx first_op) return 4; } +/* Compute the atrribute "length" of insn. Currently, this function is used + for "*load_multiple_with_writeback", "*pop_multiple_with_return" and + "*pop_multiple_with_writeback_and_return". */ + s/atrribute/attribute/ Also, please add a description of the function arguments to the function comment. +int +arm_attr_length_pop_multi(rtx *operands, bool return_pc, bool write_back_p) +{ space between function name and '('. + /* ARM mode. */ + if (TARGET_ARM) + return 4; + /* Thumb1 mode. */ + if (TARGET_THUMB1) + return 2; + + /* For POP/PUSH/LDM/STM under Thumb2 mode, we can use 16-bit Thumb encodings + if the register list is 8-bit. Normally this means all registers in the + list must be LO_REGS, that is (R0 -R7). If any HI_REGS used, then we must + use 32-bit encodings. But there are two exceptions to this rule where + special HI_REGS used in PUSH/POP. + + 1. 16-bit Thumb encoding POP can use an 8-bit register list, and an + additional bit, P, that corresponds to the PC. This means it can access + any of {PC, R7-R0}. It took me a bit to figure it out but this bit 'P' you're talking about is a just a bit in the illustration in the ARM Architecture Reference Manual (section A8.8.131). I don't think it's useful to refer to it. This would be better phrased as "The encoding is 16 bits wide if the register list contains only the registers in {R0 - R7} or the PC". + 2. 16-bit Thumb encoding PUSH can use an 8-bit register list, and an + additional bit, M , that corresponds to the LR. This means it can + access any of {LR, R7-R0}. */ + This function only deals with POP instructions, so talking about PUSH encodings is confusing. I suggest you drop point 2). + rtx parallel_op = operands[0]; + /* Initialize to elements number of PARALLEL. */ + unsigned indx = XVECLEN (parallel_op, 0) - 1; + /* Initialize the value to base register. */ + unsigned regno = REGNO (operands[1]); + /* Skip return and write back pattern. + We only need register pop pattern for later analysis. */ + unsigned first_indx = 0; + first_indx += return_pc ? 1 : 0; + first_indx += write_back_p ? 1 : 0; + + /* A pop operation can be done through LDM or POP. If the base register is SP + and if it's with write back, then a LDM will be alias of POP. */ + bool pop_p = (regno == SP_REGNUM && write_back_p); + bool ldm_p = !pop_p;