Re: [PATCH, rs6000] Additional cleanup of rs6000_builtin_mask

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

Hi Will,

Thanks for the cleanup!  Some comments are inlined.

on 2022/7/14 05:39, will schmidt wrote:
> [PATCH, rs6000] Additional cleanup of rs6000_builtin_mask
>     
> Hi,
>   Post the rs6000 builtins rewrite, some of the leftover builtin
> code is redundant and can be removed.
>   This replaces the remaining usage of bu_mask in
> rs6000_target_modify_macros() with checks against the rs6000_cpu directly.
> Thusly the bu_mask variable can be removed.  After that variable
> is eliminated there are no other uses of rs6000_builtin_mask_calculate(),
> so that function can also be safely removed.
> 

The TargetVariable rs6000_builtin_mask in rs6000.opt is useless, it seems
it can be removed together?

> I have tested this on current systems (P8,P9,P10) without regressions.
> 
> OK for trunk?
> 
> 
> Thanks,
> -Will
>     
> gcc/
> 	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros): Remove
> 	bu_mask references.  (rs6000_define_or_undefine_macro): Replace
> 	bu_mask reference with a rs6000_cpu value check.
> 	(rs6000_cpu_cpp_builtins): Remove rs6000_builtin_mask_calculate()
> 	parameter from call to rs6000_target_modify_macros.
> 	* config/rs6000/rs6000-protos.h (rs6000_target_modify_macros,
> 	rs6000_target_modify_macros_ptr): Remove parameter from extern
> 	for the prototype.
> 	* config/rs6000/rs6000.cc (rs6000_target_modify_macros_ptr): Remove
> 	parameter from prototype, update calls to this function.
> 	(rs6000_print_builtin_options): Remove prototype, call and function.
> 	(rs6000_builtin_mask_calculate): Remove function.
> 	(rs6000_debug_reg_global): Remove call to rs6000_print_builtin_options.
> 	(rs6000_option_override_internal): Remove rs6000_builtin_mask var
> 	and builtin_mask debug output.
> 	(rs6000_pragma_target_parse): Update calls to
> 	rs6000_target_modify_ptr.
> 
> 
> diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
> index 0d13645040ff..4d051b906582 100644
> --- a/gcc/config/rs6000/rs6000-c.cc
> +++ b/gcc/config/rs6000/rs6000-c.cc
> @@ -333,24 +333,20 @@ rs6000_define_or_undefine_macro (bool define_p, const char *name)
>    else
>      cpp_undef (parse_in, name);
>  }
>  
>  /* Define or undefine macros based on the current target.  If the user does
> -   #pragma GCC target, we need to adjust the macros dynamically.  Note, some of
> -   the options needed for builtins have been moved to separate variables, so
> -   have both the target flags and the builtin flags as arguments.  */
> +   #pragma GCC target, we need to adjust the macros dynamically.  */
>  
>  void
> -rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags,
> -			     HOST_WIDE_INT bu_mask)
> +rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags)
>  {
>    if (TARGET_DEBUG_BUILTIN || TARGET_DEBUG_TARGET)
>      fprintf (stderr,
> -	     "rs6000_target_modify_macros (%s, " HOST_WIDE_INT_PRINT_HEX
> -	     ", " HOST_WIDE_INT_PRINT_HEX ")\n",
> +	     "rs6000_target_modify_macros (%s, " HOST_WIDE_INT_PRINT_HEX ")\n",
>  	     (define_p) ? "define" : "undef",
> -	     flags, bu_mask);
> +	     flags);
>  
>    /* Each of the flags mentioned below controls whether certain
>       preprocessor macros will be automatically defined when
>       preprocessing source files for compilation by this compiler.
>       While most of these flags can be enabled or disabled
> @@ -593,14 +589,12 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags,
>    /* OPTION_MASK_FLOAT128_HARDWARE can be turned on if -mcpu=power9 is used or
>       via the target attribute/pragma.  */
>    if ((flags & OPTION_MASK_FLOAT128_HW) != 0)
>      rs6000_define_or_undefine_macro (define_p, "__FLOAT128_HARDWARE__");
>  
> -  /* options from the builtin masks.  */
> -  /* Note that OPTION_MASK_FPRND is enabled only if
> -     (rs6000_cpu == PROCESSOR_CELL) (e.g. -mcpu=cell).  */
> -  if ((bu_mask & OPTION_MASK_FPRND) != 0)
> +  /* Tell the user if we are targeting CELL.  */
> +  if (rs6000_cpu == PROCESSOR_CELL)
>      rs6000_define_or_undefine_macro (define_p, "__PPU__");
>  
>    /* Tell the user if we support the MMA instructions.  */
>    if ((flags & OPTION_MASK_MMA) != 0)
>      rs6000_define_or_undefine_macro (define_p, "__MMA__");
> @@ -614,12 +608,11 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags,
>  
>  void
>  rs6000_cpu_cpp_builtins (cpp_reader *pfile)
>  {
>    /* Define all of the common macros.  */
> -  rs6000_target_modify_macros (true, rs6000_isa_flags,
> -			       rs6000_builtin_mask_calculate ());
> +  rs6000_target_modify_macros (true, rs6000_isa_flags);
>  
>    if (TARGET_FRE)
>      builtin_define ("__RECIP__");
>    if (TARGET_FRES)
>      builtin_define ("__RECIPF__");
> diff --git a/gcc/config/rs6000/rs6000-protos.h b/gcc/config/rs6000/rs6000-protos.h
> index 3ea010236090..b3c16e7448d8 100644
> --- a/gcc/config/rs6000/rs6000-protos.h
> +++ b/gcc/config/rs6000/rs6000-protos.h
> @@ -318,13 +318,12 @@ extern void rs6000_pragma_longcall (struct cpp_reader *);
>  extern void rs6000_cpu_cpp_builtins (struct cpp_reader *);
>  #ifdef TREE_CODE
>  extern bool rs6000_pragma_target_parse (tree, tree);
>  #endif
>  extern void rs6000_activate_target_options (tree new_tree);
> -extern void rs6000_target_modify_macros (bool, HOST_WIDE_INT, HOST_WIDE_INT);
> -extern void (*rs6000_target_modify_macros_ptr) (bool, HOST_WIDE_INT,
> -						HOST_WIDE_INT);
> +extern void rs6000_target_modify_macros (bool, HOST_WIDE_INT);
> +extern void (*rs6000_target_modify_macros_ptr) (bool, HOST_WIDE_INT);
>  
>  /* Declare functions in rs6000-d.cc  */
>  extern void rs6000_d_target_versions (void);
>  extern void rs6000_d_register_target_info (void);
>  
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 3ff16b8ae04d..5fda18d82795 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -276,11 +276,11 @@ const char *tcb_verification_symbol = "__parse_hwcap_and_convert_at_platform";
>  bool cpu_builtin_p = false;
>  
>  /* Pointer to function (in rs6000-c.cc) that can define or undefine target
>     macros that have changed.  Languages that don't support the preprocessor
>     don't link in rs6000-c.cc, so we can't call it directly.  */
> -void (*rs6000_target_modify_macros_ptr) (bool, HOST_WIDE_INT, HOST_WIDE_INT);
> +void (*rs6000_target_modify_macros_ptr) (bool, HOST_WIDE_INT);
>  
>  /* Simplfy register classes into simpler classifications.  We assume
>     GPR_REG_TYPE - FPR_REG_TYPE are ordered so that we can use a simple range
>     check for standard register classes (gpr/floating/altivec/vsx) and
>     floating/vector classes (float/altivec/vsx).  */
> @@ -1169,12 +1169,10 @@ enum reg_class (*rs6000_preferred_reload_class_ptr) (rtx, enum reg_class)
>  
>  const int INSN_NOT_AVAILABLE = -1;
>  
>  static void rs6000_print_isa_options (FILE *, int, const char *,
>  				      HOST_WIDE_INT);
> -static void rs6000_print_builtin_options (FILE *, int, const char *,
> -					  HOST_WIDE_INT);
>  static HOST_WIDE_INT rs6000_disable_incompatible_switches (void);
>  
>  static enum rs6000_reg_type register_to_reg_type (rtx, bool *);
>  static bool rs6000_secondary_reload_move (enum rs6000_reg_type,
>  					  enum rs6000_reg_type,
> @@ -2405,13 +2403,10 @@ rs6000_debug_reg_global (void)
>  			    rs6000_isa_flags);
>  
>    rs6000_print_isa_options (stderr, 0, "rs6000_isa_flags_explicit",
>  			    rs6000_isa_flags_explicit);
>  
> -  rs6000_print_builtin_options (stderr, 0, "rs6000_builtin_mask",
> -				rs6000_builtin_mask);
> -
>    rs6000_print_isa_options (stderr, 0, "TARGET_DEFAULT", TARGET_DEFAULT);
>  
>    fprintf (stderr, DEBUG_FMT_S, "--with-cpu default",
>  	   OPTION_TARGET_CPU_DEFAULT ? OPTION_TARGET_CPU_DEFAULT : "<none>");
>  
> @@ -3370,45 +3365,10 @@ darwin_rs6000_override_options (void)
>  
>  #ifndef RS6000_DEFAULT_LONG_DOUBLE_SIZE
>  #define RS6000_DEFAULT_LONG_DOUBLE_SIZE 64
>  #endif
>  
> -/* Return the builtin mask of the various options used that could affect which
> -   builtins were used.  In the past we used target_flags, but we've run out of
> -   bits, and some options are no longer in target_flags.  */
> -
> -HOST_WIDE_INT
> -rs6000_builtin_mask_calculate (void)
> -{
> -  return (((TARGET_ALTIVEC)		    ? OPTION_MASK_ALTIVEC    : 0)
> -	  | ((TARGET_CMPB)		    ? OPTION_MASK_CMPB	     : 0)
> -	  | ((TARGET_VSX)		    ? OPTION_MASK_VSX	     : 0)
> -	  | ((TARGET_FRE)		    ? OPTION_MASK_POPCNTB    : 0)
> -	  | ((TARGET_FRES)		    ? OPTION_MASK_PPC_GFXOPT : 0)
> -	  | ((TARGET_FRSQRTE)		    ? OPTION_MASK_PPC_GFXOPT : 0)
> -	  | ((TARGET_FRSQRTES)		    ? OPTION_MASK_POPCNTB    : 0)
> -	  | ((TARGET_POPCNTD)		    ? OPTION_MASK_POPCNTD    : 0)
> -	  | ((rs6000_cpu == PROCESSOR_CELL) ? OPTION_MASK_FPRND      : 0)
> -	  | ((TARGET_P8_VECTOR)		    ? OPTION_MASK_P8_VECTOR  : 0)
> -	  | ((TARGET_P9_VECTOR)		    ? OPTION_MASK_P9_VECTOR  : 0)
> -	  | ((TARGET_P9_MISC)		    ? OPTION_MASK_P9_MISC    : 0)
> -	  | ((TARGET_MODULO)		    ? OPTION_MASK_MODULO     : 0)
> -	  | ((TARGET_64BIT)		    ? MASK_64BIT	     : 0)
> -	  | ((TARGET_POWERPC64)		    ? MASK_POWERPC64	     : 0)
> -	  | ((TARGET_CRYPTO)		    ? OPTION_MASK_CRYPTO     : 0)
> -	  | ((TARGET_HTM)		    ? OPTION_MASK_HTM	     : 0)
> -	  | ((TARGET_DFP)		    ? OPTION_MASK_DFP	     : 0)
> -	  | ((TARGET_HARD_FLOAT)	    ? OPTION_MASK_SOFT_FLOAT : 0)
> -	  | ((TARGET_LONG_DOUBLE_128
> -	      && TARGET_HARD_FLOAT
> -	      && !TARGET_IEEEQUAD)	    ? OPTION_MASK_MULTIPLE   : 0)
> -	  | ((TARGET_FLOAT128_TYPE)	    ? OPTION_MASK_FLOAT128_KEYWORD : 0)
> -	  | ((TARGET_FLOAT128_HW)	    ? OPTION_MASK_FLOAT128_HW : 0)
> -	  | ((TARGET_MMA)		    ? OPTION_MASK_MMA	     : 0)
> -	  | ((TARGET_POWER10)		    ? OPTION_MASK_POWER10    : 0));
> -}
> -
>  /* Implement TARGET_MD_ASM_ADJUST.  All asm statements are considered
>     to clobber the XER[CA] bit because clobbering that bit without telling
>     the compiler worked just fine with versions of GCC before GCC 5, and
>     breaking a lot of older code in ways that are hard to track down is
>     not such a great idea.  */
> @@ -3616,15 +3576,10 @@ glibc_supports_ieee_128bit (void)
>       compilation efforts.  This has the effect of also turning on the
>       associated TARGET_XXX values since these are macros which are
>       generally defined to test the corresponding bit of the
>       rs6000_isa_flags variable.
>  
> -     The variable rs6000_builtin_mask is set to represent the target
> -     options for the most current compilation efforts, consistent with
> -     the current contents of rs6000_isa_flags.  This variable controls
> -     expansion of built-in functions.
> -
>       Various other global variables and fields of global structures
>       (over 50 in all) are initialized to reflect the desired options
>       for the most current compilation efforts.  */
>  
>  static bool
> @@ -4888,18 +4843,10 @@ rs6000_option_override_internal (bool global_init_p)
>  	  else
>  	    rs6000_recip_control |= mask;
>  	}
>      }
>  
> -  /* Set the builtin mask of the various options used that could affect which
> -     builtins were used.  In the past we used target_flags, but we've run out
> -     of bits, and some options are no longer in target_flags.  */
> -  rs6000_builtin_mask = rs6000_builtin_mask_calculate ();
> -  if (TARGET_DEBUG_BUILTIN || TARGET_DEBUG_TARGET)
> -    rs6000_print_builtin_options (stderr, 0, "builtin mask",
> -				  rs6000_builtin_mask);
> -

