[PATCH, V4] PowerPC Turn on -mpcrel by default for -mcpu=future

[PATCH, V4] PowerPC Turn on -mpcrel by default for -mcpu=future

[Michael Meissner]

Commit message:
Enable -mpcrel for -mcpu=future if it is allowed by the ABI.

2020-04-06  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): Enable
	prefixed PC-relative addressing if the ABI supports it.
	* config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Do not
	set OPTION_MASK_PREFIXED here.
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Enable
	OPTION_MASK_PREFIXED and OPTION_MASK_PCREL on -mcpu=future by
	default if the current ABI allows the options.

I tested this on a little endian PowerPC power8 system, doing bootstrap and
make check.  There were no regressions.  I tested by hand the various
conditions where -mpcrel is not enabled, and they all used the normal power9
TOC references.

    -mcpu=power9			generates TOC;
    -mcpu=future -mcmodel=large		generates TOC;
    -mcpu=future -mcmode=small		generates TOC;
    -mcpu=future -mno-prefixed		generates TOC;
    -mcpu=future -mno-pcrel		generates TOC;
    -mcpu=future			generates PC-relative.

--- /tmp/apbaWN_linux64.h	2020-04-03 17:15:05.059677000 -0400
+++ gcc/config/rs6000/linux64.h	2020-04-03 17:01:05.580426937 -0400
@@ -640,3 +640,10 @@ extern int dot_symbols;
    enabling the __float128 keyword.  */
 #undef	TARGET_FLOAT128_ENABLE_TYPE
 #define TARGET_FLOAT128_ENABLE_TYPE 1
+
+/* Enable using prefixed PC-relative addressing on the 'future' machine if the
+   ABI supports it.  The ELF v2 ABI only supports PC-relative relocations for
+   the medium code model.  */
+#define PCREL_SUPPORTED_BY_OS	(TARGET_FUTURE && TARGET_PREFIXED	\
+				 && ELFv2_ABI_CHECK			\
+				 && (TARGET_CMODEL == CMODEL_MEDIUM))
--- /tmp/XzRKno_rs6000-cpus.def	2020-04-03 17:15:05.068676928 -0400
+++ gcc/config/rs6000/rs6000-cpus.def	2020-04-03 17:00:50.115550614 -0400
@@ -75,10 +75,14 @@
 				 | 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.  Do not set OPTION_MASK_PREFIXED
+   or OPTION_MASK_PCREL here.  Those options are enabled in the function
+   rs6000_option_override if the ABI supports them.  */
 #define ISA_FUTURE_MASKS_SERVER	(ISA_3_0_MASKS_SERVER			\
-				 | OPTION_MASK_FUTURE			\
+				 | OPTION_MASK_FUTURE)
+
+/* Flags that need to be turned off if -mno-future.  */
+#define OTHER_FUTURE_MASKS	(OPTION_MASK_PCREL			\
 				 | OPTION_MASK_PREFIXED)
 
 /* Flags that need to be turned off if -mno-future.  */
--- /tmp/nyxSRY_rs6000.c	2020-04-03 17:15:05.081676823 -0400
+++ gcc/config/rs6000/rs6000.c	2020-04-03 17:03:19.846353197 -0400
@@ -4020,6 +4020,12 @@ 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)
     {
@@ -4171,6 +4177,14 @@ rs6000_option_override_internal (bool gl
   SUB3TARGET_OVERRIDE_OPTIONS;
 #endif
 
+#ifdef PCREL_SUPPORTED_BY_OS
+  /* If the ABI has support for PC-relative relocations, enable it by
+     default.  */
+  if (PCREL_SUPPORTED_BY_OS
+      && (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) == 0)
+    rs6000_isa_flags |= OPTION_MASK_PCREL;
+#endif
+
   /* -mpcrel requires -mcmodel=medium, but we can't check TARGET_CMODEL until
       after the subtarget override options are done.  */
   if (TARGET_PCREL && TARGET_CMODEL != CMODEL_MEDIUM)

-- 
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, V4] PowerPC Turn on -mpcrel by default for -mcpu=future

[will schmidt]

On Mon, 2020-04-06 at 12:52 -0400, Michael Meissner via Gcc-patches wrote:

Hi, 

Just a single extra blank line below.

I'm still not a fan of the "Do not set..." comment, but will assume there
is history that necessitates the comment.

Other sections looked OK to me.

Over to Segher. :-)

Thanks,
-Will

