[V4] Eliminate power8 fusion options, use power8 tuning, PR target/102059

  Eliminate power8 fusion options, use power8 tuning, PR target/102059

This is V4 of the patch.  Compared to V3 of the patch, GCC will just
ignore -m{,no-}power8-fusion and -m{,no-}power8-fusion-sign.

The splitting of signed halfword and word loads into unsigned load and
sign extension is now suppressed with -Os, but it is done normally if we
are not optimizing for space.

The power8 fusion support used to be set automatically when -mcpu=power8 or
-mtune=power8 was used, and it was cleared for other cpu's.  However, if you
used the target attribute or target #pragma to change the default cpu type or
tuning, you would get an error that a target specifiction option mismatch
occurred.

This occurred because the rs6000_can_inline_p function just compares the ISA
bits between the called inline function and the caller.  If the ISA flags of
the called function is not a subset of the ISA flags of the caller, we won't do
the inlinging.  When a power9 or power10 function inlines a function that is
explicitly compiled for power8, the power8 function has the power8 fusion bits
set and the power9 or power10 functions do not have the fusion bits set.

This code makes the -mpower8-fusion option a nop.  It is accepted without
warning, but it does nothing.  Power8 fusion is only enabled if we are tuning
for a power8.

The undocumented -mpower8-fusion-sign option is also made into a nop.

I left in the pragma target and attribute target support for power8-fusion, but
using it doesn't do anything now.  This is because I told the customer who
encountered this problem that one solution was to add an explicit
no-power8-fusion option in their target pragma or attribute to work around the
problem.

I have tested this patch on a little endian power10 system.  I have tested
previous versions on little endian power9 and big endian power8 systems.
Can I apply this patch to the master branch?

If it is accepted, I will produce a similar patch for back porting to GCC 11
and GCC 10.

2022-04-12   Michael Meissner  <meissner@linux.ibm.com>

gcc/
	PR target/102059
	* config/rs6000/rs6000-cpus.def (OTHER_FUSION_MASKS): Delete.
	(ISA_3_0_MASKS_SERVER): Don't clear the fusion masks.
	(POWERPC_MASKS): Remove OPTION_MASK_P8_FUSION.
	* config/rs6000/rs6000.cc (rs6000_option_override_internal):
	Delete code that set the power8 fusion options automatically.
	(rs6000_opt_masks): Allow #pragma target and attribute target
	power8-fusion option for backwards compatibility.
	(rs6000_print_options_internal): Skip printing backward
	compatibility options that are just ignored.
	* config/rs6000/rs6000.h (TARGET_P8_FUSION): New macro.
	(TARGET_P8_FUSION_SIGN): Likewise.
	(MASK_P8_FUSION): Delete.
	* config/rs6000/rs6000.opt (-mpower8-fusion): Recognize the option but
	ignore it completely.
	(-mpower8-fusion-sign): Likewise.
	* doc/invoke.texi (RS/6000 and PowerPC Options): Delete
	-mpower8-fusion.

gcc/testsuite/
	PR target/102059
	* gcc.dg/lto/pr102059-1_0.c: Remove -mno-power8-fusion.
	* gcc.dg/lto/pr102059-2_0.c: Likewise.
	* gcc.target/powerpc/pr102059-3.c: Likewise.
	* gcc.target/powerpc/pr102059-4.c: New test.
---
 gcc/config/rs6000/rs6000-cpus.def             | 18 +++----
 gcc/config/rs6000/rs6000.cc                   | 49 +++++--------------
 gcc/config/rs6000/rs6000.h                    | 13 ++++-
 gcc/config/rs6000/rs6000.opt                  |  8 +--
 gcc/doc/invoke.texi                           | 13 +----
 gcc/testsuite/gcc.dg/lto/pr102059-1_0.c       |  2 +-
 gcc/testsuite/gcc.dg/lto/pr102059-2_0.c       |  2 +-
 gcc/testsuite/gcc.target/powerpc/pr102059-3.c |  2 +-
 gcc/testsuite/gcc.target/powerpc/pr102059-4.c | 23 +++++++++
 9 files changed, 62 insertions(+), 68 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr102059-4.c
  

Ping patch.

| Date: Tue, 12 Apr 2022 21:14:55 -0400
| From: Michael Meissner <meissner@linux.ibm.com>
| Subject: [PATCH, V4] Eliminate power8 fusion options, use power8 tuning, PR target/102059
| Message-ID: <YlYkDy9LVeN2LfZ6@toto.the-meissners.org>

I feel this is an important patch.  Please look at it and approve the patch or
give me feedback on how to change it.  Note, I will be in today (April 20th)
and tomorrow (April 21st), but I will be away from a computer on April 22-25
(Friday through Monday).
  

On 4/20/22 11:01 AM, Michael Meissner wrote:
> Ping patch.
> 
> | Date: Tue, 12 Apr 2022 21:14:55 -0400
> | From: Michael Meissner <meissner@linux.ibm.com>
> | Subject: [PATCH, V4] Eliminate power8 fusion options, use power8 tuning, PR target/102059
> | Message-ID: <YlYkDy9LVeN2LfZ6@toto.the-meissners.org>
> 
> I feel this is an important patch.  Please look at it and approve the patch or
> give me feedback on how to change it.  Note, I will be in today (April 20th)
> and tomorrow (April 21st), but I will be away from a computer on April 22-25
> (Friday through Monday).

I agree this is important and we want this is in so we can get it backported.
I'm being pinged about this from a customer who is using GCC10 and this issue
is holding them back, so the quicker we get this in, the better.

Peter
  

On Tue, 2022-04-12 at 21:14 -0400, Michael Meissner wrote:
> Eliminate power8 fusion options, use power8 tuning, PR target/102059
> 
> This is V4 of the patch.  Compared to V3 of the patch, GCC will just
> ignore -m{,no-}power8-fusion and -m{,no-}power8-fusion-sign.
> 


