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

[Segher Boessenkool <segher@kernel.crashing.org>]

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