[PATCH], Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2

[PATCH], Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2

[Michael Meissner]

This is a revised version of the patch I posted on March 23rd.  The changes are
to update the comments and improve the ChangeLog.

There were no regressions when I did the bootstrap and make check steps.  I
verified that -mcpu=future does turn on -mprecl if you are targeting a Linux
ELF v2 system and use the medium code model.  Can I check this into the master
branch?

2020-03-27  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): New macro.
	* config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Do not
	set -mprefixed here.
	* config/rs6000/rs6000.c (PCREL_SUPPORTED_BY_OS): New macro.
	(rs6000_option_override_internal): Set the -mprefixed and -mpcrel
	options for -mcpu=future if these options can be used.

--- /tmp/JVBhAf_linux64.h	2020-03-27 16:27:05.478619500 -0400
+++ gcc/config/rs6000/linux64.h	2020-03-27 16:21:56.268876616 -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 default support for PC-relative addressing on the 'future' system if
+   we can use the PC-relative instructions.  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/KyQOUN_rs6000-cpus.def	2020-03-27 16:27:05.488619427 -0400
+++ gcc/config/rs6000/rs6000-cpus.def	2020-03-27 16:23:51.780030238 -0400
@@ -75,11 +75,11 @@
 				 | 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.  We do not set -mpcrel or
+   -mprefixed here.  These bits are set in rs6000_option_override if the system
+   supports those options. */
 #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/z4Mwhm_rs6000.c	2020-03-27 16:27:05.500619340 -0400
+++ gcc/config/rs6000/rs6000.c	2020-03-27 16:20:13.066641659 -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;
 
@@ -4014,6 +4020,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)
     {
@@ -4175,6 +4186,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], Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2

[will schmidt]

On Fri, 2020-03-27 at 21:31 -0400, Michael Meissner via Gcc-patches
wrote:


Hi, 
   A few cosmetic nits and comments sprinkled in below.  I defer to
Segher for his approvals and comments.  thanks,
-Will

> This is a revised version of the patch I posted on March 23rd.  The
> changes are
> to update the comments and improve the ChangeLog.

Missing the description of what the patch actually does. 
(From Mar 23):
>  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.


> There were no regressions when I did the bootstrap and make check
> steps.  I
> verified that -mcpu=future does turn on -mprecl if you are targeting
> a Linux

-mpcrel

> ELF v2 system and use the medium code model.  Can I check this into
> the master
> branch?
> 
> 2020-03-27  Michael Meissner  <meissner@linux.ibm.com>
> 
> 	* config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): New macro.
> 	* config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Do
> not
> 	set -mprefixed here.
> 	* config/rs6000/rs6000.c (PCREL_SUPPORTED_BY_OS): New macro.
> 	(rs6000_option_override_internal): Set the -mprefixed and
> -mpcrel
> 	options for -mcpu=future if these options can be used.
> 
s/can be used/are supported by the platform/ ? 


> --- /tmp/JVBhAf_linux64.h	2020-03-27 16:27:05.478619500 -0400
> +++ gcc/config/rs6000/linux64.h	2020-03-27 16:21:56.268876616
> -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 default support for PC-relative addressing on the 'future'
> system if
> +   we can use the PC-relative instructions.  Currently this support
> only exits

exists

> +   for the ELF v2 object file format using the medium code
> model.  */

should that be "s/object file format/ABI/" ? 

> +#undef  PCREL_SUPPORTED_BY_OS
> +#define PCREL_SUPPORTED_BY_OS	(TARGET_FUTURE &&
> TARGET_PREFIXED	\
> +				 && ELFv2_ABI_CHECK			
> \
> +				 && (TARGET_CMODEL == CMODEL_MEDIUM))
> --- /tmp/KyQOUN_rs6000-cpus.def	2020-03-27 16:27:05.488619427
> -0400
> +++ gcc/config/rs6000/rs6000-cpus.def	2020-03-27 16:23:51.780030238
> -0400
> @@ -75,11 +75,11 @@
>  				 | 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.  We do not set -mpcrel
> or
> +   -mprefixed here.  These bits are set in rs6000_option_override if
> the system
> +   supports those options. */

I'm still not sure the comment here is actually necessary, there are
many other places where we also do not set -mpcrel or -mprefixed.  If
history of the code here requires a hint to point at those options
being set in rs6000_option_override, then it's fine.