Hi, 
No comments on code, a few comments about the comments below.



> The splitting of signed halfword and word loads into unsigned load and
> sign extension is now suppressed with -Os, but it is done normally if we
> are not optimizing for space.

I see references to TARGET_P8_FUSION_SIGN in the patch below, and some
removal of old code.  I assume this describes the implementation that
remains.  

> 
> The power8 fusion support used to be set automatically when -mcpu=power8 or
> -mtune=power8 was used, and it was cleared for other cpu's.  However, if you
> used the target attribute or target #pragma to change the default cpu type or
> tuning, you would get an error that a target specifiction option mismatch
> occurred.

specification.  :-)

> 
> This occurred because the rs6000_can_inline_p function just compares the ISA
> bits between the called inline function and the caller.  If the ISA flags of
> the called function is not a subset of the ISA flags of the caller, we won't do
> the inlinging.  When a power9 or power10 function inlines a function that is
> explicitly compiled for power8, the power8 function has the power8 fusion bits
> set and the power9 or power10 functions do not have the fusion bits set.

inlining. 


> 
> This code makes the -mpower8-fusion option a nop.  It is accepted without
> warning, but it does nothing.  Power8 fusion is only enabled if we are tuning
> for a power8.
> 
> The undocumented -mpower8-fusion-sign option is also made into a nop.
> 
> I left in the pragma target and attribute target support for power8-fusion, but
> using it doesn't do anything now.  This is because I told the customer who
> encountered this problem that one solution was to add an explicit
> no-power8-fusion option in their target pragma or attribute to work around the
> problem.
> 
> I have tested this patch on a little endian power10 system.  I have tested
> previous versions on little endian power9 and big endian power8 systems.
> Can I apply this patch to the master branch?
> 
> If it is accepted, I will produce a similar patch for back porting to GCC 11
> and GCC 10.
> 
> 2022-04-12   Michael Meissner  <meissner@linux.ibm.com>
> 
> gcc/
> 	PR target/102059
> 	* config/rs6000/rs6000-cpus.def (OTHER_FUSION_MASKS): Delete.
> 	(ISA_3_0_MASKS_SERVER): Don't clear the fusion masks.
> 	(POWERPC_MASKS): Remove OPTION_MASK_P8_FUSION.
> 	* config/rs6000/rs6000.cc (rs6000_option_override_internal):
> 	Delete code that set the power8 fusion options automatically.
> 	(rs6000_opt_masks): Allow #pragma target and attribute target
> 	power8-fusion option for backwards compatibility.
> 	(rs6000_print_options_internal): Skip printing backward
> 	compatibility options that are just ignored.
> 	* config/rs6000/rs6000.h (TARGET_P8_FUSION): New macro.
> 	(TARGET_P8_FUSION_SIGN): Likewise.
> 	(MASK_P8_FUSION): Delete.
> 	* config/rs6000/rs6000.opt (-mpower8-fusion): Recognize the option but
> 	ignore it completely.
> 	(-mpower8-fusion-sign): Likewise.
> 	* doc/invoke.texi (RS/6000 and PowerPC Options): Delete
> 	-mpower8-fusion.
> 
> gcc/testsuite/
> 	PR target/102059
> 	* gcc.dg/lto/pr102059-1_0.c: Remove -mno-power8-fusion.
> 	* gcc.dg/lto/pr102059-2_0.c: Likewise.
> 	* gcc.target/powerpc/pr102059-3.c: Likewise.
> 	* gcc.target/powerpc/pr102059-4.c: New test.
> ---
>  gcc/config/rs6000/rs6000-cpus.def             | 18 +++----
>  gcc/config/rs6000/rs6000.cc                   | 49 +++++--------------
>  gcc/config/rs6000/rs6000.h                    | 13 ++++-
>  gcc/config/rs6000/rs6000.opt                  |  8 +--
>  gcc/doc/invoke.texi                           | 13 +----
>  gcc/testsuite/gcc.dg/lto/pr102059-1_0.c       |  2 +-
>  gcc/testsuite/gcc.dg/lto/pr102059-2_0.c       |  2 +-
>  gcc/testsuite/gcc.target/powerpc/pr102059-3.c |  2 +-
>  gcc/testsuite/gcc.target/powerpc/pr102059-4.c | 23 +++++++++
>  9 files changed, 62 insertions(+), 68 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr102059-4.c
> 
> diff --git a/gcc/config/rs6000/rs6000-cpus.def b/gcc/config/rs6000/rs6000-cpus.def
> index 963947f6939..d913a3d6b73 100644
> --- a/gcc/config/rs6000/rs6000-cpus.def
> +++ b/gcc/config/rs6000/rs6000-cpus.def
> @@ -54,19 +54,14 @@
>  				 | OPTION_MASK_QUAD_MEMORY		\
>  				 | OPTION_MASK_QUAD_MEMORY_ATOMIC)
> 
> -/* ISA masks setting fusion options.  */
> -#define OTHER_FUSION_MASKS	(OPTION_MASK_P8_FUSION			\
> -				 | OPTION_MASK_P8_FUSION_SIGN)
> -
>  /* Add ISEL back into ISA 3.0, since it is supposed to be a win.  Do not add
>     FLOAT128_HW here until we are ready to make -mfloat128 on by default.  */
> -#define ISA_3_0_MASKS_SERVER	((ISA_2_7_MASKS_SERVER			\
> -				  | OPTION_MASK_ISEL			\
> -				  | OPTION_MASK_MODULO			\
> -				  | OPTION_MASK_P9_MINMAX		\
> -				  | OPTION_MASK_P9_MISC			\
> -				  | OPTION_MASK_P9_VECTOR)		\
> -				 & ~OTHER_FUSION_MASKS)
> +#define ISA_3_0_MASKS_SERVER	(ISA_2_7_MASKS_SERVER			\
> +				 | OPTION_MASK_ISEL			\
> +				 | OPTION_MASK_MODULO			\
> +				 | OPTION_MASK_P9_MINMAX		\
> +				 | OPTION_MASK_P9_MISC			\
> +				 | OPTION_MASK_P9_VECTOR)
> 
>  /* Support for the IEEE 128-bit floating point hardware requires a lot of the
>     VSX instructions that are part of ISA 3.0.  */
> @@ -140,7 +135,6 @@
>  				 | OPTION_MASK_MODULO			\
>  				 | OPTION_MASK_MULHW			\
>  				 | OPTION_MASK_NO_UPDATE		\
> -				 | OPTION_MASK_P8_FUSION		\
>  				 | OPTION_MASK_P8_VECTOR		\
>  				 | OPTION_MASK_P9_MINMAX		\
>  				 | OPTION_MASK_P9_MISC			\
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index ceaddafd33b..269c7510e3e 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -4040,41 +4040,6 @@ rs6000_option_override_internal (bool global_init_p)
>        && optimize_function_for_speed_p (cfun))
>      rs6000_isa_flags |= OPTION_MASK_SAVE_TOC_INDIRECT;
> 
> -  /* Enable power8 fusion if we are tuning for power8, even if we aren't
> -     generating power8 instructions.  Power9 does not optimize power8 fusion
> -     cases.  */
> -  if (!(rs6000_isa_flags_explicit & OPTION_MASK_P8_FUSION))
> -    {
> -      if (processor_target_table[tune_index].processor == PROCESSOR_POWER8)
> -	rs6000_isa_flags |= OPTION_MASK_P8_FUSION;
> -      else
> -	rs6000_isa_flags &= ~OPTION_MASK_P8_FUSION;
> -    }
> -
> -  /* Setting additional fusion flags turns on base fusion.  */
> -  if (!TARGET_P8_FUSION && TARGET_P8_FUSION_SIGN)
> -    {
> -      if (rs6000_isa_flags_explicit & OPTION_MASK_P8_FUSION)
> -	{
> -	  if (TARGET_P8_FUSION_SIGN)
> -	    error ("%qs requires %qs", "-mpower8-fusion-sign",
> -		   "-mpower8-fusion");
> -
> -	  rs6000_isa_flags &= ~OPTION_MASK_P8_FUSION;
> -	}
> -      else
> -	rs6000_isa_flags |= OPTION_MASK_P8_FUSION;
> -    }
> -
> -  /* Power8 does not fuse sign extended loads with the addis.  If we are
> -     optimizing at high levels for speed, convert a sign extended load into a
> -     zero extending load, and an explicit sign extension.  */
> -  if (TARGET_P8_FUSION
> -      && !(rs6000_isa_flags_explicit & OPTION_MASK_P8_FUSION_SIGN)
> -      && optimize_function_for_speed_p (cfun)
> -      && optimize >= 3)
> -    rs6000_isa_flags |= OPTION_MASK_P8_FUSION_SIGN;
> -
>    /* ISA 3.0 vector instructions include ISA 2.07.  */
>    if (TARGET_P9_VECTOR && !TARGET_P8_VECTOR)
>      {
> @@ -24010,8 +23975,6 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
>    { "pcrel-opt",		OPTION_MASK_PCREL_OPT,		false, true  },
>    { "popcntb",			OPTION_MASK_POPCNTB,		false, true  },
>    { "popcntd",			OPTION_MASK_POPCNTD,		false, true  },
> -  { "power8-fusion",		OPTION_MASK_P8_FUSION,		false, true  },
> -  { "power8-fusion-sign",	OPTION_MASK_P8_FUSION_SIGN,	false, true  },
>    { "power8-vector",		OPTION_MASK_P8_VECTOR,		false, true  },
>    { "power9-minmax",		OPTION_MASK_P9_MINMAX,		false, true  },
>    { "power9-misc",		OPTION_MASK_P9_MISC,		false, true  },
> @@ -24051,6 +24014,13 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
>  #endif
>    { "soft-float",		OPTION_MASK_SOFT_FLOAT,		false, false },
>    { "string",			0,				false, false },
> +
> +  /* The Power8 fusion option was removed.  We ignore using it in #pragma and
> +     attribute target.  Users may have used the options to suppress errors if
> +     they declare an inline function to be specifically power8 and the function
> +     was included by power9 or power10 which turned off the power8 fusion
> +     support.  */

"was removed" is a good detail for history, but possibly not required
here?    Maybe stl "The Power8 fusion option is a no-op".  ?


> +  { "power8-fusion",		0,				false, true  },
>  };
> 
>  /* Builtin mask mapping for printing the flags.  */
> @@ -24687,6 +24657,11 @@ rs6000_print_options_internal (FILE *file,
>        HOST_WIDE_INT mask = opts[i].mask;
>        size_t len = comma_len + prefix_len + strlen (name);
> 
> +      /* Don't print options that exist for backwards compatibility, but are
> +	 ignored now like -mpower8-fusion.  */
> +      if (!mask)
> +	continue;

I'm not sure calling out the -mpower8-fusion option here is necessary. 
perhaps s/ignored now like -mpower8-fusion/masked out./



> +
>        if (!invert)
>  	{
>  	  if ((flags & mask) == 0)
> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
> index 523256a5c9d..52a29e1c702 100644
> --- a/gcc/config/rs6000/rs6000.h
> +++ b/gcc/config/rs6000/rs6000.h
> @@ -504,6 +504,18 @@ extern int rs6000_vector_align[];
>  #define TARGET_MINMAX	(TARGET_HARD_FLOAT && TARGET_PPC_GFXOPT		\
>  			 && (TARGET_P9_MINMAX || !flag_trapping_math))
> 
> +/* Power8 has special fusion operations that are enabled if we are tuning for
> +   power8.  This used to be settable with an option (-mpower8-fusion), but that
> +   option has been removed.  */

s/has been removed/is now ignored/  ?


> +#define TARGET_P8_FUSION	(rs6000_tune == PROCESSOR_POWER8)
> +
> +/* Power8 fusion does not fuse loads with sign extends.  If we are not
> +   optimizing for space, split loads with sign extension to loads with zero
> +   extension and an explicit sign extend operation, so that the zero extending
> +   load can be fused.  */
> +#define TARGET_P8_FUSION_SIGN	(TARGET_P8_FUSION			\
> +				 && !optimize_function_for_size_p (cfun))
> +
>  /* In switching from using target_flags to using rs6000_isa_flags, the options
>     machinery creates OPTION_MASK_<xxx> instead of MASK_<xxx>.  For now map
>     OPTION_MASK_<xxx> back into MASK_<xxx>.  */
> @@ -517,7 +529,6 @@ extern int rs6000_vector_align[];
>  #define MASK_FLOAT128_KEYWORD		OPTION_MASK_FLOAT128_KEYWORD
>  #define MASK_FLOAT128_HW		OPTION_MASK_FLOAT128_HW
>  #define MASK_FPRND			OPTION_MASK_FPRND
> -#define MASK_P8_FUSION			OPTION_MASK_P8_FUSION
>  #define MASK_HARD_FLOAT			OPTION_MASK_HARD_FLOAT
>  #define MASK_HTM			OPTION_MASK_HTM
>  #define MASK_ISEL			OPTION_MASK_ISEL
> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
> index 4931d781c4e..6c4caf4c9ee 100644
> --- a/gcc/config/rs6000/rs6000.opt
> +++ b/gcc/config/rs6000/rs6000.opt
> @@ -474,13 +474,13 @@ Save the TOC in the prologue for indirect calls rather than inline.
>  mvsx-timode
>  Target RejectNegative Undocumented Ignore
> 
> +; The -mpower8-fusion and -mpower8-fusion-sign options existed in the past, but
> +; they are ignored now.

