Re: Repost [PATCH 1/6] Add -mcpu=future

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Mike,

on 2024/1/6 07:35, Michael Meissner wrote:
> This patch implements support for a potential future PowerPC cpu.  Features
> added with -mcpu=future, may or may not be added to new PowerPC processors.
> 
> This patch adds support for the -mcpu=future option.  If you use -mcpu=future,
> the macro __ARCH_PWR_FUTURE__ is defined, and the assembler .machine directive
> "future" is used.  Future patches in this series will add support for new
> instructions that may be present in future PowerPC processors.
> 
> This particular patch does not any new features.  It exists as a ground work
> for future patches to support for a possible PowerPC processor in the future.
> 
> This patch does not implement any differences in tuning when -mcpu=future is
> used compared to -mcpu=power10.  If -mcpu=future is used, GCC will use power10
> tuning.  If you explicitly use -mtune=future, you will get a warning that
> -mtune=future is not supported, and default tuning will be set for power10.
> 
> The patches have been tested on both little and big endian systems.  Can I check
> it into the master branch?
> 
> 2024-01-05   Michael Meissner  <meissner@linux.ibm.com>
> 
> gcc/
> 
> 	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Define
> 	__ARCH_PWR_FUTURE__ if -mcpu=future.
> 	* config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS): New macro.
> 	(POWERPC_MASKS): Add -mcpu=future support.
> 	* config/rs6000/rs6000-opts.h (enum processor_type): Add
> 	PROCESSOR_FUTURE.
> 	* config/rs6000/rs6000-tables.opt: Regenerate.
> 	* config/rs6000/rs6000.cc (rs600_cpu_index_lookup): New helper
> 	function.
> 	(rs6000_option_override_internal): Make -mcpu=future set
> 	-mtune=power10.  If the user explicitly uses -mtune=future, give a
> 	warning and reset the tuning to power10.
> 	(rs6000_option_override_internal): Use power10 costs for future
> 	machine.
> 	(rs6000_machine_from_flags): Add support for -mcpu=future.
> 	(rs6000_opt_masks): Likewise.
> 	* config/rs6000/rs6000.h (ASM_CPU_SUPPORT): Likewise.
> 	* config/rs6000/rs6000.md (cpu attribute): Likewise.
> 	* config/rs6000/rs6000.opt (-mfuture): New undocumented debug switch.
> 	* doc/invoke.texi (IBM RS/6000 and PowerPC Options): Document -mcpu=future.
> ---
>  gcc/config/rs6000/rs6000-c.cc       |  2 +
>  gcc/config/rs6000/rs6000-cpus.def   |  6 +++
>  gcc/config/rs6000/rs6000-opts.h     |  4 +-
>  gcc/config/rs6000/rs6000-tables.opt |  3 ++
>  gcc/config/rs6000/rs6000.cc         | 58 ++++++++++++++++++++++++-----
>  gcc/config/rs6000/rs6000.h          |  1 +
>  gcc/config/rs6000/rs6000.md         |  2 +-
>  gcc/config/rs6000/rs6000.opt        |  4 ++
>  gcc/doc/invoke.texi                 |  2 +-
>  9 files changed, 69 insertions(+), 13 deletions(-)
> 
> diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
> index ce0b14a8d37..f2fb5bef678 100644
> --- a/gcc/config/rs6000/rs6000-c.cc
> +++ b/gcc/config/rs6000/rs6000-c.cc
> @@ -447,6 +447,8 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR9");
>    if ((flags & OPTION_MASK_POWER10) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR10");
> +  if ((flags & OPTION_MASK_FUTURE) != 0)
> +    rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR_FUTURE");
>    if ((flags & OPTION_MASK_SOFT_FLOAT) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_SOFT_FLOAT");
>    if ((flags & OPTION_MASK_RECIP_PRECISION) != 0)
> diff --git a/gcc/config/rs6000/rs6000-cpus.def b/gcc/config/rs6000/rs6000-cpus.def
> index d28cc87eb2a..8754635f3d9 100644
> --- a/gcc/config/rs6000/rs6000-cpus.def
> +++ b/gcc/config/rs6000/rs6000-cpus.def
> @@ -88,6 +88,10 @@
>  				 | OPTION_MASK_POWER10			\
>  				 | OTHER_POWER10_MASKS)
>  
> +/* Flags for a potential future processor that may or may not be delivered.  */
> +#define ISA_FUTURE_MASKS	(ISA_3_1_MASKS_SERVER			\
> +				 | OPTION_MASK_FUTURE)
> +

