From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26633 invoked by alias); 10 Nov 2011 14:25:47 -0000 Received: (qmail 26618 invoked by uid 22791); 10 Nov 2011 14:25:45 -0000 X-SWARE-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from service87.mimecast.com (HELO service87.mimecast.com) (91.220.42.44) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 10 Nov 2011 14:25:32 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Thu, 10 Nov 2011 14:25:29 +0000 Received: from [10.1.79.40] ([10.1.255.212]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.0); Thu, 10 Nov 2011 14:25:27 +0000 Subject: Re: [RFA/ARM][Patch 01/02]: Thumb2 epilogue in RTL From: Sameera Deshpande To: Richard Earnshaw Cc: "gcc-patches@gcc.gnu.org" , "nickc@redhat.com" , "paul@codesourcery.com" , Ramana Radhakrishnan In-Reply-To: <4EBBD52A.6040707@arm.com> References: <003301cc7df9$d80c5f80$88251e80$@deshpande@arm.com> <4EBBD52A.6040707@arm.com> Date: Thu, 10 Nov 2011 15:28:00 -0000 Message-ID: <1320935126.17411.10.camel@e102549-lin.cambridge.arm.com> Mime-Version: 1.0 X-MC-Unique: 111111014252901501 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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 X-SW-Source: 2011-11/txt/msg01418.txt.bz2 Hi Richard, thanks for your comments. --=20 > + if (GET_CODE (SET_SRC (elt =3D XVECEXP (op, 0, offset_adj))) =3D=3D PL= US) >=20 > It's generally best not to use assignments within conditionals unless > there is a strong reason otherwise (that normally implies something like > being deep within a condition test where you only want to update the > variable if some pre-conditions are true and that can't be easily > factored out). >=20 > + !=3D (unsigned int) (first_dest_regno + regs_per_val * > (i - base)))) >=20 > Line length (split the line just before the '+' operator. >=20 > + /* now show EVERY reg that will be restored, using a SET for each. */ >=20 > Capital letter at start of sentence. Why is EVERY in caps? >=20 > + saved_regs_mask =3D offsets->saved_regs_mask; > + for (i =3D 0, num_regs =3D 0; i <=3D LAST_ARM_REGNUM; i++) >=20 > blank line before the for loop. >=20 > + /* It's illegal to do a pop for only one reg, so generate an ldr. = */ >=20 > GCC coding standards suggest avoiding the use of 'illegal'. Suggest > changing that to 'Pop can only be used for more than one reg; so...' >=20 > + reg_names[REGNO (XEXP (XVECEXP (operands[0], 0, 2), > 0))]); > + > + /* Skip over the first two elements and the one we just generated. > */ > + for (i =3D 3; i < (num_saves); i++) > + { > + strcat (pattern, \", %|\"); >=20 > + strcat (pattern, >=20 > + reg_names[REGNO (XEXP (XVECEXP (operands[0], 0, i), > 0))]); > + } > + > + strcat (pattern, \"}\"); > + output_asm_insn (pattern, operands); > + >=20 > + return \"\"; > + } > + " >=20 > + [(set_attr "type" "load4")] >=20 > There's a lot of trailing white space here. Please remove. Removed white spaces in reworked patch http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01009.html >=20 > +(define_insn "*thumb2_ldr_with_return" > + [(return) > + (set (reg:SI PC_REGNUM) > + (mem:SI (post_inc:SI (match_operand:SI 0 "s_register_operand" > "k"))))] > + "TARGET_THUMB2" > + "ldr%?\t%|pc, [%0], #4" > + [(set_attr "type" "load1") > + (set_attr "predicable" "yes")] > +) > + >=20 > This pattern doesn't seem to be used. What's its purpose? This pattern is generated from thumb2_expand_return in=20 + if (num_regs =3D=3D 1) + { + rtx par =3D gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (2)); + rtx reg =3D gen_rtx_REG (SImode, PC_REGNUM); + rtx addr =3D gen_rtx_MEM (SImode, + gen_rtx_POST_INC (SImode, + stack_pointer_rtx)); + set_mem_alias_set (addr, get_frame_alias_set ()); + XVECEXP (par, 0, 0) =3D ret_rtx; + XVECEXP (par, 0, 1) =3D gen_rtx_SET (SImode, reg, addr); + RTX_FRAME_RELATED_P (par) =3D 1; + emit_jump_insn (par); + } >=20 > + static const struct { const char *const name; } table[] > + =3D { {\"d0\"}, {\"d1\"}, {\"d2\"}, {\"d3\"}, >=20 > I'm not keen on having this table. Generally the register names should > be configurable depending on the assembler flavour and this patch > defeats that. Is there any way to rewrite this code so that it can use > the standard operand methods for generating register names? The updated patch was resent after comments from Ramana and Paul which eliminates this table. http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01009.html I will take care of other formatting issues and will resend the patch. >=20 > In summary, this is mostly OK, apart from the last two items. >=20 > R. - Thanks and regards, Sameera D.