On Mon, 4 Nov 2019, Segher Boessenkool wrote:
> Hi!
>
> On Mon, Nov 04, 2019 at 02:31:43PM +0800, Jiufu Guo wrote:
> > In this patch, loop unroll adjust hook is introduced for powerpc. We can do
> > target related hueristic adjustment in this hook. In this patch, small loops
> > is unrolled 2 times for O2 and O3 by default. With this patch, we can see
> > some improvement for spec2017. This patch enhanced a little for [Patch V2]
> > to
> > enable small loops unroll for O3 by default like O2.
>
> > * gcc/config/rs6000/rs6000.opt (unroll_small_loops): New internal flag.
>
> That's the declaration of a variable. A command line flag is something
> like -munroll-small-loops. Do we want a command line option like that?
> It makes testing simpler.
>
> > - /* unroll very small loops 2 time if no -funroll-loops. */
> > + /* If funroll-loops is not enabled explicitly, then enable small
> > loops
> > + unrolling for -O2, and do not turn fweb or frename-registers on. */
> > if (!global_options_set.x_flag_unroll_loops
> > && !global_options_set.x_flag_unroll_all_loops)
> > {
> > - maybe_set_param_value (PARAM_MAX_UNROLL_TIMES, 2,
> > - global_options.x_param_values,
> > - global_options_set.x_param_values);
> > -
> > - maybe_set_param_value (PARAM_MAX_UNROLLED_INSNS, 20,
> > - global_options.x_param_values,
> > - global_options_set.x_param_values);
> > + unroll_small_loops = optimize >= 2 ? 1 : 0;
>
> That includes -Os?
It also re-introduces the exactly same issue as the --param with LTO.
> I think you shouldn't always set it to some value, only enable it where
> you want to enable it. If you make a command line option for it this is
> especially simple (the table in common/config/rs6000/rs6000-common.c).
>
> > +static unsigned
> > +rs6000_loop_unroll_adjust (unsigned nunroll, struct loop * loop)
> > +{
> > + if (unroll_small_loops)
> > + {
> > + /* TODO: This is hardcoded to 10 right now. It can be refined, for
> > + example we may want to unroll very small loops more times (4 perhaps).
> > + We also should use a PARAM for this. */
> > + if (loop->ninsns <= 10)
> > + return MIN (2, nunroll);
> > + else
> > + return 0;
> > + }
>
> (Add an empty line here?)
>
> > + return nunroll;
> > +}
>
> Great :-)
>
> > @@ -23472,6 +23488,7 @@ rs6000_function_specific_save (struct
> > cl_target_option *ptr,
> > {
> > ptr->x_rs6000_isa_flags = opts->x_rs6000_isa_flags;
> > ptr->x_rs6000_isa_flags_explicit = opts->x_rs6000_isa_flags_explicit;
> > + ptr->x_unroll_small_loops = opts->x_unroll_small_loops;
> > }
>
> Yeah we shouldn't need to add that, this should all be automatic.
>
>
> Segher
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
