| Message ID | 990b2492-9198-b713-c79f-68563d488ba0@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Make density_test only for vector version | expand |
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.
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
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
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];
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(+)