Segher Boessenkool <seg...@kernel.crashing.org> writes:
> Hi!
>
> On Tue, Nov 05, 2019 at 04:33:23PM +0800, Jiufu Guo wrote:
>> --- gcc/common/config/rs6000/rs6000-common.c (revision 277765)
>> +++ gcc/common/config/rs6000/rs6000-common.c (working copy)
>> @@ -35,7 +35,9 @@ static const struct default_options rs6000_option_
>> { OPT_LEVELS_ALL, OPT_fsplit_wide_types_early, NULL, 1 },
>> /* Enable -fsched-pressure for first pass instruction scheduling. */
>> { OPT_LEVELS_1_PLUS, OPT_fsched_pressure, NULL, 1 },
>> - { OPT_LEVELS_2_PLUS, OPT_funroll_loops, NULL, 1 },
>> + /* Enable -funroll-loops with -munroll-small-loops. */
>> + { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_funroll_loops, NULL, 1 },
>> + { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_munroll_small_loops, NULL, 1 },
>
> I guess the comment should say what we enable here more than the generic
> code does. Something like
>
> /* Enable -funroll-loops at -O2 already. Also enable
> -munroll-small-loops. */
updated to:
/* Enable -munroll-only-small-loops with -funroll-loops to unroll small
loops at -O2 and above by default. */
{ OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_funroll_loops, NULL, 1 },
{ OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_munroll_small_loops, NULL, 1 },
/* Disable -fweb and -frename-registers to avoid bad impacts. */
{ OPT_LEVELS_ALL, OPT_fweb, NULL, 0 },
{ OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 },
Thanks for more comments to make it better!
>
>> + /* Explicit funroll-loops turns -munroll-small-loops off.
>> + Implicit funroll-loops does not turn fweb or frename-registers on. */
>> + if ((global_options_set.x_flag_unroll_loops && flag_unroll_loops)
>> + || (global_options_set.x_flag_unroll_all_loops
>> + && flag_unroll_all_loops))
>> {
>> + if (!global_options_set.x_unroll_small_loops)
>> + unroll_small_loops = 0;
>> + }
>> + else
>> + {
>> if (!global_options_set.x_flag_web)
>> + flag_web = 0;
>> if (!global_options_set.x_flag_rename_registers)
>> + flag_rename_registers = 0;
>> }
>
> So unroll-small-loops should better be called unroll-only-small-loops?
Thanks again. Right, unroll-only-small-loops is better.
>
> Why does explicit unroll-loops turns on web and rnreg? Why only explicit?
> Isn't it good and/or bad in all the same cases, implicit and explicit?
Good question!
Turn off them by default, because they do not help too much for generic
cases, and did not see performance gain on SPEC2017. And turning them
off will help to consistent with previous -O2/-O3 which does not turn
them on. This could avoid regressions in test cases.
If do not turn them on with -funroll-loops, user may see performance
difference on some cases. For example, in SPEC peak which option
contains -funroll-loops, it may need to add -frename-registers manually
for some benchmarks.
Any sugguestions? Do you think it is a good idea to disable them by
default, and let user to add them when they are helpful? e.g. add them
for some benchmarks at `peak`.
>
>> +munroll-small-loops
>> +Target Undocumented Var(unroll_small_loops) Init(0) Save
>> +Use conservative small loop unrolling.
>
> Undocumented means undocumented, so you don't have a comment string in
> here. But you can comment it:
>
> ; Use conservative small loop unrolling.
Thanks again for you kindly review!
Jiufu,
BR.
>
>
> Segher