>  #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/z4Mwhm_rs6000.c	2020-03-27 16:27:05.500619340 -0400
> +++ gcc/config/rs6000/rs6000.c	2020-03-27 16:20:13.066641659
> -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;
> 
> @@ -4014,6 +4020,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)
>      {
> @@ -4175,6 +4186,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. 

thanks
-Will

Re: [PATCH], Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2

[Segher Boessenkool]

Hi!

On Mon, Mar 30, 2020 at 12:50:43PM -0500, will schmidt wrote:
> On Fri, 2020-03-27 at 21:31 -0400, Michael Meissner via Gcc-patches
> > 	* config/rs6000/rs6000.c (PCREL_SUPPORTED_BY_OS): New macro.
> > 	(rs6000_option_override_internal): Set the -mprefixed and
> > -mpcrel
> > 	options for -mcpu=future if these options can be used.
> > 
> s/can be used/are supported by the platform/ ? 

The code says
  /* Enable -mprefixed by default on 64-bit 'future' systems.  */
  /* If the OS has support for PC-relative relocations, enable it now.  */
and something like that should go in the changelog as well (two lines in
changelog is fine -- they are two hunks of patch as well, anyway!)

> > +/* Enable default support for PC-relative addressing on the 'future'
> > system if
> > +   we can use the PC-relative instructions.  Currently this support
> > only exits
> 
> exists
> 
> > +   for the ELF v2 object file format using the medium code
> > model.  */
> 
> should that be "s/object file format/ABI/" ? 

Yes.

> > -/* Support for a future processor's features.  Do not enable -mpcrel
> > until it
> > -   is fully functional.  */
> > +/* Support for a future processor's features.  We do not set -mpcrel
> > or
> > +   -mprefixed here.  These bits are set in rs6000_option_override if
> > the system
> > +   supports those options. */
> 
> I'm still not sure the comment here is actually necessary, there are
> many other places where we also do not set -mpcrel or -mprefixed.  If
> history of the code here requires a hint to point at those options
> being set in rs6000_option_override, then it's fine.

If you really need to say you do *not* do something, you should say why
not.  Without that it only leaves more questions to the reader :-)

Hopefully that then also explains why the reader should care about this.


Segher

Re: [PATCH], Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2

[Michael Meissner]

On Mon, Mar 30, 2020 at 05:51:49PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Mar 30, 2020 at 12:50:43PM -0500, will schmidt wrote:
> > On Fri, 2020-03-27 at 21:31 -0400, Michael Meissner via Gcc-patches
> > > 	* config/rs6000/rs6000.c (PCREL_SUPPORTED_BY_OS): New macro.
> > > 	(rs6000_option_override_internal): Set the -mprefixed and
> > > -mpcrel
> > > 	options for -mcpu=future if these options can be used.
> > > 
> > s/can be used/are supported by the platform/ ? 
> 
> The code says
>   /* Enable -mprefixed by default on 64-bit 'future' systems.  */
>   /* If the OS has support for PC-relative relocations, enable it now.  */
> and something like that should go in the changelog as well (two lines in
> changelog is fine -- they are two hunks of patch as well, anyway!)

Ok.

> > > +/* Enable default support for PC-relative addressing on the 'future'
> > > system if
> > > +   we can use the PC-relative instructions.  Currently this support
> > > only exits
> > 
> > exists
> > 
> > > +   for the ELF v2 object file format using the medium code
> > > model.  */
> > 
> > should that be "s/object file format/ABI/" ? 
> 
> Yes.

Ok.

> > > -/* Support for a future processor's features.  Do not enable -mpcrel
> > > until it
> > > -   is fully functional.  */
> > > +/* Support for a future processor's features.  We do not set -mpcrel
> > > or
> > > +   -mprefixed here.  These bits are set in rs6000_option_override if
> > > the system
> > > +   supports those options. */
> > 
> > I'm still not sure the comment here is actually necessary, there are
> > many other places where we also do not set -mpcrel or -mprefixed.  If
> > history of the code here requires a hint to point at those options
> > being set in rs6000_option_override, then it's fine.
> 
> If you really need to say you do *not* do something, you should say why
> not.  Without that it only leaves more questions to the reader :-)
> 
> Hopefully that then also explains why the reader should care about this.

Given this comment is against Will's comment, and not the original code, is
there anything I need to do to the code (other than the ChangeLog and adjusting
object file format to ABI?

-- 
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], Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2

[Segher Boessenkool]

