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

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

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