Re: [PATCH 1/4 v2 GCC11] Add middle-end unroll factor estimation

[Jiufu Guo <guojiufu@linux.ibm.com>]

"Kewen.Lin" <linkw@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  <linkw@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.