On Wed, Apr 01, 2020 at 03:16:52PM -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.  We do not set -mpcrel
> > > > or
> > > > +   -mprefixed here.  These bits are set in rs6000_option_override if
> > > > the system
> > > > +   supports those options. */
> > > 
> > > I'm still not sure the comment here is actually necessary, there are
> > > many other places where we also do not set -mpcrel or -mprefixed.  If
> > > history of the code here requires a hint to point at those options
> > > being set in rs6000_option_override, then it's fine.
> > 
> > If you really need to say you do *not* do something, you should say why
> > not.  Without that it only leaves more questions to the reader :-)
> > 
> > Hopefully that then also explains why the reader should care about this.
> 
> Given this comment is against Will's comment, and not the original code, is
> there anything I need to do to the code (other than the ChangeLog and adjusting
> object file format to ABI?

I am agreeing with Will's comment here, just expanding on it.  This
comment isn't helpful (maybe it would be with more provided context,
but as it is, it is not).


Segher

Re: [PATCH], Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2

[Michael Meissner]

On Wed, Apr 01, 2020 at 04:36:05PM -0500, Segher Boessenkool wrote:
> On Wed, Apr 01, 2020 at 03:16:52PM -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.  We do not set -mpcrel
> > > > > or
> > > > > +   -mprefixed here.  These bits are set in rs6000_option_override if
> > > > > the system
> > > > > +   supports those options. */
> > > > 
> > > > I'm still not sure the comment here is actually necessary, there are
> > > > many other places where we also do not set -mpcrel or -mprefixed.  If
> > > > history of the code here requires a hint to point at those options
> > > > being set in rs6000_option_override, then it's fine.
> > > 
> > > If you really need to say you do *not* do something, you should say why
> > > not.  Without that it only leaves more questions to the reader :-)
> > > 
> > > Hopefully that then also explains why the reader should care about this.
> > 
> > Given this comment is against Will's comment, and not the original code, is
> > there anything I need to do to the code (other than the ChangeLog and adjusting
> > object file format to ABI?
> 
> I am agreeing with Will's comment here, just expanding on it.  This
> comment isn't helpful (maybe it would be with more provided context,
> but as it is, it is not).

Ok, I will revist the comment.

-- 
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], Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2

[Segher Boessenkool]

Hi!

Some more comments:

On Fri, Mar 27, 2020 at 09:31:46PM -0400, Michael Meissner wrote:
> There were no regressions when I did the bootstrap and make check steps.  I
> verified that -mcpu=future does turn on -mprecl if you are targeting a Linux
> ELF v2 system and use the medium code model.  Can I check this into the master
> branch?

Please post the commit message you would use as well.  It often can
*replace* what you would type in the mail otherwise (just add some
things like how it was tested, etc).

> 2020-03-27  Michael Meissner  <meissner@linux.ibm.com>
> 
> 	* config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): New macro.
> 	* config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Do not
> 	set -mprefixed here.

OPTION_MASK_PREFIXED

> 	* config/rs6000/rs6000.c (PCREL_SUPPORTED_BY_OS): New macro.

It cannot be "new macro" both here and in linux64.h .  This latter one
is the default implementation.

> 	(rs6000_option_override_internal): Set the -mprefixed and -mpcrel
> 	options for -mcpu=future if these options can be used.

Similar, OPTION_*.  Please talk about PCREL_SUPPORTED_BY_OS.

A changelog very boringly says *what* and *how*, never "why".


> --- /tmp/JVBhAf_linux64.h	2020-03-27 16:27:05.478619500 -0400
> +++ gcc/config/rs6000/linux64.h	2020-03-27 16:21:56.268876616 -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 default support for PC-relative addressing on the 'future' system if
> +   we can use the PC-relative instructions.  Currently this support only exits
> +   for the ELF v2 object file format using the medium code model.  */

(exists, typo)

> +#undef  PCREL_SUPPORTED_BY_OS
> +#define PCREL_SUPPORTED_BY_OS	(TARGET_FUTURE && TARGET_PREFIXED	\
> +				 && ELFv2_ABI_CHECK			\
> +				 && (TARGET_CMODEL == CMODEL_MEDIUM))

There should not be an #undef here, it just hides bugs (or causes them).

> --- /tmp/KyQOUN_rs6000-cpus.def	2020-03-27 16:27:05.488619427 -0400
> +++ gcc/config/rs6000/rs6000-cpus.def	2020-03-27 16:23:51.780030238 -0400
> @@ -75,11 +75,11 @@
>  				 | 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.  We do not set -mpcrel or
> +   -mprefixed here.  These bits are set in rs6000_option_override if the system
> +   supports those options. */

We talked about this before.

Things might be easier if you had different OPTIONs for "can the CPU do
this" and "can the OS do it".

> +/* 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;
>  
> @@ -4014,6 +4020,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;

This does not need OS support?

> +  /* 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;

So the user can enable pcrel even if PCREL_SUPPORTED_BY_OS is false.
Okay, that is good.  Maybe a better name could be found, but I can't
think of one either.


Segher