Hi,
Comments inline.
(Taking a pass with focus on cosmetic stuff. This is intended to help Segher
focus on the harder parts :-) ).
On Mon, 2020-03-23 at 16:38 -0400, Michael Meissner via Gcc-patches wrote:
Subject: [Patch v327] set -mcprel by default ...
Include some powerpc or rs6000 reference in the subject. My filters
missed this one. (I'm assuming this is powerpc target specific).
> This is a revision of a patch that I've submitted several times. 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.
The fewer words, the easier to review. History is important but so is
making the review easy.
"This patch makes -mpcrel the default on Linux 64-bit systems that use
ELF V2, medium code model, and have not disabled prefix instructions."
>
> Previous versions of the patch had two macros that the target os could set,
> one
> to say that the target os supported prefixed instructions with large numeric
> offsets, and the second whether PC-relative prefixed load/store instructions
> could be generated. Segher asked that we drop the capability to configure
> whether prefixed numeric load/store instructions would be enabled by default,
> and this patch does this. All of the PC-relative support is in the master
> branch, we just need to enable it by default.
[v327] dropped macros to indicate if target supports prefixed
instructions pre previous feedback.
>
> Is this patch acceptable to be committed to the master branch. I have done
> various tests with this patch, including most recently bootstraping and
> running
> make check. I have built the Spec 2017 benchmark suite with this patch.
>
> 2020-03-23 Michael Meissner <meiss...@linux.ibm.com>
>
> * config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): Enable -mpcrel
> for -mcpu=future by default on 64-bit systems with medium code
> model.
> * config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Do not
> define the bits for -mprefixed or -mpcrel here.
The change to ISA_FUTURE_MASKS_SERVER only drops OPTION_MASK_PREFIXED.
No change to PCREL there.
> (OTHER_FUTURE_MASKS): Define the bits for -mprefixed and -mpcrel
> here.
No touches to OTHER_FUTURE_MASKS below. (accidental patch ommission
or missed a changelog update?)
> * config/rs6000/rs6000.c (PCREL_SUPPORTED_BY_OS): If not defined,
> don't enable -mpcrel by default.
I suggest
s/If not defined//
> (rs6000_option_override_internal): Enable -mpcrel on systems that
> support it, if the user did not do -mno-pcrel.
I suggest
s/if the user ...//
>
> --- /tmp/QuuFm5_linux64.h 2020-03-20 20:15:38.321862650 -0400
> +++ gcc/config/rs6000/linux64.h 2020-03-20 18:36:33.654484833 -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 support for PC-relative addressing on the 'future' system.
> 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))
Is there need or desire to explicitly set TARGET_FUTURE or
TARGET_PREFIXED to zero in the header file now? There are no other
references to those in the header file.
Otherwise OK.
> --- /tmp/sO5cAE_rs6000-cpus.def 2020-03-20 20:15:38.331862575 -0400
> +++ gcc/config/rs6000/rs6000-cpus.def 2020-03-20 17:05:17.347638233 -0400
> @@ -75,11 +75,10 @@
> | 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. The addressing related options
> + (like -mprefixed, -mpcrel) are not set here. */
So, where are they set? why is it important to say they are not set
here?
> #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/xQt3Pd_rs6000.c 2020-03-20 20:15:38.343862485 -0400
> +++ gcc/config/rs6000/rs6000.c 2020-03-20 20:11:02.942928364 -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
Presumably this will one day have additional logic to enable PCREL.
Ok.
> +
> /* Support targetm.vectorize.builtin_mask_for_load. */
> tree altivec_builtin_mask_for_load;
>
> @@ -4012,6 +4018,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)
> {
Ok.
> @@ -4173,6 +4184,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.
>
>