I wonder if it's a good idea to still dump some information for built-in
functions debugging even with new bif framework, it can be handled in a
separated patch if yes.  The new bif framework adopts stanzas for bif
guarding, if we want to do similar things, we can refer to the code
like:

  if (!(e == ENB_ALWAYS
	|| (e == ENB_P5 && TARGET_POPCNTB)
	|| (e == ENB_P6 && TARGET_CMPB)
	|| (e == ENB_P6_64 && TARGET_CMPB && TARGET_POWERPC64)
	|| (e == ENB_ALTIVEC && TARGET_ALTIVEC)
	|| (e == ENB_CELL && TARGET_ALTIVEC && rs6000_cpu == PROCESSOR_CELL)
	|| (e == ENB_VSX && TARGET_VSX)
	|| (e == ENB_P7 && TARGET_POPCNTD)
	|| (e == ENB_P7_64 && TARGET_POPCNTD && TARGET_POWERPC64)
	|| (e == ENB_P8 && TARGET_DIRECT_MOVE)
	|| (e == ENB_P8V && TARGET_P8_VECTOR)
	|| (e == ENB_P9 && TARGET_MODULO)
	|| (e == ENB_P9_64 && TARGET_MODULO && TARGET_POWERPC64)
	|| (e == ENB_P9V && TARGET_P9_VECTOR)
	|| (e == ENB_IEEE128_HW && TARGET_FLOAT128_HW)
	|| (e == ENB_DFP && TARGET_DFP)
	|| (e == ENB_CRYPTO && TARGET_CRYPTO)
	|| (e == ENB_HTM && TARGET_HTM)
	|| (e == ENB_P10 && TARGET_POWER10)
	|| (e == ENB_P10_64 && TARGET_POWER10 && TARGET_POWERPC64)
	|| (e == ENB_MMA && TARGET_MMA)))
    {
      rs6000_invalid_builtin (fcode);
      return expand_call (exp, target, ignore);
    }