> Commit message:
> Enable -mpcrel for -mcpu=future if it is allowed by the ABI.
> 
> 2020-04-06  Michael Meissner  <meissner@linux.ibm.com>
> 
> 	* config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): Enable
> 	prefixed PC-relative addressing if the ABI supports it.
> 	* config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Do not
> 	set OPTION_MASK_PREFIXED here.
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Enable
> 	OPTION_MASK_PREFIXED and OPTION_MASK_PCREL on -mcpu=future by
> 	default if the current ABI allows the options.
> 
> I tested this on a little endian PowerPC power8 system, doing bootstrap and
> make check.  There were no regressions.  I tested by hand the various
> conditions where -mpcrel is not enabled, and they all used the normal power9
> TOC references.
> 
>     -mcpu=power9			generates TOC;
>     -mcpu=future -mcmodel=large		generates TOC;
>     -mcpu=future -mcmode=small		generates TOC;
>     -mcpu=future -mno-prefixed		generates TOC;
>     -mcpu=future -mno-pcrel		generates TOC;
>     -mcpu=future			generates PC-relative.
> 

ok,

> --- /tmp/apbaWN_linux64.h	2020-04-03 17:15:05.059677000 -0400
> +++ gcc/config/rs6000/linux64.h	2020-04-03 17:01:05.580426937 -0400
> @@ -640,3 +640,10 @@ extern int dot_symbols;
>     enabling the __float128 keyword.  */
>  #undef	TARGET_FLOAT128_ENABLE_TYPE
>  #define TARGET_FLOAT128_ENABLE_TYPE 1
> +
> +/* Enable using prefixed PC-relative addressing on the 'future' machine if the
> +   ABI supports it.  The ELF v2 ABI only supports PC-relative relocations for
> +   the medium code model.  */
> +#define PCREL_SUPPORTED_BY_OS	(TARGET_FUTURE && TARGET_PREFIXED	\
> +				 && ELFv2_ABI_CHECK			\
> +				 && (TARGET_CMODEL == CMODEL_MEDIUM))

ok


> --- /tmp/XzRKno_rs6000-cpus.def	2020-04-03 17:15:05.068676928 -0400
> +++ gcc/config/rs6000/rs6000-cpus.def	2020-04-03 17:00:50.115550614 -0400
> @@ -75,10 +75,14 @@
>  				 | 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.  Do not set OPTION_MASK_PREFIXED
> +   or OPTION_MASK_PCREL here.  Those options are enabled in the function
> +   rs6000_option_override if the ABI supports them.  */
>  #define ISA_FUTURE_MASKS_SERVER	(ISA_3_0_MASKS_SERVER			\
> -				 | OPTION_MASK_FUTURE			\
> +				 | OPTION_MASK_FUTURE)
> +
> +/* Flags that need to be turned off if -mno-future.  */
> +#define OTHER_FUTURE_MASKS	(OPTION_MASK_PCREL			\
>  				 | OPTION_MASK_PREFIXED)
> 

ok.

>  /* Flags that need to be turned off if -mno-future.  */
> --- /tmp/nyxSRY_rs6000.c	2020-04-03 17:15:05.081676823 -0400
> +++ gcc/config/rs6000/rs6000.c	2020-04-03 17:03:19.846353197 -0400
> @@ -4020,6 +4020,12 @@ 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;
> +
> +

extra blank line.

