[PATCH V3] rs6000: Refine small loop unroll in loop_unroll_adjust hook

[Jiufu Guo]

Richard Biener <rguenther@suse.de> writes:

> 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.
Thanks Richard,
This flag (unroll_small_loops) is saved/restored cl_target_option it can
distigush different CU. I had a test, it works when -flto for
multi-sources. 

Jiufu
BR.

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