Re: [PATCH, rs6000] Additional cleanup of rs6000_builtin_mask

[will schmidt <will_schmidt@vnet.ibm.com>]

On Thu, 2022-07-14 at 11:28 +0800, Kewen.Lin wrote:
> Hi Will,
> 
> Thanks for the cleanup!  Some comments are inlined.

Hi, 
Thanks for the review.  A few comments and responses below.  TLDR I'll
incorporate the suggestions in V2 that will show up ... after.  :-)

> 
> 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?

Yes, if I also remove usage of x_rs6000_builtin_mask.   There are a few
remaining reference to x_r_b_m, but those appear safe to remove after
this cleanup as well.  I'll confirm and likely include the removal in
V2.   


> 
> > I have tested this on current systems (P8,P9,P10) without
> > regressions.
> > 
> > OK for trunk?
> > 
> > 
> > Thanks,
> > -Will
> >     
> > 
<snip>

> >  
> > -  /* 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:


<snip...>


> 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).

Possibly.  There does exist some debug already, and I still have some
work in progress related to some of the OPTION and TARGET handling. 
I'll keep this in mind as I continue poking in this space. :-)


> >    /* 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?

It can.  I'll include removal in V2.
Thanks
-Will

> 
> BR,
> Kewen