Segher Boessenkool <seg...@kernel.crashing.org> writes:
> Hi!
>
> On Wed, Oct 30, 2019 at 11:34:42AM +0800, Jiufu Guo wrote:
>> In this patch, loop unroll adjust hook is introduced for powerpc. In this
>> hook,
>> we can do target related hueristic adjustment. For this patch, we tunned for
>> O2 to unroll small loops with small unroll factor (2 times), for other
>> optimization, default unroll factor is used.
>
>> 2019-10-30 Jiufu Guo <guoji...@linux.ibm.com>
>>
>> PR tree-optimization/88760
>
> I think this should be "target"? (Change it in bugzilla if you change it
> here, of course).
Or should I open a new bug, then PR88760 could be keep for tree-optimization?
>
>> * config/rs6000/rs6000.c (rs6000_option_override_internal): Remove
>> code which changes PARAM_MAX_UNROLL_TIMES and PARAM_MAX_UNROLLED_INSNS.
>> (TARGET_LOOP_UNROLL_ADJUST): Add loop unroll adjust hook.
>> (rs6000_loop_unroll_adjust): New hook for loop unroll adjust. Unrolling
>> small loop 2 times for -O2.
>
> (Two spaces after a full stop).
>
>> +/* This target hook implementation for TARGET_LOOP_UNROLL_ADJUST calculates
>> + a new heristic number struct loop *loop should be unrolled. */
>
> "heuristic".
>
> Argument names are written in ALL CAPS, and repeating the type is useless
> here (you can see it just a few lines down). But anyway, everything else
> here just says things like
>
> /* Implement targetm.loop_unroll_adjust. */
>
> so just do that?
>
>> +static unsigned
>> +rs6000_loop_unroll_adjust (unsigned nunroll, struct loop * loop)
>> +{
>> + /* For -O2, we only unroll small loops with small unroll factor. */
>> + if (optimize == 2)
>> + {
>> + /* If the loop contains few insns, treated it as small loops.
>> + TODO: Uing 10 hard code for now. Continue to refine, For example,
>> + if loop constians only 1-2 insns, we may unroll more times(4).
>> + And we may use PARAM to control kinds of loop size. */
>
> That first line has no new information. So maybe something like
> /* 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. */
Thanks for your so detail and careful suggestions which always help me
to make patch better!
>
> Okay for trunk like that. Thanks!
>
>
> Segher
