Re: [PATCH], Set -mpcrel by default on Linux 64-bit systems for -mcpu=future

[Michael Meissner <meissner@linux.ibm.com>]

On Wed, Mar 25, 2020 at 11:07:04AM -0500, will schmidt wrote:
> Hi,
> Comments inline.
> 
> > 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  <meissner@linux.ibm.com>
> > 
> > 	* 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.

Well there previously was a comment stating that we would set it here.

> > 	(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?)

Probably.

> > 	* 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 ...// 

Ok.

> > 
> > --- /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.

As Segher says, this is handled by the normal option handling.

> > --- /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?

They are set in rs6000_option_override in rs6000.c, like all of the other
defaults.

The issue is that not all 'future' targets will enable these bits.  In general,
it is simpler to set the bits to ON in the cases where they should be, rather
than settings the bits here and then resetting them.

In particular, other operating systems (like AIX, Linux using ELF v1, or
32-bit) might not have the necessary support for the PC-relative relocations.
In addition, if the user did -mcmodel=large or -mcmodel=small, we cannot turn
on the PC-relative addressing, because the instructions only have a 34-bit
offset, and the other code models have different assumptions.

Perhaps one day, we might think about adding the support for -mcmodel=large,
but for now, you need to use the normal TOC addressing for that.

> >  #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.

Well this particular definition is the catch all case, in case the target OS
header did not say it was supported.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797