If the comment is necessary (it may be redundant with the Ignore
attribute in place)  I suggest  s/existed in the past, but they//

>  mpower8-fusion
> -Target Mask(P8_FUSION) Var(rs6000_isa_flags)
> -Fuse certain integer operations together for better performance on power8.
> +Target Undocumented Ignore
> 
>  mpower8-fusion-sign
> -Target Undocumented Mask(P8_FUSION_SIGN) Var(rs6000_isa_flags)
> -Allow sign extension in fusion operations.
> +Target Undocumented Ignore
> 
>  mpower8-vector
>  Target Mask(P8_VECTOR) Var(rs6000_isa_flags)
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 1a51759e6e4..7c21779e3b2 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -1266,8 +1266,7 @@ See RS/6000 and PowerPC Options.
>  -mveclibabi=@var{type}  -mfriz  -mno-friz @gol
>  -mpointers-to-nested-functions  -mno-pointers-to-nested-functions @gol
>  -msave-toc-indirect  -mno-save-toc-indirect @gol
> --mpower8-fusion  -mno-mpower8-fusion  -mpower8-vector  -mno-power8-vector @gol
> --mcrypto  -mno-crypto  -mhtm  -mno-htm @gol
> +-mpower8-vector  -mno-power8-vector -mcrypto  -mno-crypto  -mhtm  -mno-htm @gol
>  -mquad-memory  -mno-quad-memory @gol
>  -mquad-memory-atomic  -mno-quad-memory-atomic @gol
>  -mcompat-align-parm  -mno-compat-align-parm @gol
> @@ -28355,7 +28354,7 @@ following options:
>  -mpopcntb  -mpopcntd  -mpowerpc64 @gol
>  -mpowerpc-gpopt  -mpowerpc-gfxopt @gol
>  -mmulhw  -mdlmzb  -mmfpgpr  -mvsx @gol
> --mcrypto  -mhtm  -mpower8-fusion  -mpower8-vector @gol
> +-mcrypto  -mhtm  -mpower8-vector @gol
>  -mquad-memory  -mquad-memory-atomic  -mfloat128 @gol
>  -mfloat128-hardware -mprefixed -mpcrel -mmma @gol
>  -mrop-protect}
> @@ -28470,14 +28469,6 @@ Enable (disable) the use of the built-in functions that allow direct
>  access to the Hardware Transactional Memory (HTM) instructions that
>  were added in version 2.07 of the PowerPC ISA.
> 
> -@item -mpower8-fusion
> -@itemx -mno-power8-fusion
> -@opindex mpower8-fusion
> -@opindex mno-power8-fusion
> -Generate code that keeps (does not keeps) some integer operations
> -adjacent so that the instructions can be fused together on power8 and
> -later processors.
> -
>  @item -mpower8-vector
>  @itemx -mno-power8-vector
>  @opindex mpower8-vector
> diff --git a/gcc/testsuite/gcc.dg/lto/pr102059-1_0.c b/gcc/testsuite/gcc.dg/lto/pr102059-1_0.c
> index e1004f5cfbf..b215b701097 100644
> --- a/gcc/testsuite/gcc.dg/lto/pr102059-1_0.c
> +++ b/gcc/testsuite/gcc.dg/lto/pr102059-1_0.c
> @@ -1,7 +1,7 @@
>  /* { dg-lto-do link } */
>  /* { dg-skip-if "power10 and above only" { ! { power10_ok } } } */
>  /* -Wno-attributes suppresses always_inline warnings.  */
> -/* { dg-lto-options { "-O2 -mdejagnu-cpu=power8 -flto -Wno-attributes -mno-power8-fusion" } } */
> +/* { dg-lto-options { "-O2 -mdejagnu-cpu=power8 -flto -Wno-attributes" } } */
> 
>  int __attribute__ ((always_inline))
>  foo1 (int *b)
> diff --git a/gcc/testsuite/gcc.dg/lto/pr102059-2_0.c b/gcc/testsuite/gcc.dg/lto/pr102059-2_0.c
> index ebb0a7b8fa1..ffab23ab7e1 100644
> --- a/gcc/testsuite/gcc.dg/lto/pr102059-2_0.c
> +++ b/gcc/testsuite/gcc.dg/lto/pr102059-2_0.c
> @@ -1,6 +1,6 @@
>  /* { dg-lto-do link } */
>  /* { dg-skip-if "power10 and above only" { ! { power10_ok } } } */
> -/* { dg-lto-options { "-O2 -mdejagnu-cpu=power8 -mno-power8-fusion -flto -fdump-ipa-inline" } } */
> +/* { dg-lto-options { "-O2 -mdejagnu-cpu=power8 -flto -fdump-ipa-inline" } } */
> 
>  int
>  foo1 (int *b)
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr102059-3.c b/gcc/testsuite/gcc.target/powerpc/pr102059-3.c
> index 21c982d93f0..0cb3a4cb9f9 100644
> --- a/gcc/testsuite/gcc.target/powerpc/pr102059-3.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr102059-3.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-O2 -mdejagnu-cpu=power8 -mno-power8-fusion -fdump-tree-einline-optimized" } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power8 -fdump-tree-einline-optimized" } */
> 
>  /* Like pr102059-1.c, to verify the inlining still happens
>     even without always_inline attribute.  */
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr102059-4.c b/gcc/testsuite/gcc.target/powerpc/pr102059-4.c
> new file mode 100644
> index 00000000000..5fe66f8af4b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr102059-4.c
> @@ -0,0 +1,23 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10" } */
> +/* { dg-require-effective-target power10_ok } */

