From patchwork Wed Jun 28 20:21:49 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Aaron Sawdey X-Patchwork-Id: 781907 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 3wyZ3D2wZlz9s65 for ; Thu, 29 Jun 2017 06:22:24 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="lXgZKvR+"; 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 :subject:from:to:cc:date:in-reply-to:references:content-type :mime-version:message-id; q=dns; s=default; b=WF3myLkQjJjXrW+pbJ DkrIIkDwfu78Dn+H31dKsjk8AU3BqEE8q33tRZvBn+yQnCFN3WhVSGJc2oH6REer GjFr2WKd/6QtPIX3rL7r65ym8LEq6Y3mSZAZzroK2v7vxugvqCUyEFcjnnhv89ux 8GaOT9APanv4UpeiifZkzU4kg= 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 :subject:from:to:cc:date:in-reply-to:references:content-type :mime-version:message-id; s=default; bh=5Hx/W8UGFaOZUNsLI8o1kEH3 9kI=; b=lXgZKvR+N18x1DadTG5wns7+2MJxVLNK1M2gsi7/f0sbRLKHWmllVwAR ltJi+PCglZv6Q248pR8dx2J9hcTVsdkOhwogIlW59v8LXMH+72LJibBp8uZ3DaNn Az+WbiziwHz+lbTjTTxlkkb8RWNJcK0bib6Ilx40L3MLBPvff50= Received: (qmail 32718 invoked by alias); 28 Jun 2017 20:22:08 -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 29846 invoked by uid 89); 28 Jun 2017 20:22:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.8 required=5.0 tests=BAYES_00, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy= X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0a-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.156.1) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 28 Jun 2017 20:22:03 +0000 Received: from pps.filterd (m0098404.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v5SKJTOP109238 for ; Wed, 28 Jun 2017 16:22:00 -0400 Received: from e31.co.us.ibm.com (e31.co.us.ibm.com [32.97.110.149]) by mx0a-001b2d01.pphosted.com with ESMTP id 2bc7a4b871-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 28 Jun 2017 16:22:00 -0400 Received: from localhost by e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 28 Jun 2017 14:21:59 -0600 Received: from b03cxnp08025.gho.boulder.ibm.com (9.17.130.17) by e31.co.us.ibm.com (192.168.1.131) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 28 Jun 2017 14:21:56 -0600 Received: from b03ledav004.gho.boulder.ibm.com (b03ledav004.gho.boulder.ibm.com [9.17.130.235]) by b03cxnp08025.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v5SKLuJh57802882; Wed, 28 Jun 2017 13:21:56 -0700 Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 50CBA7804D; Wed, 28 Jun 2017 14:21:56 -0600 (MDT) Received: from ragesh3a (unknown [9.85.176.254]) by b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP id 64CE37803F; Wed, 28 Jun 2017 14:21:55 -0600 (MDT) Subject: Re: [PATCH rs6000] remove implicit static var outputs of toc_relative_expr_p From: Aaron Sawdey To: Segher Boessenkool Cc: gcc-patches@gcc.gnu.org, David Edelsohn , Bill Schmidt Date: Wed, 28 Jun 2017 15:21:49 -0500 In-Reply-To: <20170627233531.GE16550@gate.crashing.org> References: <1498581837.6219.12.camel@linux.vnet.ibm.com> <20170627233531.GE16550@gate.crashing.org> Mime-Version: 1.0 X-TM-AS-GCONF: 00 x-cbid: 17062820-8235-0000-0000-00000BD15EAD X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007291; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000214; SDB=6.00880055; UDB=6.00438681; IPR=6.00660210; BA=6.00005445; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00015994; XFM=3.00000015; UTC=2017-06-28 20:21:58 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17062820-8236-0000-0000-00003C805E7A Message-Id: <1498681309.6219.15.camel@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-06-28_12:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1706280324 X-IsSubscribed: yes Hi Segher, On Tue, 2017-06-27 at 18:35 -0500, Segher Boessenkool wrote: > Hi Aaron, > > On Tue, Jun 27, 2017 at 11:43:57AM -0500, Aaron Sawdey wrote: > > The function toc_relative_expr_p implicitly sets two static vars > > (tocrel_base and tocrel_offset) that are declared in rs6000.c. The > > real > > purpose of this is to communicate between > > print_operand/print_operand_address and > > rs6000_output_addr_const_extra, > > which is called through the asm_out hook vector by something in the > > call tree under output_addr_const. > > > > This patch changes toc_relative_expr_p to make tocrel_base and > > tocrel_offset be explicit const_rtx * args. All of the calls other > > than > > print_operand/print_operand_address are changed to have local > > const_rtx > > vars that are passed in. > > If those locals aren't used, can you arrange to call > toc_relative_expr_p > with NULL instead?  Or are they always used? There are calls where neither is used or only tocrel_base is used. > > > The statics in rs6000.c are now called > > tocrel_base_oac and tocrel_offset_oac to reflect their use to > > communicate across output_addr_const, and that is now the only > > thing > > they are used for. > > Can't say I like those names, very cryptical.  Not that I know > something > better, the short names as they were weren't very nice either. > > > --- gcc/config/rs6000/rs6000.c (revision 249639) > > +++ gcc/config/rs6000/rs6000.c (working copy) > > @@ -8628,18 +8628,25 @@ > >     && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant > > (base), Pmode)); > >  } > >   > > -static const_rtx tocrel_base, tocrel_offset; > > +/* These are only used to pass through from > > print_operand/print_operand_address > > + * to rs6000_output_addr_const_extra over the intervening > > function  > > + * output_addr_const which is not target code.  */ > > No leading * in a block comment please.  (And you have a trailing > space). > > > +static const_rtx tocrel_base_oac, tocrel_offset_oac; > >   > >  /* Return true if OP is a toc pointer relative address (the output > >     of create_TOC_reference).  If STRICT, do not match non-split > > -   -mcmodel=large/medium toc pointer relative addresses.  */ > > +   -mcmodel=large/medium toc pointer relative addresses.  Places > > base  > > +   and offset pieces in TOCREL_BASE and TOCREL_OFFSET > > respectively.  */ > > s/Places/Place/ (and another trailing space). > > > -  tocrel_base = op; > > -  tocrel_offset = const0_rtx; > > +  *tocrel_base = op; > > +  *tocrel_offset = const0_rtx; > >    if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1), > > GET_MODE (op))) > >      { > > -      tocrel_base = XEXP (op, 0); > > -      tocrel_offset = XEXP (op, 1); > > +      *tocrel_base = XEXP (op, 0); > > +      *tocrel_offset = XEXP (op, 1); > >      } > > Maybe write this as > >   if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1), > GET_MODE (op))) >     { >       *tocrel_base = XEXP (op, 0); >       *tocrel_offset = XEXP (op, 1); >     } >   else >     { >       *tocrel_base = op; >       *tocrel_offset = const0_rtx; >     } > > or, if you allow NULL pointers, > >   bool with_offset = GET_CODE (op) == PLUS >      && add_cint_operand (XEXP (op, 1), GET_MODE (op)); >   if (tocrel_base) >     *tocrel_base = with_offset ? XEXP (op, 0) : op; >   if (tocrel_offset) >     *tocrel_offset = with_offset ? XEXP (op, 1) : const0_rtx; > > or such. > > > -  return (GET_CODE (tocrel_base) == UNSPEC > > -   && XINT (tocrel_base, 1) == UNSPEC_TOCREL); > > +  return (GET_CODE (*tocrel_base) == UNSPEC > > +   && XINT (*tocrel_base, 1) == UNSPEC_TOCREL); > > Well, and then you have this, so you need to assign tocrel_base to a > local > as well. > > >  legitimate_constant_pool_address_p (const_rtx x, machine_mode > > mode, > >       bool strict) > >  { > > -  return (toc_relative_expr_p (x, strict) > > +  const_rtx tocrel_base, tocrel_offset; > > +  return (toc_relative_expr_p (x, strict, &tocrel_base, > > &tocrel_offset) > > For example here it seems nothing uses tocrel_base? > > It is probably nicer to have a separate function for > toc_relative_expr_p > and one to pull the base/offset out.  And maybe don't keep it cached > for > the output function either?  It has all info it needs, right, the > full > address RTX?  I don't think it is measurably slower to pull the > address > apart an extra time? I think it doesn't make a lot of sense to have two functions as you have to do nearly all the work just to get the true/false return value, you have to completely compute tocrel_base. I've restructured this so that either pointer can be null. The function uses locals for tocrel_base/tocrel_offset, then assigns to the pointers if non-null. bool toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,      const_rtx *tocrel_offset_ret) {   if (!TARGET_TOC)     return false;   if (TARGET_CMODEL != CMODEL_SMALL)     {       /* When strict ensure we have everything tidy.  */       if (strict   && !(GET_CODE (op) == LO_SUM        && REG_P (XEXP (op, 0))        && INT_REG_OK_FOR_BASE_P (XEXP (op, 0), strict))) return false;       /* When not strict, allow non-split TOC addresses and also allow (lo_sum (high ..)) TOC addresses created during reload.  */       if (GET_CODE (op) == LO_SUM) op = XEXP (op, 1);     }   const_rtx tocrel_base = op;   const_rtx tocrel_offset = const0_rtx;   if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1), GET_MODE (op)))     {       tocrel_base = XEXP (op, 0);       if (tocrel_offset_ret) tocrel_offset = XEXP (op, 1);     }   if (tocrel_base_ret)     *tocrel_base_ret = tocrel_base;   if (tocrel_offset_ret)     *tocrel_offset_ret = tocrel_offset;   return (GET_CODE (tocrel_base) == UNSPEC   && XINT (tocrel_base, 1) == UNSPEC_TOCREL); } Revised patch is attached, bootstrap/regtest in progress. If everything passes, ok for trunk? Thanks,     Aaron Index: gcc/config/rs6000/rs6000-protos.h =================================================================== --- gcc/config/rs6000/rs6000-protos.h (revision 249639) +++ gcc/config/rs6000/rs6000-protos.h (working copy) @@ -40,7 +40,7 @@ extern int small_data_operand (rtx, machine_mode); extern bool mem_operand_gpr (rtx, machine_mode); extern bool mem_operand_ds_form (rtx, machine_mode); -extern bool toc_relative_expr_p (const_rtx, bool); +extern bool toc_relative_expr_p (const_rtx, bool, const_rtx *, const_rtx *); extern void validate_condition_mode (enum rtx_code, machine_mode); extern bool legitimate_constant_pool_address_p (const_rtx, machine_mode, bool); Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 249639) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -8628,14 +8628,19 @@ && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (base), Pmode)); } -static const_rtx tocrel_base, tocrel_offset; +/* These are only used to pass through from print_operand/print_operand_address + to rs6000_output_addr_const_extra over the intervening function + output_addr_const which is not target code. */ +static const_rtx tocrel_base_oac, tocrel_offset_oac; /* Return true if OP is a toc pointer relative address (the output of create_TOC_reference). If STRICT, do not match non-split - -mcmodel=large/medium toc pointer relative addresses. */ + -mcmodel=large/medium toc pointer relative addresses. Place base + and offset pieces in TOCREL_BASE and TOCREL_OFFSET respectively. */ bool -toc_relative_expr_p (const_rtx op, bool strict) +toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret, + const_rtx *tocrel_offset_ret) { if (!TARGET_TOC) return false; @@ -8655,14 +8660,21 @@ op = XEXP (op, 1); } - tocrel_base = op; - tocrel_offset = const0_rtx; + const_rtx tocrel_base = op; + const_rtx tocrel_offset = const0_rtx; + if (GET_CODE (op) == PLUS && add_cint_operand (XEXP (op, 1), GET_MODE (op))) { tocrel_base = XEXP (op, 0); - tocrel_offset = XEXP (op, 1); + if (tocrel_offset_ret) + tocrel_offset = XEXP (op, 1); } + if (tocrel_base_ret) + *tocrel_base_ret = tocrel_base; + if (tocrel_offset_ret) + *tocrel_offset_ret = tocrel_offset; + return (GET_CODE (tocrel_base) == UNSPEC && XINT (tocrel_base, 1) == UNSPEC_TOCREL); } @@ -8674,7 +8686,8 @@ legitimate_constant_pool_address_p (const_rtx x, machine_mode mode, bool strict) { - return (toc_relative_expr_p (x, strict) + const_rtx tocrel_base, tocrel_offset; + return (toc_relative_expr_p (x, strict, &tocrel_base, &tocrel_offset) && (TARGET_CMODEL != CMODEL_MEDIUM || constant_pool_expr_p (XVECEXP (tocrel_base, 0, 0)) || mode == QImode @@ -11069,7 +11082,7 @@ > (TARGET_CMODEL != CMODEL_SMALL ? 3 : 2))) || (GET_CODE (operands[0]) == REG && FP_REGNO_P (REGNO (operands[0])))) - && !toc_relative_expr_p (operands[1], false) + && !toc_relative_expr_p (operands[1], false, NULL, NULL) && (TARGET_CMODEL == CMODEL_SMALL || can_create_pseudo_p () || (REG_P (operands[0]) @@ -21812,7 +21825,7 @@ } else { - if (toc_relative_expr_p (x, false)) + if (toc_relative_expr_p (x, false, &tocrel_base_oac, &tocrel_offset_oac)) /* This hack along with a corresponding hack in rs6000_output_addr_const_extra arranges to output addends where the assembler expects to find them. eg. @@ -21819,7 +21832,7 @@ (plus (unspec [(symbol_ref ("x")) (reg 2)] tocrel) 4) without this hack would be output as "x@toc+4". We want "x+4@toc". */ - output_addr_const (file, CONST_CAST_RTX (tocrel_base)); + output_addr_const (file, CONST_CAST_RTX (tocrel_base_oac)); else output_addr_const (file, x); } @@ -21886,7 +21899,7 @@ fprintf (file, "@l(%s)", reg_names[ REGNO (XEXP (x, 0)) ]); } #endif - else if (toc_relative_expr_p (x, false)) + else if (toc_relative_expr_p (x, false, &tocrel_base_oac, &tocrel_offset_oac)) { /* This hack along with a corresponding hack in rs6000_output_addr_const_extra arranges to output addends @@ -21895,17 +21908,17 @@ . (plus (unspec [(symbol_ref ("x")) (reg 2)] tocrel) 8)) without this hack would be output as "x@toc+8@l(9)". We want "x+8@toc@l(9)". */ - output_addr_const (file, CONST_CAST_RTX (tocrel_base)); + output_addr_const (file, CONST_CAST_RTX (tocrel_base_oac)); if (GET_CODE (x) == LO_SUM) fprintf (file, "@l(%s)", reg_names[REGNO (XEXP (x, 0))]); else - fprintf (file, "(%s)", reg_names[REGNO (XVECEXP (tocrel_base, 0, 1))]); + fprintf (file, "(%s)", reg_names[REGNO (XVECEXP (tocrel_base_oac, 0, 1))]); } else gcc_unreachable (); } -/* Implement TARGET_OUTPUT_ADDR_CONST_EXTRA. */ +/* Implement TARGET_ASM_OUTPUT_ADDR_CONST_EXTRA. */ static bool rs6000_output_addr_const_extra (FILE *file, rtx x) @@ -21918,11 +21931,11 @@ && REG_P (XVECEXP (x, 0, 1)) && REGNO (XVECEXP (x, 0, 1)) == TOC_REGISTER); output_addr_const (file, XVECEXP (x, 0, 0)); - if (x == tocrel_base && tocrel_offset != const0_rtx) + if (x == tocrel_base_oac && tocrel_offset_oac != const0_rtx) { - if (INTVAL (tocrel_offset) >= 0) + if (INTVAL (tocrel_offset_oac) >= 0) fprintf (file, "+"); - output_addr_const (file, CONST_CAST_RTX (tocrel_offset)); + output_addr_const (file, CONST_CAST_RTX (tocrel_offset_oac)); } if (!TARGET_AIX || (TARGET_ELF && TARGET_MINIMAL_TOC)) { @@ -39312,6 +39325,8 @@ if (!insn_entry[uid].is_swap || insn_entry[uid].is_load) return false; + const_rtx tocrel_base; + /* Find the unique use in the swap and locate its def. If the def isn't unique, punt. */ struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn); @@ -39357,7 +39372,7 @@ rtx tocrel_expr = SET_SRC (tocrel_body); if (GET_CODE (tocrel_expr) == MEM) tocrel_expr = XEXP (tocrel_expr, 0); - if (!toc_relative_expr_p (tocrel_expr, false)) + if (!toc_relative_expr_p (tocrel_expr, false, &tocrel_base, NULL)) return false; split_const (XVECEXP (tocrel_base, 0, 0), &base, &offset); if (GET_CODE (base) != SYMBOL_REF || !CONSTANT_POOL_ADDRESS_P (base)) @@ -40118,11 +40133,12 @@ to set tocrel_base; otherwise it would be unnecessary as we've already established it will return true. */ rtx base, offset; + const_rtx tocrel_base; rtx tocrel_expr = SET_SRC (PATTERN (tocrel_insn)); /* There is an extra level of indirection for small/large code models. */ if (GET_CODE (tocrel_expr) == MEM) tocrel_expr = XEXP (tocrel_expr, 0); - if (!toc_relative_expr_p (tocrel_expr, false)) + if (!toc_relative_expr_p (tocrel_expr, false, &tocrel_base, NULL)) gcc_unreachable (); split_const (XVECEXP (tocrel_base, 0, 0), &base, &offset); rtx const_vector = get_pool_constant (base);