Nit: Named as "ISA_FUTURE_MASKS_SERVER" seems more accurate as it's constituted
with ISA_3_1_MASKS_**SERVER** ...

>  /* Flags that need to be turned off if -mno-power9-vector.  */
>  #define OTHER_P9_VECTOR_MASKS	(OPTION_MASK_FLOAT128_HW		\
>  				 | OPTION_MASK_P9_MINMAX)
> @@ -135,6 +139,7 @@
>  				 | OPTION_MASK_LOAD_VECTOR_PAIR		\
>  				 | OPTION_MASK_POWER10			\
>  				 | OPTION_MASK_P10_FUSION		\
> +				 | OPTION_MASK_FUTURE			\
>  				 | OPTION_MASK_HTM			\
>  				 | OPTION_MASK_ISEL			\
>  				 | OPTION_MASK_MFCRF			\
> @@ -267,3 +272,4 @@ RS6000_CPU ("powerpc64", PROCESSOR_POWERPC64, OPTION_MASK_PPC_GFXOPT
>  RS6000_CPU ("powerpc64le", PROCESSOR_POWER8, MASK_POWERPC64
>  	    | ISA_2_7_MASKS_SERVER | OPTION_MASK_HTM)
>  RS6000_CPU ("rs64", PROCESSOR_RS64A, OPTION_MASK_PPC_GFXOPT | MASK_POWERPC64)
> +RS6000_CPU ("future", PROCESSOR_FUTURE, MASK_POWERPC64 | ISA_FUTURE_MASKS)

..., then this need to be updated accordingly.

> diff --git a/gcc/config/rs6000/rs6000-opts.h b/gcc/config/rs6000/rs6000-opts.h
> index 33fd0efc936..25890ae3034 100644
> --- a/gcc/config/rs6000/rs6000-opts.h
> +++ b/gcc/config/rs6000/rs6000-opts.h
> @@ -67,7 +67,9 @@ enum processor_type
>     PROCESSOR_MPCCORE,
>     PROCESSOR_CELL,
>     PROCESSOR_PPCA2,
> -   PROCESSOR_TITAN
> +   PROCESSOR_TITAN,
> +

Nit: unintentional empty line?

> +   PROCESSOR_FUTURE
>  };
>  
>  
> diff --git a/gcc/config/rs6000/rs6000-tables.opt b/gcc/config/rs6000/rs6000-tables.opt
> index 65f46709716..97fa98a2e65 100644
> --- a/gcc/config/rs6000/rs6000-tables.opt
> +++ b/gcc/config/rs6000/rs6000-tables.opt
> @@ -197,3 +197,6 @@ Enum(rs6000_cpu_opt_value) String(powerpc64le) Value(55)
>  EnumValue
>  Enum(rs6000_cpu_opt_value) String(rs64) Value(56)
>  
> +EnumValue
> +Enum(rs6000_cpu_opt_value) String(future) Value(57)
> +
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 5a7e00b03d1..bc509399cf6 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -1809,6 +1809,18 @@ rs6000_cpu_name_lookup (const char *name)
>    return -1;
>  }
>  
> +/* Look up the index for a specific processor.  */
> +
> +static int
> +rs600_cpu_index_lookup (enum processor_type processor)

s/rs600_cpu_index_lookup/rs6000_cpu_index_lookup/

> +{
> +  for (size_t i = 0; i < ARRAY_SIZE (processor_target_table); i++)
> +    if (processor_target_table[i].processor == processor)
> +      return i;
> +
> +  return -1;
> +}

