rs6000: Make density_test only for vector version

Message ID 990b2492-9198-b713-c79f-68563d488ba0@linux.ibm.com
State New
Headers show
Series
  • rs6000: Make density_test only for vector version
Related show

Commit Message

Jason Merrill via Gcc-patches May 7, 2021, 2:28 a.m.
Hi,

When I was investigating density_test heuristics, I noticed that
the current rs6000_density_test could be used for single scalar
iteration cost calculation, through the call trace:
  vect_compute_single_scalar_iteration_cost
    -> rs6000_finish_cost
         -> rs6000_density_test

It looks unexpected as its desriptive function comments and Bill
helped to confirm this needs to be fixed (thanks!).

So this patch is to check the passed data, if it's the same as
the one in loop_vinfo, it indicates it's working on vector version
cost calculation, otherwise just early return.

Bootstrapped/regtested on powerpc64le-linux-gnu P9.

Nothing remarkable was observed with SPEC2017 Power9 full run.

Is it ok for trunk?

BR,
Kewen
------
gcc/ChangeLog:

	* config/rs6000/rs6000.c (rs6000_density_test): Early return if
	calculating single scalar iteration cost.
gcc/config/rs6000/rs6000.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.17.1

Comments

Jason Merrill via Gcc-patches May 7, 2021, 9:43 a.m. | #1
On Fri, May 7, 2021 at 5:30 AM Kewen.Lin via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>

> Hi,

>

> When I was investigating density_test heuristics, I noticed that

> the current rs6000_density_test could be used for single scalar

> iteration cost calculation, through the call trace:

>   vect_compute_single_scalar_iteration_cost

>     -> rs6000_finish_cost

>          -> rs6000_density_test

>

> It looks unexpected as its desriptive function comments and Bill

> helped to confirm this needs to be fixed (thanks!).

>

> So this patch is to check the passed data, if it's the same as

> the one in loop_vinfo, it indicates it's working on vector version

> cost calculation, otherwise just early return.

>

> Bootstrapped/regtested on powerpc64le-linux-gnu P9.

>

> Nothing remarkable was observed with SPEC2017 Power9 full run.

>

> Is it ok for trunk?


+  /* Only care about cost of vector version, so exclude scalar
version here.  */
+  if (LOOP_VINFO_TARGET_COST_DATA (loop_vinfo) != (void *) data)
+    return;

Hmm, looks like a quite "random" test to me.  What about adding a
parameter to finish_cost () (or init_cost?) indicating the cost kind?

OTOH we already pass scalar_stmt to individual add_stmt_cost,
so not sure whether the context really matters.  That said,
the density test looks "interesting" ... the intent was that finish_cost
might look at gathered data from add_stmt, not that it looks at
the GIMPLE IL ... so why are you not counting vector_stmt vs.
scalar_stmt entries in vect_body and using that for this metric?

Richard.

> BR,

> Kewen

> ------

> gcc/ChangeLog:

>

>         * config/rs6000/rs6000.c (rs6000_density_test): Early return if

>         calculating single scalar iteration cost.
Jason Merrill via Gcc-patches May 7, 2021, 11:43 a.m. | #2
On Fri, 2021-05-07 at 10:28 +0800, Kewen.Lin via Gcc-patches wrote:
> Hi,

> 

> When I was investigating density_test heuristics, I noticed that

> the current rs6000_density_test could be used for single scalar

> iteration cost calculation, through the call trace:

>   vect_compute_single_scalar_iteration_cost

>     -> rs6000_finish_cost

>          -> rs6000_density_test

> 

> It looks unexpected as its desriptive function comments and Bill

> helped to confirm this needs to be fixed (thanks!).

> 

> So this patch is to check the passed data, if it's the same as

> the one in loop_vinfo, it indicates it's working on vector version

> cost calculation, otherwise just early return.

> 

> Bootstrapped/regtested on powerpc64le-linux-gnu P9.

> 

> Nothing remarkable was observed with SPEC2017 Power9 full run.

> 

> Is it ok for trunk?

> 

> BR,

> Kewen

> ------

> gcc/ChangeLog:

> 

> 	* config/rs6000/rs6000.c (rs6000_density_test): Early return if

> 	calculating single scalar iteration cost.




