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

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

[Michael Meissner]

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.

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.

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.
	(OTHER_FUTURE_MASKS): Define the bits for -mprefixed and -mpcrel
	here.
	* config/rs6000/rs6000.c (PCREL_SUPPORTED_BY_OS): If not defined,
	don't enable -mpcrel by default.
	(rs6000_option_override_internal): Enable -mpcrel on systems that
	support it, if the user did not do -mno-pcrel.

--- /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))
--- /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.  */
 #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
+
 /* 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)
     {
@@ -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);
 


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

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

[will schmidt]

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

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

> 
> 

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

[Segher Boessenkool]

On Wed, Mar 25, 2020 at 11:07:04AM -0500, will schmidt 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). 

rs6000: Enable pcrel by default (etc.)

Subjects start with a capital, but end without dot.  Just like in normal
email.

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

Yes :-)  But, don't say "This patch", if you can avoid it...  just use an
imperative: "Make -mpcrel the default ..."

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

It is automatically defined as
  #define TARGET_FUTURE ((rs6000_isa_flags & OPTION_MASK_FUTURE) != 0)
so that will work just fine.


Thanks for the review Will.  Mike, could you send a patch with those
fixes please?  Make sure the changelog agrees with the patch (and don't
say "why" in changelog -- say that in the commit message.  Which is
free form, so you have much more freedom to explain things in a useful
order).


Segher

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

[Michael Meissner]

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

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

[Segher Boessenkool]

On Fri, Mar 27, 2020 at 02:30:36PM -0400, Michael Meissner wrote:
> > > -/* 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.

Please just don't mention it here at all then?  It only confuses
matters, it does not help the reader at all.


Segher