I think for coverage, this test could actually target power9_ok, but
sufficient as is. 


lgtm,
thanks
-Will


> +
> +/* Verify that power10 can explicity include functions compiled for power8.
> +   The issue was -mcpu=power8 enables -mpower8-fusion, but -mcpu=power9 or
> +   -mcpu=power10 do not set power8-fusion by default.  Thus when doing this
> +   compilation, they would get an error that the inline function failed in its
> +   inlining due to having incompatible options.  */
> +
> +static inline int __attribute__ ((always_inline,target("cpu=power8")))
> +foo (int *b)
> +{
> +  *b += 10;
> +  return *b;
> +}
> +
> +int
> +bar (int *a)
> +{
> +  *a = foo (a);
> +  return 0;
> +}
> -- 
> 2.35.1
> 
>
  

Ping patch.  The customer really needs this patch.  We need to apply it to the
trunk, and then I will have to refactor it for GCC 10 that the customer is
using.

| Date: Tue, 12 Apr 2022 21:14:55 -0400
| From: Michael Meissner <meissner@linux.ibm.com>
| Subject: [PATCH, V4] Eliminate power8 fusion options, use power8 tuning, PR target/102059
| Message-ID: <YlYkDy9LVeN2LfZ6@toto.the-meissners.org>

https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593153.html
  