Ok, so data is passed in.. 
  static void
  rs6000_density_test (rs6000_cost_data *data)
  {
  ...
and loop_vinfo is calculated via...
  loop_vec_info loop_vinfo = loop_vec_info_for_loop (data->loop_info);
which is
  static inline loop_vec_info
  loop_vec_info_for_loop (class loop *loop)
  {
    return (loop_vec_info) loop->aux;
  }


> +  /* Only care about cost of vector version, so exclude scalar

> version here.  */

> 

> +  if (LOOP_VINFO_TARGET_COST_DATA (loop_vinfo) != (void *) data)

> 

> +    return;


Can the loop contain both vector and scalar parts?  Comments in the
function now mention a percentage of vector instructions within the
loop.  So..  this is meant to return early if there are no(?) vector
instructions?

I'm admittedly not clear on what 'scalar version' means here.
Would it
be accurate or clearer to update the comment to something like 
/* Return early if the loop_vinfo value indicates there are no vector
instructions within this loop. */ ?

thanks
-Will
Jason Merrill via Gcc-patches May 8, 2021, 8:47 a.m. | #3
Hi Will,

Thanks for the comments!

on 2021/5/7 下午7:43, will schmidt wrote:
> On Fri, 2021-05-07 at 10:28 +0800, Kewen.Lin via Gcc-patches wrote:

>> Hi,

>>

>> When I was investigating density_test heuristics, I noticed that

>> the current rs6000_density_test could be used for single scalar

>> iteration cost calculation, through the call trace:

>>   vect_compute_single_scalar_iteration_cost

>>     -> rs6000_finish_cost

>>          -> rs6000_density_test

>>

>> It looks unexpected as its desriptive function comments and Bill

>> helped to confirm this needs to be fixed (thanks!).

>>

>> So this patch is to check the passed data, if it's the same as

>> the one in loop_vinfo, it indicates it's working on vector version

>> cost calculation, otherwise just early return.

>>

>> Bootstrapped/regtested on powerpc64le-linux-gnu P9.

>>

>> Nothing remarkable was observed with SPEC2017 Power9 full run.

>>

>> Is it ok for trunk?

>>

>> BR,

>> Kewen

>> ------

>> gcc/ChangeLog:

>>

>> 	* config/rs6000/rs6000.c (rs6000_density_test): Early return if

>> 	calculating single scalar iteration cost.

> 

> 

> 

> Ok, so data is passed in.. 

>   static void

>   rs6000_density_test (rs6000_cost_data *data)

>   {

>   ...

> and loop_vinfo is calculated via...

>   loop_vec_info loop_vinfo = loop_vec_info_for_loop (data->loop_info);

> which is

>   static inline loop_vec_info

>   loop_vec_info_for_loop (class loop *loop)

>   {

>     return (loop_vec_info) loop->aux;

>   }

> 

> 

>> +  /* Only care about cost of vector version, so exclude scalar

>> version here.  */

>>

>> +  if (LOOP_VINFO_TARGET_COST_DATA (loop_vinfo) != (void *) data)

>>

>> +    return;

> 

> Can the loop contain both vector and scalar parts?  Comments in the

> function now mention a percentage of vector instructions within the

> loop.  So..  this is meant to return early if there are no(?) vector

> instructions?


Sorry for the confusion.  There are two call sites for finish_cost
in loop vectorization code, one is in function
vect_compute_single_scalar_iteration_cost, which is calculating the
cost of one scalar iteration of the loop, since it's costing the scalar
version of the loop, I called it as scalar version.  The other is in
function vect_estimate_min_profitable_iters, which is to finalize the
cost of the vector version of the loop, I called it as vector version.

Since the density test aims at the cost calculation of vector version
of the loop, we have to avoid the possible penalization when we are
actually calculating the cost for the scalar version of the loop.

As you pointed out, the scalar version looks easily confusing with the
scalar part of the vectorized loop.  I updated the comments in v2 with
the explicit words saying it's computing single scalar iteration cost.

Does it look good to you?

v2: https://gcc.gnu.org/pipermail/gcc-patches/2021-May/569896.html

BR,
Kewen

Patch

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 48b8efd732b..ffdf10098a9 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -5252,6 +5252,10 @@  rs6000_density_test (rs6000_cost_data *data)
   int vec_cost = data->cost[vect_body], not_vec_cost = 0;
   int i, density_pct;
 
+  /* Only care about cost of vector version, so exclude scalar version here.  */
+  if (LOOP_VINFO_TARGET_COST_DATA (loop_vinfo) != (void *) data)
+    return;
+
   for (i = 0; i < nbbs; i++)
     {
       basic_block bb = bbs[i];