Re: [PATCH 2/2] rs6000: tune loop size for cunroll at O2

[Segher Boessenkool]

Hi!

Yes, you can just use estimate_num_loop_insns afaics.

On Wed, May 20, 2020 at 11:58:20AM +0800, Jiufu Guo wrote:
>if (unroll_only_small_loops && optimize == 2)
>  {
>if (maxiter >= 4
> -   && !(TREE_CODE (niter) == INTEGER_CST && single_exit (loop)))
> +   && !(TREE_CODE (niter) == INTEGER_CST && num_stmt_in_loop (loop) < 24
> +&& single_exit (loop)))

Please right this with both && in the inner condition on their own line,
like

   if (maxiter >= 4
  && !(TREE_CODE (niter) == INTEGER_CST
   && single_exit (loop)
   && num_stmt_in_loop (loop) < 24))

(also keep the single_exit test first, it is a simpler thing, in code
executed but conceptually as well).

Looks fine then.


Segher

Re: [PATCH 2/2] rs6000: tune loop size for cunroll at O2

[Jiufu Guo via Gcc-patches]

"Kewen.Lin"  writes:

> Hi Jeff,
>
> on 2020/5/20 上午11:58, Jiufu Guo via Gcc-patches wrote:
>> Hi,
>> 
>> This patch check the size of a loop to be unrolled/peeled completely,
>> and set the limits to a number (24).  This prevents large loop from
>> being unrolled, then avoid binary size increasing, and this limit keeps
>> performance.
>> 
>> Bootstrap pass on powerpc64le, ok for trunk?
>> 
>> Jiufu
>> 
>> ---
>>  gcc/config/rs6000/rs6000.c | 19 ++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
>> index a1a3f9cb583..f3abb92d046 100644
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -5142,6 +5142,22 @@ rs6000_loop_unroll_adjust (unsigned nunroll, struct 
>> loop *loop)
>>return nunroll;
>>  }
>>  
>> +/* Count the number of statements in LOOP.  */
>> +
>> +static int
>> +num_stmt_in_loop (class loop *loop)
>> +{
>> +  int res = 0;
>> +  basic_block *bbs = get_loop_body (loop);
>> +  for (unsigned i = 0; i < loop->num_nodes; i++)
>> +for (gimple_stmt_iterator bsi = gsi_start_bb (bbs[i]); !gsi_end_p (bsi);
>> + gsi_next ())
>> +  if (!is_gimple_debug (gsi_stmt (bsi)))
>> +res++;
>> +
>> +  return res;
>> +}
>> +
>
> Would it be possible to use tree_num_loop_insns here?
> If no, this can be part of tree-ssa-loop.c, and we need to free(bbs).

free (bbs) is missed here. 
tree_num_loop_insns may be used, I would have a try. 

Thanks!
Jiufu(Jeff)

>
> BR,
> Kewen
>
>>  /* Implement targetm.loop_allow_unroll_completely_peel.  */
>>  
>>  static bool
>> @@ -5151,7 +5167,8 @@ rs6000_loop_allow_unroll_completely_peel 
>> (HOST_WIDE_INT maxiter, tree niter,
>>if (unroll_only_small_loops && optimize == 2)
>>  {
>>if (maxiter >= 4
>> -  && !(TREE_CODE (niter) == INTEGER_CST && single_exit (loop)))
>> +  && !(TREE_CODE (niter) == INTEGER_CST && num_stmt_in_loop (loop) < 24
>> +   && single_exit (loop)))
>>  return false;
>>  }
>>  
>> 

Re: [PATCH 2/2] rs6000: tune loop size for cunroll at O2

[Kewen.Lin via Gcc-patches]

Hi Jeff,

on 2020/5/20 上午11:58, Jiufu Guo via Gcc-patches wrote:
> Hi,
> 
> This patch check the size of a loop to be unrolled/peeled completely,
> and set the limits to a number (24).  This prevents large loop from
> being unrolled, then avoid binary size increasing, and this limit keeps
> performance.
> 
> Bootstrap pass on powerpc64le, ok for trunk?
> 
> Jiufu
> 
> ---
>  gcc/config/rs6000/rs6000.c | 19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index a1a3f9cb583..f3abb92d046 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -5142,6 +5142,22 @@ rs6000_loop_unroll_adjust (unsigned nunroll, struct 
> loop *loop)
>return nunroll;
>  }
>  
> +/* Count the number of statements in LOOP.  */
> +
> +static int
> +num_stmt_in_loop (class loop *loop)
> +{
> +  int res = 0;
> +  basic_block *bbs = get_loop_body (loop);
> +  for (unsigned i = 0; i < loop->num_nodes; i++)
> +for (gimple_stmt_iterator bsi = gsi_start_bb (bbs[i]); !gsi_end_p (bsi);
> +  gsi_next ())
> +  if (!is_gimple_debug (gsi_stmt (bsi)))
> + res++;
> +
> +  return res;
> +}
> +