Ping #4:

| Date: Tue, 12 Apr 2022 21:14:55 -0400
| From: Michael Meissner <meissner@linux.ibm.com>
| Subject: [PATCH, V4] Eliminate power8 fusion options, use power8 tuning, PR target/102059
| Message-ID: <YlYkDy9LVeN2LfZ6@toto.the-meissners.org>

https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593153.html
  

Ping #5:

| Date: Tue, 12 Apr 2022 21:14:55 -0400
| From: Michael Meissner <meissner@linux.ibm.com>
| Subject: [PATCH, V4] Eliminate power8 fusion options, use power8 tuning, PR target/102059
| Message-ID: <YlYkDy9LVeN2LfZ6@toto.the-meissners.org>

https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593153.html

We really need closure on this so I can do the backport to GCC 10 that the
customer is asking for.
  

On 5/2/22 8:06 PM, Michael Meissner wrote:
> Ping #5:
> 
> | Date: Tue, 12 Apr 2022 21:14:55 -0400
> | From: Michael Meissner <meissner@linux.ibm.com>
> | Subject: [PATCH, V4] Eliminate power8 fusion options, use power8 tuning, PR target/102059
> | Message-ID: <YlYkDy9LVeN2LfZ6@toto.the-meissners.org>
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593153.html
> 
> We really need closure on this so I can do the backport to GCC 10 that the
> customer is asking for.
> 
If we cannot get this in soonish, maybe we can at least get approval for
applying Mike's simpler patch to the release branches, specifically GCC 10?

   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102059#c31


Peter
  

On Tue, Apr 12, 2022 at 09:14:55PM -0400, Michael Meissner wrote:
> This is V4 of the patch.  Compared to V3 of the patch, GCC will just
> ignore -m{,no-}power8-fusion and -m{,no-}power8-fusion-sign.

But incorrectly :-(

> The splitting of signed halfword and word loads into unsigned load and
> sign extension is now suppressed with -Os, but it is done normally if we
> are not optimizing for space.

I have no idea what that means.  Other than that I asked to remove that.

> This code makes the -mpower8-fusion option a nop.  It is accepted without
> warning, but it does nothing.  Power8 fusion is only enabled if we are tuning
> for a power8.

It should *delete* the option, and have
;; This option existed in the past, but now is always off.
mno-power8-fusion
Target RejectNegative Undocumented Ignore

> The undocumented -mpower8-fusion-sign option is also made into a nop.

That one should be deleted.

> +  /* The Power8 fusion option was removed.  We ignore using it in #pragma and
> +     attribute target.  Users may have used the options to suppress errors if
> +     they declare an inline function to be specifically power8 and the function
> +     was included by power9 or power10 which turned off the power8 fusion
> +     support.  */
> +  { "power8-fusion",		0,				false, true  },

What does the comment mean?

> +      /* Don't print options that exist for backwards compatibility, but are
> +	 ignored now like -mpower8-fusion.  */
> +      if (!mask)
> +	continue;

No.  Such options should not be in the mask at all.

> +/* Power8 has special fusion operations that are enabled if we are tuning for
> +   power8.  This used to be settable with an option (-mpower8-fusion), but that
> +   option has been removed.  */
> +#define TARGET_P8_FUSION	(rs6000_tune == PROCESSOR_POWER8)

The plan was to not have p8 fusion at all.  GCC never implemented any of
the more useful p8 fusion things anyway, and those were only marginally
beneficial anyway.

> +/* Power8 fusion does not fuse loads with sign extends.  If we are not
> +   optimizing for space, split loads with sign extension to loads with zero
> +   extension and an explicit sign extend operation, so that the zero extending
> +   load can be fused.  */
> +#define TARGET_P8_FUSION_SIGN	(TARGET_P8_FUSION			\
> +				 && !optimize_function_for_size_p (cfun))