Nit: Since this is given with a valid enum processor_type, I think it should
never return -1?  If so, may be more clear with gcc_unreachable () or adjust
with initial -1, break when hits and assert it's not -1.

> +
>  \f
>  /* Return number of consecutive hard regs needed starting at reg REGNO
>     to hold something of mode MODE.
> @@ -3756,23 +3768,45 @@ rs6000_option_override_internal (bool global_init_p)
>      rs6000_isa_flags &= ~OPTION_MASK_POWERPC64;
>  #endif
>  
> +  /* At the moment, we don't have explict -mtune=future support.  If the user

Nit: s/explict/explicit/

> +     explicitly tried to use -mtune=future, give a warning.  If not, use the

Nit: s/tried/tries/?

> +     power10 tuning until future tuning is added.  */
>    if (rs6000_tune_index >= 0)
> -    tune_index = rs6000_tune_index;
> +    {
> +      enum processor_type cur_proc
> +	= processor_target_table[rs6000_tune_index].processor;
> +
> +      if (cur_proc == PROCESSOR_FUTURE)
> +	{
> +	  static bool issued_future_tune_warning = false;
> +	  if (!issued_future_tune_warning)
> +	    {
> +	      issued_future_tune_warning = true;

This seems to ensure we only warn this once, but I noticed that in rs6000/
only some OPT_Wpsabi related warnings adopt this way, I wonder if we don't
restrict it like this, for a tiny simple case, how many times it would warn?

> +	      warning (0, "%qs is not currently supported", "-mtune=future");
> +	    }
> +> +	  rs6000_tune_index = rs600_cpu_index_lookup (PROCESSOR_POWER10);
> +	}
> +      tune_index = rs6000_tune_index;
> +    }
>    else if (cpu_index >= 0)
> -    rs6000_tune_index = tune_index = cpu_index;
> +    {
> +      enum processor_type cur_cpu
> +	= processor_target_table[cpu_index].processor;
> +
> +      rs6000_tune_index = tune_index
> +	= (cur_cpu == PROCESSOR_FUTURE
> +	   ? rs600_cpu_index_lookup (PROCESSOR_POWER10)

s/rs600_cpu_index_lookup/rs6000_cpu_index_lookup/

> +	   : cpu_index);
> +    }
>    else
>      {
> -      size_t i;
>        enum processor_type tune_proc
>  	= (TARGET_POWERPC64 ? PROCESSOR_DEFAULT64 : PROCESSOR_DEFAULT);
>  
> -      tune_index = -1;
> -      for (i = 0; i < ARRAY_SIZE (processor_target_table); i++)
> -	if (processor_target_table[i].processor == tune_proc)
> -	  {
> -	    tune_index = i;
> -	    break;
> -	  }
> +      tune_index = rs600_cpu_index_lookup (tune_proc == PROCESSOR_FUTURE
> +					   ? PROCESSOR_POWER10
> +					   : tune_proc);

This part looks useless, as tune_proc is impossible to be PROCESSOR_FUTURE.

>      }

Maybe re-structure the above into:

bool explicit_tune = false;
if (rs6000_tune_index >= 0)
  {
    tune_index = rs6000_tune_index;
    explicit_tune = true;
  }
else if (cpu_index >= 0)
  // as before
  rs6000_tune_index = tune_index = cpu_index;
else
  {
   //as before
   ...
  }

// Check tune_index here instead.

if (processor_target_table[tune_index].processor == PROCESSOR_FUTURE)
  {
    tune_index = rs6000_cpu_index_lookup (PROCESSOR_POWER10);
    if (explicit_tune)
      warn ...
  }

// as before
rs6000_tune = processor_target_table[tune_index].processor;