Would it be possible to use tree_num_loop_insns here?
If no, this can be part of tree-ssa-loop.c, and we need to free(bbs).

BR,
Kewen

>  /* Implement targetm.loop_allow_unroll_completely_peel.  */
>  
>  static bool
> @@ -5151,7 +5167,8 @@ rs6000_loop_allow_unroll_completely_peel (HOST_WIDE_INT 
> maxiter, tree niter,
>if (unroll_only_small_loops && optimize == 2)
>  {
>if (maxiter >= 4
> -   && !(TREE_CODE (niter) == INTEGER_CST && single_exit (loop)))
> +   && !(TREE_CODE (niter) == INTEGER_CST && num_stmt_in_loop (loop) < 24
> +&& single_exit (loop)))
>   return false;
>  }
>  
> 

Re: [PATCH 2/2] rs6000: tune loop size for cunroll at O2

[Jiufu Guo via Gcc-patches]

Jiufu Guo  writes:

> Hi,
>
> This patch check the size of a loop to be unrolled/peeled completely,
> and set the limits to a number (24).  This prevents large loop from
> being unrolled, then avoid binary size increasing, and this limit keeps
> performance.
>
> Bootstrap pass on powerpc64le, ok for trunk?
>
> Jiufu

ChangeLog
2020-05-20  Jiufu Guo   

PR target/95018
* config/rs6000/rs6000.c (num_stmt_in_loop): New function.
(rs6000_loop_allow_unroll_completely_peel): Call num_stmt_in_loop.

>
> ---
>  gcc/config/rs6000/rs6000.c | 19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index a1a3f9cb583..f3abb92d046 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -5142,6 +5142,22 @@ rs6000_loop_unroll_adjust (unsigned nunroll, struct 
> loop *loop)
>return nunroll;
>  }
>  
> +/* Count the number of statements in LOOP.  */
> +
> +static int
> +num_stmt_in_loop (class loop *loop)
> +{
> +  int res = 0;
> +  basic_block *bbs = get_loop_body (loop);
> +  for (unsigned i = 0; i < loop->num_nodes; i++)
> +for (gimple_stmt_iterator bsi = gsi_start_bb (bbs[i]); !gsi_end_p (bsi);
> +  gsi_next ())
> +  if (!is_gimple_debug (gsi_stmt (bsi)))
> + res++;
> +
> +  return res;
> +}
> +
>  /* Implement targetm.loop_allow_unroll_completely_peel.  */
>  
>  static bool
> @@ -5151,7 +5167,8 @@ rs6000_loop_allow_unroll_completely_peel (HOST_WIDE_INT 
> maxiter, tree niter,
>if (unroll_only_small_loops && optimize == 2)
>  {
>if (maxiter >= 4
> -   && !(TREE_CODE (niter) == INTEGER_CST && single_exit (loop)))
> +   && !(TREE_CODE (niter) == INTEGER_CST && num_stmt_in_loop (loop) < 24
> +&& single_exit (loop)))
>   return false;
>  }

[PATCH 2/2] rs6000: tune loop size for cunroll at O2

[Jiufu Guo via Gcc-patches]

Hi,

This patch check the size of a loop to be unrolled/peeled completely,
and set the limits to a number (24).  This prevents large loop from
being unrolled, then avoid binary size increasing, and this limit keeps
performance.

Bootstrap pass on powerpc64le, ok for trunk?

Jiufu

---
 gcc/config/rs6000/rs6000.c | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index a1a3f9cb583..f3abb92d046 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -5142,6 +5142,22 @@ rs6000_loop_unroll_adjust (unsigned nunroll, struct loop 
*loop)
   return nunroll;
 }
 
+/* Count the number of statements in LOOP.  */
+
+static int
+num_stmt_in_loop (class loop *loop)
+{
+  int res = 0;
+  basic_block *bbs = get_loop_body (loop);
+  for (unsigned i = 0; i < loop->num_nodes; i++)
+for (gimple_stmt_iterator bsi = gsi_start_bb (bbs[i]); !gsi_end_p (bsi);
+gsi_next ())
+  if (!is_gimple_debug (gsi_stmt (bsi)))
+   res++;
+
+  return res;
+}
+
 /* Implement targetm.loop_allow_unroll_completely_peel.  */
 
 static bool
@@ -5151,7 +5167,8 @@ rs6000_loop_allow_unroll_completely_peel (HOST_WIDE_INT 
maxiter, tree niter,
   if (unroll_only_small_loops && optimize == 2)
 {
   if (maxiter >= 4
- && !(TREE_CODE (niter) == INTEGER_CST && single_exit (loop)))
+ && !(TREE_CODE (niter) == INTEGER_CST && num_stmt_in_loop (loop) < 24
+  && single_exit (loop)))
return false;
 }
 
-- 
2.17.1