As I said before, don't do this.  Just remove the whole thing.

> +; The -mpower8-fusion and -mpower8-fusion-sign options existed in the past, but
> +; they are ignored now.

Don't put them together.  It is much easier for everything if they are
separate, boring, and exactly like everything else.

>  mpower8-fusion
> -Target Mask(P8_FUSION) Var(rs6000_isa_flags)
> -Fuse certain integer operations together for better performance on power8.
> +Target Undocumented Ignore

It should be deleted, instead, and be replaced with

;; This option existed in the past, but now is always off.
mno-power8-fusion
Target RejectNegative Undocumented Ignore

mpower8-fusion
Target RejectNegative Undocumented WarnRemoved

i.e. just like all other removed flags.  If someone explicitly tries to
enable it he/she *should* get a warning.

>  mpower8-fusion-sign
> -Target Undocumented Mask(P8_FUSION_SIGN) Var(rs6000_isa_flags)
> -Allow sign extension in fusion operations.
> +Target Undocumented Ignore

And this one should be completely removed, since no one ever used it.


Segher
  

On Thu, May 05, 2022 at 01:59:05PM -0500, Peter Bergner wrote:
> If we cannot get this in soonish, maybe we can at least get approval for
> applying Mike's simpler patch to the release branches, specifically GCC 10?
> 
>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102059#c31

Just an unconditional

  callee_isa &= ~OPTION_MASK_P8_FUSION;
  explicit_isa &= ~OPTION_MASK_P8_FUSION;

will do, no?  That is fine since these options should never have been
used to determine if anything can be inlined, in the first place.

A patch like that is pre-approved, even for trunk.

Thanks,


Segher
  

On 5/5/22 2:35 PM, Segher Boessenkool wrote:
> On Thu, May 05, 2022 at 01:59:05PM -0500, Peter Bergner wrote:
>> If we cannot get this in soonish, maybe we can at least get approval for
>> applying Mike's simpler patch to the release branches, specifically GCC 10?
>>
>>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102059#c31
> 
> Just an unconditional
> 
>   callee_isa &= ~OPTION_MASK_P8_FUSION;
>   explicit_isa &= ~OPTION_MASK_P8_FUSION;
> 
> will do, no?  That is fine since these options should never have been
> used to determine if anything can be inlined, in the first place.
>
> A patch like that is pre-approved, even for trunk.

That works for me!  I will apply this directly to GCC 10 and regtest and
push if clean so we can unblock our customer.

As for trunk, GCC 12 & 11, I think we can wait for the backport of Mike's
patch that removes the option altogether.

Peter
  

