From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id A272E385E008 for ; Wed, 25 Mar 2020 16:10:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A272E385E008 Received: from pps.filterd (m0098404.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 02PG5n4W100276; Wed, 25 Mar 2020 12:10:04 -0400 Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 2ywf0qch4m-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 25 Mar 2020 12:09:59 -0400 Received: from m0098404.ppops.net (m0098404.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 02PG6PJZ103931; Wed, 25 Mar 2020 12:09:52 -0400 Received: from ppma03wdc.us.ibm.com (ba.79.3fa9.ip4.static.sl-reverse.com [169.63.121.186]) by mx0a-001b2d01.pphosted.com with ESMTP id 2ywf0qcfqw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 25 Mar 2020 12:09:51 -0400 Received: from pps.filterd (ppma03wdc.us.ibm.com [127.0.0.1]) by ppma03wdc.us.ibm.com (8.16.0.27/8.16.0.27) with SMTP id 02PG5MCE032597; Wed, 25 Mar 2020 16:07:07 GMT Received: from b03cxnp07028.gho.boulder.ibm.com (b03cxnp07028.gho.boulder.ibm.com [9.17.130.15]) by ppma03wdc.us.ibm.com with ESMTP id 2ywawa2hat-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 25 Mar 2020 16:07:07 +0000 Received: from b03ledav001.gho.boulder.ibm.com (b03ledav001.gho.boulder.ibm.com [9.17.130.232]) by b03cxnp07028.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 02PG76Kx23462264 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 25 Mar 2020 16:07:06 GMT Received: from b03ledav001.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A42916E04E; Wed, 25 Mar 2020 16:07:06 +0000 (GMT) Received: from b03ledav001.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id AF7AA6E052; Wed, 25 Mar 2020 16:07:05 +0000 (GMT) Received: from lexx (unknown [9.160.6.102]) by b03ledav001.gho.boulder.ibm.com (Postfix) with ESMTP; Wed, 25 Mar 2020 16:07:05 +0000 (GMT) Message-ID: <4844b04a2480609207ef41f48cbe3b363c4ed627.camel@vnet.ibm.com> Subject: Re: [PATCH], Set -mpcrel by default on Linux 64-bit systems for -mcpu=future From: will schmidt To: Michael Meissner , gcc-patches@gcc.gnu.org, Segher Boessenkool , David Edelsohn Date: Wed, 25 Mar 2020 11:07:04 -0500 In-Reply-To: <20200323203838.GA32034@ibm-toto.the-meissners.org> References: <20200323203838.GA32034@ibm-toto.the-meissners.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 (3.28.5-5.el7) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138, 18.0.645 definitions=2020-03-25_09:2020-03-24, 2020-03-25 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 phishscore=0 impostorscore=0 malwarescore=0 mlxlogscore=999 spamscore=0 clxscore=1015 lowpriorityscore=0 adultscore=0 suspectscore=0 bulkscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003250128 X-Spam-Status: No, score=-18.4 required=5.0 tests=BAYES_00, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Mar 2020 16:10:08 -0000 Hi, Comments inline. (Taking a pass with focus on cosmetic stuff. This is intended to help Segher focus on the harder parts :-) ). On Mon, 2020-03-23 at 16:38 -0400, Michael Meissner via Gcc-patches wrote: Subject: [Patch v327] set -mcprel by default ... Include some powerpc or rs6000 reference in the subject. My filters missed this one. (I'm assuming this is powerpc target specific). > This is a revision of a patch that I've submitted several times. It makes > -mpcrel the default on Linux 64-bit systems that use ELF v2, use the medium > code mode, and if the user did not disable prefixed load/store instructions for > -mcpu=future. The fewer words, the easier to review. History is important but so is making the review easy. "This patch makes -mpcrel the default on Linux 64-bit systems that use ELF V2, medium code model, and have not disabled prefix instructions." > > Previous versions of the patch had two macros that the target os could set, one > to say that the target os supported prefixed instructions with large numeric > offsets, and the second whether PC-relative prefixed load/store instructions > could be generated. Segher asked that we drop the capability to configure > whether prefixed numeric load/store instructions would be enabled by default, > and this patch does this. All of the PC-relative support is in the master > branch, we just need to enable it by default. [v327] dropped macros to indicate if target supports prefixed instructions pre previous feedback. > > Is this patch acceptable to be committed to the master branch. I have done > various tests with this patch, including most recently bootstraping and running > make check. I have built the Spec 2017 benchmark suite with this patch. > > 2020-03-23 Michael Meissner > > * config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): Enable -mpcrel > for -mcpu=future by default on 64-bit systems with medium code > model. > * config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Do not > define the bits for -mprefixed or -mpcrel here. The change to ISA_FUTURE_MASKS_SERVER only drops OPTION_MASK_PREFIXED. No change to PCREL there. > (OTHER_FUTURE_MASKS): Define the bits for -mprefixed and -mpcrel > here. No touches to OTHER_FUTURE_MASKS below. (accidental patch ommission or missed a changelog update?) > * config/rs6000/rs6000.c (PCREL_SUPPORTED_BY_OS): If not defined, > don't enable -mpcrel by default. I suggest s/If not defined// > (rs6000_option_override_internal): Enable -mpcrel on systems that > support it, if the user did not do -mno-pcrel. I suggest s/if the user ...// > > --- /tmp/QuuFm5_linux64.h 2020-03-20 20:15:38.321862650 -0400 > +++ gcc/config/rs6000/linux64.h 2020-03-20 18:36:33.654484833 -0400 > @@ -640,3 +640,11 @@ extern int dot_symbols; > enabling the __float128 keyword. */ > #undef TARGET_FLOAT128_ENABLE_TYPE > #define TARGET_FLOAT128_ENABLE_TYPE 1 > + > +/* Enable support for PC-relative addressing on the 'future' system. Currently > + this support only exits for the ELF v2 object file format using the medium > + code model. */ > +#undef PCREL_SUPPORTED_BY_OS > +#define PCREL_SUPPORTED_BY_OS (TARGET_FUTURE && TARGET_PREFIXED \ > + && ELFv2_ABI_CHECK \ > + && (TARGET_CMODEL == CMODEL_MEDIUM)) Is there need or desire to explicitly set TARGET_FUTURE or TARGET_PREFIXED to zero in the header file now? There are no other references to those in the header file. Otherwise OK. > --- /tmp/sO5cAE_rs6000-cpus.def 2020-03-20 20:15:38.331862575 -0400 > +++ gcc/config/rs6000/rs6000-cpus.def 2020-03-20 17:05:17.347638233 -0400 > @@ -75,11 +75,10 @@ > | OPTION_MASK_P8_VECTOR \ > | OPTION_MASK_P9_VECTOR) > > -/* Support for a future processor's features. Do not enable -mpcrel until it > - is fully functional. */ > +/* Support for a future processor's features. The addressing related options > + (like -mprefixed, -mpcrel) are not set here. */ So, where are they set? why is it important to say they are not set here? > #define ISA_FUTURE_MASKS_SERVER (ISA_3_0_MASKS_SERVER \ > - | OPTION_MASK_FUTURE \ > - | OPTION_MASK_PREFIXED) > + | OPTION_MASK_FUTURE) > > /* Flags that need to be turned off if -mno-future. */ > #define OTHER_FUTURE_MASKS (OPTION_MASK_PCREL \ > --- /tmp/xQt3Pd_rs6000.c 2020-03-20 20:15:38.343862485 -0400 > +++ gcc/config/rs6000/rs6000.c 2020-03-20 20:11:02.942928364 -0400 > @@ -98,6 +98,12 @@ > #endif > #endif > > +/* Set up the defaults for whether PC-relative addressing is supported by the > + target system. */ > +#ifndef PCREL_SUPPORTED_BY_OS > +#define PCREL_SUPPORTED_BY_OS 0 > +#endif Presumably this will one day have additional logic to enable PCREL. Ok. > + > /* Support targetm.vectorize.builtin_mask_for_load. */ > tree altivec_builtin_mask_for_load; > > @@ -4012,6 +4018,11 @@ rs6000_option_override_internal (bool gl > rs6000_isa_flags &= ~OPTION_MASK_FLOAT128_HW; > } > > + /* Enable -mprefixed by default on 64-bit 'future' systems. */ > + if (TARGET_FUTURE && TARGET_POWERPC64 > + && (rs6000_isa_flags_explicit & OPTION_MASK_PREFIXED) == 0) > + rs6000_isa_flags |= OPTION_MASK_PREFIXED; > + > /* -mprefixed (and hence -mpcrel) requires -mcpu=future. */ > if (TARGET_PREFIXED && !TARGET_FUTURE) > { Ok. > @@ -4173,6 +4184,11 @@ rs6000_option_override_internal (bool gl > rs6000_isa_flags &= ~OPTION_MASK_PCREL; > } > > + /* If the OS has support for PC-relative relocations, enable it now. */ > + if (PCREL_SUPPORTED_BY_OS > + && (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) == 0) > + rs6000_isa_flags |= OPTION_MASK_PCREL; > + > if (TARGET_DEBUG_REG || TARGET_DEBUG_TARGET) > rs6000_print_isa_options (stderr, 0, "after subtarget", rs6000_isa_flags); > Ok. > >