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