>  
>    if (cpu_index >= 0)
> @@ -4785,6 +4819,7 @@ rs6000_option_override_internal (bool global_init_p)
>  	break;
>  
>        case PROCESSOR_POWER10:
> +      case PROCESSOR_FUTURE:
>  	rs6000_cost = &power10_cost;
>  	break;
>  
> @@ -5944,6 +5979,8 @@ rs6000_machine_from_flags (void)
>    /* Disable the flags that should never influence the .machine selection.  */
>    flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL);
>  
> +  if ((flags & (ISA_FUTURE_MASKS & ~ISA_3_1_MASKS_SERVER)) != 0)
> +    return "future";
>    if ((flags & (ISA_3_1_MASKS_SERVER & ~ISA_3_0_MASKS_SERVER)) != 0)
>      return "power10";
>    if ((flags & (ISA_3_0_MASKS_SERVER & ~ISA_2_7_MASKS_SERVER)) != 0)
> @@ -24500,6 +24537,7 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
>    { "float128-hardware",	OPTION_MASK_FLOAT128_HW,	false, true  },
>    { "fprnd",			OPTION_MASK_FPRND,		false, true  },
>    { "power10",			OPTION_MASK_POWER10,		false, true  },
> +  { "future",			OPTION_MASK_FUTURE,		false, true  },
>    { "hard-dfp",			OPTION_MASK_DFP,		false, true  },
>    { "htm",			OPTION_MASK_HTM,		false, true  },
>    { "isel",			OPTION_MASK_ISEL,		false, true  },
> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
> index 2291fe8d3a3..43209f9a6e7 100644
> --- a/gcc/config/rs6000/rs6000.h
> +++ b/gcc/config/rs6000/rs6000.h
> @@ -163,6 +163,7 @@
>    mcpu=e5500: -me5500; \
>    mcpu=e6500: -me6500; \
>    mcpu=titan: -mtitan; \
> +  mcpu=future: -mfuture; \
>    !mcpu*: %{mpower9-vector: -mpower9; \
>  	    mpower8-vector|mcrypto|mdirect-move|mhtm: -mpower8; \
>  	    mvsx: -mpower7; \

I think we should also update asm_names in driver-rs6000.cc.

The others look good to me, thanks!

BR,
Kewen

> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 969d34b69e6..a125fd8fc99 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -351,7 +351,7 @@ (define_attr "cpu"
>     ppc403,ppc405,ppc440,ppc476,
>     ppc8540,ppc8548,ppce300c2,ppce300c3,ppce500mc,ppce500mc64,ppce5500,ppce6500,
>     power4,power5,power6,power7,power8,power9,power10,
> -   rs64a,mpccore,cell,ppca2,titan"
> +   rs64a,mpccore,cell,ppca2,titan,future"
>    (const (symbol_ref "(enum attr_cpu) rs6000_tune")))
>  
>  ;; The ISA we implement.
> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
> index 60b923f5e4b..775ba830eac 100644
> --- a/gcc/config/rs6000/rs6000.opt
> +++ b/gcc/config/rs6000/rs6000.opt
> @@ -628,6 +628,10 @@ mieee128-constant
>  Target Var(TARGET_IEEE128_CONSTANT) Init(1) Save
>  Generate (do not generate) code that uses the LXVKQ instruction.
>  
> +mfuture
> +Target Undocumented Mask(FUTURE) Var(rs6000_isa_flags)
> +Generate (do not generate) future instructions.
> +
>  ; Documented parameters
>  
>  -param=rs6000-vect-unroll-limit=
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index d71583853f0..0e817ee923a 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -30423,7 +30423,7 @@ Supported values for @var{cpu_type} are @samp{401}, @samp{403},
>  @samp{titan}, @samp{power3}, @samp{power4}, @samp{power5}, @samp{power5+},
>  @samp{power6}, @samp{power6x}, @samp{power7}, @samp{power8},
>  @samp{power9}, @samp{power10}, @samp{powerpc}, @samp{powerpc64},
> -@samp{powerpc64le}, @samp{rs64}, and @samp{native}.
> +@samp{powerpc64le}, @samp{rs64}, @samp{future}, and @samp{native}.
>  
>  @option{-mcpu=powerpc}, @option{-mcpu=powerpc64}, and
>  @option{-mcpu=powerpc64le} specify pure 32-bit PowerPC (either