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

Message ID 20200328013146.GA26011@ibm-tinman.the-meissners.org
State New
Headers show
Series
  • , Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2
Related show

Commit Message

Jakub Jelinek via Gcc-patches March 28, 2020, 1:31 a.m.
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.



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

Comments

Jakub Jelinek via Gcc-patches March 30, 2020, 5:50 p.m. | #1
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
Segher Boessenkool March 30, 2020, 10:51 p.m. | #2
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
Jakub Jelinek via Gcc-patches April 1, 2020, 7:16 p.m. | #3
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
Segher Boessenkool April 1, 2020, 9:36 p.m. | #4
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
Jakub Jelinek via Gcc-patches April 1, 2020, 11:11 p.m. | #5
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
Segher Boessenkool April 2, 2020, 7:27 p.m. | #6
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

Patch

--- /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);