TARGET_POPCNTB means all bifs with ENB_P5 are available
TARGET_CMPB means all bifs with ENB_P6 are available
...

, dump information like "current enabled stanzas: ENB_xx, ENB_xxx, ..."
(even without ENB_ prefix).


>    /* Initialize all of the registers.  */
>    rs6000_init_hard_regno_mode_ok (global_init_p);
>  
>    /* Save the initial options in case the user does function specific options */
>    if (global_init_p)
> @@ -24495,17 +24442,15 @@ rs6000_pragma_target_parse (tree args, tree pop_target)
>  
>        if ((diff_flags != 0) || (diff_bumask != 0))
>  	{
>  	  /* Delete old macros.  */
>  	  rs6000_target_modify_macros_ptr (false,
> -					   prev_flags & diff_flags,
> -					   prev_bumask & diff_bumask);
> +					   prev_flags & diff_flags);
>  
>  	  /* Define new macros.  */
>  	  rs6000_target_modify_macros_ptr (true,
> -					   cur_flags & diff_flags,
> -					   cur_bumask & diff_bumask);
> +					   cur_flags & diff_flags);
>  	}
>      }
>  
>    return true;
>  }
> @@ -24732,19 +24677,10 @@ rs6000_print_isa_options (FILE *file, int indent, const char *string,
>    rs6000_print_options_internal (file, indent, string, flags, "-m",
>  				 &rs6000_opt_masks[0],
>  				 ARRAY_SIZE (rs6000_opt_masks));
>  }
>  
> -static void
> -rs6000_print_builtin_options (FILE *file, int indent, const char *string,
> -			      HOST_WIDE_INT flags)
> -{
> -  rs6000_print_options_internal (file, indent, string, flags, "",
> -				 &rs6000_builtin_mask_names[0],
> -				 ARRAY_SIZE (rs6000_builtin_mask_names));
> -}

rs6000_builtin_mask_names becomes useless too, can be removed too?

BR,
Kewen