On Thu, May 05, 2022 at 02:12:43PM -0500, Segher Boessenkool wrote:
> On Tue, Apr 12, 2022 at 09:14:55PM -0400, Michael Meissner wrote:
> > This is V4 of the patch.  Compared to V3 of the patch, GCC will just
> > ignore -m{,no-}power8-fusion and -m{,no-}power8-fusion-sign.
> 
> But incorrectly :-(
> 
> > The splitting of signed halfword and word loads into unsigned load and
> > sign extension is now suppressed with -Os, but it is done normally if we
> > are not optimizing for space.
> 
> I have no idea what that means.  Other than that I asked to remove that.
> 
> > This code makes the -mpower8-fusion option a nop.  It is accepted without
> > warning, but it does nothing.  Power8 fusion is only enabled if we are tuning
> > for a power8.
> 
> It should *delete* the option, and have
> ;; This option existed in the past, but now is always off.
> mno-power8-fusion
> Target RejectNegative Undocumented Ignore
> 
> > The undocumented -mpower8-fusion-sign option is also made into a nop.
> 
> That one should be deleted.

Sure, but note the customer that asked for the patch, explicitly is using
-mno-power8-fusion.  I don't want to break their makefiles.

> 
> > +  /* The Power8 fusion option was removed.  We ignore using it in #pragma and
> > +     attribute target.  Users may have used the options to suppress errors if
> > +     they declare an inline function to be specifically power8 and the function
> > +     was included by power9 or power10 which turned off the power8 fusion
> > +     support.  */
> > +  { "power8-fusion",		0,				false, true  },
> 
> What does the comment mean?

One of the suggestions that I made to the customer was to change their code
from:

static inline int
__attribute__ ((always_inline,target("cpu=power8")))
foo (..)
{
  ...
}

to:

static inline int
__attribute__ ((always_inline,target("cpu=power8,no-power8-fusion")))
foo (...)
{
  ...
}

If they used this, it would avoid the issue, and not need to use a new switch.
Whether they used it, I don't know.  Whether some other customer who ran into
the problem used it, again I don't know.  If we remove the support for it in
target pragma and attribute, we can break code.

> 
> > +      /* Don't print options that exist for backwards compatibility, but are
> > +	 ignored now like -mpower8-fusion.  */
> > +      if (!mask)
> > +	continue;
> 
> No.  Such options should not be in the mask at all.

It is just to support ignoring setting no-power8-fusion in the target pragma
and attribute options.
> 
> > +/* Power8 has special fusion operations that are enabled if we are tuning for
> > +   power8.  This used to be settable with an option (-mpower8-fusion), but that
> > +   option has been removed.  */
> > +#define TARGET_P8_FUSION	(rs6000_tune == PROCESSOR_POWER8)
> 
> The plan was to not have p8 fusion at all.  GCC never implemented any of
> the more useful p8 fusion things anyway, and those were only marginally
> beneficial anyway.

No, no, no, no.  That is incorrect.

In the power8 days, we (mostly me) specifically spent a lot of time to add
fusion support.  I ultimately ran out of time in the initial GCC release to do
all of the optimizations planned.  Later, when it became apparent that power9
would not implement this fusion (origianlly it had been in the plan), the code
kind of withered, and it left some warts.

In particular, the fusion work was presented at the 2014 Gnu Cauldron (slides
24-27 in my deck) among the other power8 changes.

And by the way, we likely will have a similar issue with power10 fusion.  I
specifically have not done anything for that to avoid clouding the issue for
this bug.  But I imagine we may need to look at that in the future.

> 
> > +/* Power8 fusion does not fuse loads with sign extends.  If we are not
> > +   optimizing for space, split loads with sign extension to loads with zero
> > +   extension and an explicit sign extend operation, so that the zero extending
> > +   load can be fused.  */
> > +#define TARGET_P8_FUSION_SIGN	(TARGET_P8_FUSION			\
> > +				 && !optimize_function_for_size_p (cfun))
> 
> As I said before, don't do this.  Just remove the whole thing.
> 
> > +; The -mpower8-fusion and -mpower8-fusion-sign options existed in the past, but
> > +; they are ignored now.
> 
> Don't put them together.  It is much easier for everything if they are
> separate, boring, and exactly like everything else.
> 
> >  mpower8-fusion
> > -Target Mask(P8_FUSION) Var(rs6000_isa_flags)
> > -Fuse certain integer operations together for better performance on power8.
> > +Target Undocumented Ignore
> 
> It should be deleted, instead, and be replaced with
> 
> ;; This option existed in the past, but now is always off.
> mno-power8-fusion
> Target RejectNegative Undocumented Ignore
> 
> mpower8-fusion
> Target RejectNegative Undocumented WarnRemoved
> 
> i.e. just like all other removed flags.  If someone explicitly tries to
> enable it he/she *should* get a warning.
> 
> >  mpower8-fusion-sign
> > -Target Undocumented Mask(P8_FUSION_SIGN) Var(rs6000_isa_flags)
> > -Allow sign extension in fusion operations.
> > +Target Undocumented Ignore
> 
> And this one should be completely removed, since no one ever used it.

Well like all tuning flags, it was used quite a lot when I was doing the
initial work.
  

On Thu, May 05, 2022 at 02:59:07PM -0500, Peter Bergner wrote:
> On 5/5/22 2:35 PM, Segher Boessenkool wrote:
> > On Thu, May 05, 2022 at 01:59:05PM -0500, Peter Bergner wrote:
> >> If we cannot get this in soonish, maybe we can at least get approval for
> >> applying Mike's simpler patch to the release branches, specifically GCC 10?
> >>
> >>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102059#c31
> > 
> > Just an unconditional
> > 
> >   callee_isa &= ~OPTION_MASK_P8_FUSION;
> >   explicit_isa &= ~OPTION_MASK_P8_FUSION;
> > 
> > will do, no?  That is fine since these options should never have been
> > used to determine if anything can be inlined, in the first place.
> >
> > A patch like that is pre-approved, even for trunk.
> 
> That works for me!  I will apply this directly to GCC 10 and regtest and
> push if clean so we can unblock our customer.
> 
> As for trunk, GCC 12 & 11, I think we can wait for the backport of Mike's
> patch that removes the option altogether.

Please put it on trunk and 12 and 11 as well.  To keep things sane.

Thanks,


Segher
  

On 5/5/22 4:27 PM, Segher Boessenkool wrote:
> On Thu, May 05, 2022 at 02:59:07PM -0500, Peter Bergner wrote:
>> On 5/5/22 2:35 PM, Segher Boessenkool wrote:
>>> A patch like that is pre-approved, even for trunk.
>>
>> That works for me!  I will apply this directly to GCC 10 and regtest and
>> push if clean so we can unblock our customer.
>>
>> As for trunk, GCC 12 & 11, I think we can wait for the backport of Mike's
>> patch that removes the option altogether.
> 
> Please put it on trunk and 12 and 11 as well.  To keep things sane.

Will do.

Peter
  