>    /* -mprefixed (and hence -mpcrel) requires -mcpu=future.  */
>    if (TARGET_PREFIXED && !TARGET_FUTURE)
>      {
> @@ -4171,6 +4177,14 @@ rs6000_option_override_internal (bool gl
>    SUB3TARGET_OVERRIDE_OPTIONS;
>  #endif
> 
> +#ifdef PCREL_SUPPORTED_BY_OS
> +  /* If the ABI has support for PC-relative relocations, enable it by
> +     default.  */
> +  if (PCREL_SUPPORTED_BY_OS
> +      && (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) == 0)
> +    rs6000_isa_flags |= OPTION_MASK_PCREL;
> +#endif
> +

ok.

>    /* -mpcrel requires -mcmodel=medium, but we can't check TARGET_CMODEL until
>        after the subtarget override options are done.  */
>    if (TARGET_PCREL && TARGET_CMODEL != CMODEL_MEDIUM)
> 

Re: [PATCH, V4] PowerPC Turn on -mpcrel by default for -mcpu=future

[Segher Boessenkool]

Hi!

On Mon, Apr 06, 2020 at 12:52:04PM -0400, Michael Meissner wrote:
> Commit message:
> Enable -mpcrel for -mcpu=future if it is allowed by the ABI.

You can just put this in the mail subject.  No dot at the end, but start
with a capital, an put a subsystem label on it:

Subject: rs6000: Enable -mpcrel for -mcpu=future if it is allowed by the ABI

(As you see, it is a bit long already).

> 2020-04-06  Michael Meissner  <meissner@linux.ibm.com>
> 
> 	* config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): Enable
> 	prefixed PC-relative addressing if the ABI supports it.
> 	* config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Do not
> 	set OPTION_MASK_PREFIXED here.
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Enable
> 	OPTION_MASK_PREFIXED and OPTION_MASK_PCREL on -mcpu=future by
> 	default if the current ABI allows the options.

The changelog goes at the *end* of the commit message.  Oh, there was
nothing more?

> --- /tmp/XzRKno_rs6000-cpus.def	2020-04-03 17:15:05.068676928 -0400
> +++ gcc/config/rs6000/rs6000-cpus.def	2020-04-03 17:00:50.115550614 -0400
> @@ -75,10 +75,14 @@
>  				 | 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.  Do not set OPTION_MASK_PREFIXED
> +   or OPTION_MASK_PCREL here.  Those options are enabled in the function
> +   rs6000_option_override if the ABI supports them.  */

I am still not happy with that, and it should be fixed.  But, let's do
that later then.

The ISA mask should say what features are supported.  If we choose not
to use some we should do that elsewhere, in option_override for example.
Expressing only 95% of the features in the ISA masks is the worst of
both worlds.

> +/* Flags that need to be turned off if -mno-future.  */
> +#define OTHER_FUTURE_MASKS	(OPTION_MASK_PCREL			\
>  				 | OPTION_MASK_PREFIXED)
>  
>  /* Flags that need to be turned off if -mno-future.  */

And right after this is already a define for OTHER_FUTURE_MASKS.  Did
you mismerge this?  Please correct and repost.

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

I don't understand why only for 64 bit?

> +#ifdef PCREL_SUPPORTED_BY_OS
> +  /* If the ABI has support for PC-relative relocations, enable it by
> +     default.  */
> +  if (PCREL_SUPPORTED_BY_OS
> +      && (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) == 0)
> +    rs6000_isa_flags |= OPTION_MASK_PCREL;
> +#endif

No #ifdef at uses please, unless there is a reason (there is none here
afaics).

The #ifndef you had before for the default definition was fine.  The
#undef you had for a non-default definition was not: it hides problems,
it never helps.


Segher

Re: [PATCH, V4] PowerPC Turn on -mpcrel by default for -mcpu=future

[Michael Meissner]

On Tue, Apr 07, 2020 at 05:58:01PM -0500, Segher Boessenkool wrote:
> > +  /* 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;
> 
> I don't understand why only for 64 bit?

I have doubts whether PC-relative really works with 32-bit.  I suspect that you
may see some hidden wrap around issues if an address crosses the 32-bit
boundary.  Given the simulator I have access to only runs Linux 64-bit, I have
no way of testing it.  I would prefer to not put in code that I can't test.

But if the only way to get the patch in is to remove the test, I can remove it.

-- 
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, V4] PowerPC Turn on -mpcrel by default for -mcpu=future

[Segher Boessenkool]

On Tue, Apr 21, 2020 at 02:03:34PM -0400, Michael Meissner wrote:
> On Tue, Apr 07, 2020 at 05:58:01PM -0500, Segher Boessenkool wrote:
> > > +  /* 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;
> > 
> > I don't understand why only for 64 bit?
> 
> I have doubts whether PC-relative really works with 32-bit.  I suspect that you
> may see some hidden wrap around issues if an address crosses the 32-bit
> boundary.

Which is undefined behaviour *anyway*?  In C, etc.

The Power ISA makes it very clear how this behaves, though:

5.7.1 32-Bit Mode
The computation of the 64-bit effective address is inde-
pendent of whether the thread is in 32-bit mode or
64-bit mode. In 32-bit mode (MSR[SF]=0), the high-order
32 bits of the 64-bit effective address are treated as
zeros for the purpose of addressing storage. This
applies to both data accesses and instruction fetches. It
applies independent of whether address translation is
enabled or disabled. This truncation of the effective
address is the only respect in which storage accesses
in 32-bit mode differ from those in 64-bit mode.

> Given the simulator I have access to only runs Linux 64-bit, I have
> no way of testing it.  I would prefer to not put in code that I can't test.

Think of it as "this cannot happen", if that makes you feel better?  :-)

> But if the only way to get the patch in is to remove the test, I can remove it.


Segher