"Kewen.Lin" <li...@linux.ibm.com> writes:
> Hi Segher,
>
> Thanks for your comments! Updated to v2 as below:
>
> 1) Removed unnecessary hook loop_unroll_adjust_tree.
> 2) Updated estimated_uf to estimated_unroll and some comments.
>
> gcc/ChangeLog
>
> 2020-02-10 Kewen Lin <li...@gcc.gnu.org>
>
> * cfgloop.h (struct loop): New field estimated_unroll.
> * tree-ssa-loop-manip.c (decide_uf_const_iter): New function.
> (decide_uf_runtime_iter): Likewise.
> (decide_uf_stupid): Likewise.
> (estimate_unroll_factor): Likewise.
In RTL unroller, target hooks are also involved when decide unroll
factor, so these decide_uf_xx functions may not same with final real
unroll factor in RTL. For example, at O2 by default, small loops will be
unrolled 2 times.
> * tree-ssa-loop-manip.h (estimate_unroll_factor): New declare.
> * tree-ssa-loop.c (tree_average_num_loop_insns): New function.
> * tree-ssa-loop.h (tree_average_num_loop_insns): New declare.
>
> BR,
> Kewen
>
> on 2020/1/20 下午9:02, Segher Boessenkool wrote:
>> Hi!
>>
>> On Thu, Jan 16, 2020 at 05:39:40PM +0800, Kewen.Lin wrote:
>>> --- a/gcc/cfgloop.h
>>> +++ b/gcc/cfgloop.h
>>> @@ -232,6 +232,9 @@ public:
>>> Other values means unroll with the given unrolling factor. */
>>> unsigned short unroll;
>>>
>>> + /* Like unroll field above, but it's estimated in middle-end. */
>>> + unsigned short estimated_uf;
>>
>> Please use full words? "estimated_unroll" perhaps? (Similar for other
>> new names).
>>
>
> Done.
>
>>> +/* Implement targetm.loop_unroll_adjust_tree, strictly refers to
>>> + targetm.loop_unroll_adjust. */
>>> +
>>> +static unsigned
>>> +rs6000_loop_unroll_adjust_tree (unsigned nunroll, struct loop *loop)
>>> +{
>>> + /* For now loop_unroll_adjust is simple, just invoke directly. */
>>> + return rs6000_loop_unroll_adjust (nunroll, loop);
>>> +}
>>
>> Since the two hooks have the same arguments as well, it should really
>> just be one hook, and an implementation can check whether
>> current_pass->type == RTL_PASS
>> if it needs to do something special for RTL, etc.? Or it can use some
>> more appropriate condition -- the point is you need no extra hook.
>>
>
> Good point, removed it.
>
>>> + /* Check number of iterations is constant. */
>>> + if ((niter_desc->may_be_zero && !integer_zerop (niter_desc->may_be_zero))
>>> + || !tree_fits_uhwi_p (niter_desc->niter))
>>> + return false;
>>
>> Check, and do what? It's easier to read if you say.
>>
>
> Done.
>
>>
>> "If loop->unroll is set, use that as loop->estimated_unroll"?
>>
>
> Done.