On Thu, May 05, 2022 at 04:21:21PM -0400, Michael Meissner wrote:
> On Thu, May 05, 2022 at 02:12:43PM -0500, Segher Boessenkool wrote:
> > On Tue, Apr 12, 2022 at 09:14:55PM -0400, Michael Meissner wrote:
> > > This is V4 of the patch.  Compared to V3 of the patch, GCC will just
> > > ignore -m{,no-}power8-fusion and -m{,no-}power8-fusion-sign.
> > 
> > But incorrectly :-(
> > 
> > > The splitting of signed halfword and word loads into unsigned load and
> > > sign extension is now suppressed with -Os, but it is done normally if we
> > > are not optimizing for space.
> > 
> > I have no idea what that means.  Other than that I asked to remove that.
> > 
> > > This code makes the -mpower8-fusion option a nop.  It is accepted without
> > > warning, but it does nothing.  Power8 fusion is only enabled if we are tuning
> > > for a power8.
> > 
> > It should *delete* the option, and have
> > ;; This option existed in the past, but now is always off.
> > mno-power8-fusion
> > Target RejectNegative Undocumented Ignore
> > 
> > > The undocumented -mpower8-fusion-sign option is also made into a nop.
> > 
> > That one should be deleted.
> 
> Sure, but note the customer that asked for the patch, explicitly is using
> -mno-power8-fusion.  I don't want to break their makefiles.

-mno-power8-fusion is retained, and ignored.  -mpower8-fusion is
retained (but as separate option), ignored, and warned for.  Both
-mpower8-fusion-sign and -mno-power8-fusion-sign can be removed, since
nothing uses those options.

> > > +  /* The Power8 fusion option was removed.  We ignore using it in #pragma and
> > > +     attribute target.  Users may have used the options to suppress errors if
> > > +     they declare an inline function to be specifically power8 and the function
> > > +     was included by power9 or power10 which turned off the power8 fusion
> > > +     support.  */
> > > +  { "power8-fusion",		0,				false, true  },
> > 
> > What does the comment mean?
> 
> One of the suggestions that I made to the customer was to change their code
> from:
> 
> static inline int
> __attribute__ ((always_inline,target("cpu=power8")))
> foo (..)
> {
>   ...
> }
> 
> to:
> 
> static inline int
> __attribute__ ((always_inline,target("cpu=power8,no-power8-fusion")))
> foo (...)
> {
>   ...
> }
> 
> If they used this, it would avoid the issue, and not need to use a new switch.
> Whether they used it, I don't know.  Whether some other customer who ran into
> the problem used it, again I don't know.  If we remove the support for it in
> target pragma and attribute, we can break code.

There is still a -mno-power8-fusion option after you implemented what I
suggested all of this time.  This code will still just work, no?

> > > +      /* Don't print options that exist for backwards compatibility, but are
> > > +	 ignored now like -mpower8-fusion.  */
> > > +      if (!mask)
> > > +	continue;
> > 
> > No.  Such options should not be in the mask at all.
> 
> It is just to support ignoring setting no-power8-fusion in the target pragma
> and attribute options.

If you need to do that separately, there is something else wrong.  And
even then, if you need to do it manually, you should *do* it manually,
not make all kinds of coplications all over the place.

> > > +/* Power8 has special fusion operations that are enabled if we are tuning for
> > > +   power8.  This used to be settable with an option (-mpower8-fusion), but that
> > > +   option has been removed.  */
> > > +#define TARGET_P8_FUSION	(rs6000_tune == PROCESSOR_POWER8)
> > 
> > The plan was to not have p8 fusion at all.  GCC never implemented any of
> > the more useful p8 fusion things anyway, and those were only marginally
> > beneficial anyway.
> 
> No, no, no, no.  That is incorrect.

It was the plan I agreed with.

> And by the way, we likely will have a similar issue with power10 fusion.  I
> specifically have not done anything for that to avoid clouding the issue for
> this bug.  But I imagine we may need to look at that in the future.

Whether anything is fused or not should never be of any influence on
what is inlined or not in what or whatnot.  Fusion is comparable to
core-specific scheduling in almost all ways.

> > >  mpower8-fusion-sign
> > > -Target Undocumented Mask(P8_FUSION_SIGN) Var(rs6000_isa_flags)
> > > -Allow sign extension in fusion operations.
> > > +Target Undocumented Ignore
> > 
> > And this one should be completely removed, since no one ever used it.
> 
> Well like all tuning flags, it was used quite a lot when I was doing the
> initial work.

But it should arguably never have made it into any release branch.


Segher
  

On Thu, May 05, 2022 at 02:35:34PM -0500, Segher Boessenkool wrote:
> On Thu, May 05, 2022 at 01:59:05PM -0500, Peter Bergner wrote:
> > If we cannot get this in soonish, maybe we can at least get approval for
> > applying Mike's simpler patch to the release branches, specifically GCC 10?
> > 
> >    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102059#c31
> 
> Just an unconditional
> 
>   callee_isa &= ~OPTION_MASK_P8_FUSION;
>   explicit_isa &= ~OPTION_MASK_P8_FUSION;
> 
> will do, no?  That is fine since these options should never have been
> used to determine if anything can be inlined, in the first place.
> 
> A patch like that is pre-approved, even for trunk.

And as I said, logically we should do the same for p10 fusion.  I.e.

   callee_isa &= ~(OPTION_MASK_P8_FUSION
                   | OPTION_MASK_P10_FUSION);
   explicit_isa &= ~(OPTION_MASK_P8_FUSION
                    | OPTION_MASK_P10_FUSION);
  

On 5/5/22 5:51 PM, Michael Meissner wrote:
> On Thu, May 05, 2022 at 02:35:34PM -0500, Segher Boessenkool wrote>> A patch like that is pre-approved, even for trunk.
> 
> And as I said, logically we should do the same for p10 fusion.  I.e.
> 
>    callee_isa &= ~(OPTION_MASK_P8_FUSION
>                    | OPTION_MASK_P10_FUSION);
>    explicit_isa &= ~(OPTION_MASK_P8_FUSION
>                     | OPTION_MASK_P10_FUSION);
> 

I can add that to the simple patch while we wait for the bigger patch.

Peter
  

On 5/5/22 4:27 PM, Segher Boessenkool wrote:
> On Thu, May 05, 2022 at 02:59:07PM -0500, Peter Bergner wrote:
>> On 5/5/22 2:35 PM, Segher Boessenkool wrote:
>>> Just an unconditional
>>>
>>>   callee_isa &= ~OPTION_MASK_P8_FUSION;
>>>   explicit_isa &= ~OPTION_MASK_P8_FUSION;
>>>
>>> will do, no?  That is fine since these options should never have been
>>> used to determine if anything can be inlined, in the first place.
>>>
>>> A patch like that is pre-approved, even for trunk.
>>
>> That works for me!  I will apply this directly to GCC 10 and regtest and
>> push if clean so we can unblock our customer.
>>
>> As for trunk, GCC 12 & 11, I think we can wait for the backport of Mike's
>> patch that removes the option altogether.
> 
> Please put it on trunk and 12 and 11 as well.  To keep things sane.

Ok, pushed to trunk.  I'll work on testing and committing the backports.
Thanks!

Peter