on 2021/5/20 下午6:25, Richard Biener wrote: > On Thu, May 20, 2021 at 12:09 PM Kewen.Lin <li...@linux.ibm.com> wrote: >> >> on 2021/5/20 下午5:30, Christophe Lyon wrote: >>> On Thu, 20 May 2021 at 10:52, Kewen.Lin via Gcc-patches >>> <gcc-patches@gcc.gnu.org> wrote: >>>> >>>> on 2021/5/19 下午6:01, Richard Biener wrote: >>>>> On Wed, May 19, 2021 at 11:47 AM Kewen.Lin <li...@linux.ibm.com> wrote: >>>>>> >>>>>> Hi Richi, >>>>>> >>>>>> on 2021/5/19 下午4:15, Richard Biener wrote: >>>>>>> On Wed, May 19, 2021 at 8:20 AM Kewen.Lin <li...@linux.ibm.com> wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> This patch is to replace the current hardcoded weight factor 50 >>>>>>>> for those statements in an inner loop relative to the loop being >>>>>>>> vectorized with a specific parameter vect-inner-loop-weight-factor. >>>>>>>> >>>>>>>> The motivation behind this change is: if targets want to have one >>>>>>>> unique function to gather some information in each add_stmt_cost >>>>>>>> call, no matter that it's put before or after the cost tweaking >>>>>>>> part for inner loop, it may have the need to adjust (expand or >>>>>>>> shrink) the gathered data as the factor. Now the factor is >>>>>>>> hardcoded, it's not easily maintained. Since it's possible that >>>>>>>> targets have their own decisions on this costing like the others, >>>>>>>> I used parameter instead of one unique macro here. >>>>>>>> >>>>>>>> Testing is ongoing, is it ok for trunk if everything goes well? >>>>>>> >>>>>>> Certainly an improvement. I suppose we might want to put >>>>>>> the factor into vinfo->inner_loop_cost_factor. That way >>>>>>> we could adjust it easily in common code in the vectorizer >>>>>>> when we for example have (non-guessed) profile data. >>>>>>> >>>>>>> "weight_factor" is kind-of double-speak and I'm missing 'cost' ... >>>>>>> so, bike-shedding to vect_inner_loop_cost_factor? >>>>>>> >>>>>>> Just suggestions - as said, the patch is an improvement already. >>>>>>> >>>>>> >>>>>> Thanks for your nice suggestions! I've updated the patch accordingly >>>>>> and attached it. Does it look better to you? >>>>> >>>>> Minor nit: >>>>> >>>>> +@item vect-inner-loop-cost-factor >>>>> +The factor which loop vectorizer uses to over weight those statements in >>>>> +an inner loop relative to the loop being vectorized. >>>>> + >>>>> >>>>> the default value should be documented here, not.. >>>>> >>>>> +-param=vect-inner-loop-cost-factor= >>>>> +Common Joined UInteger Var(param_vect_inner_loop_cost_factor) >>>>> Init(50) IntegerRange(1, 999999) Param Optimization >>>>> +Indicates the factor which loop vectorizer uses to over weight those >>>>> statements in an inner loop relative to the loop being vectorized. >>>>> The default value is 50. >>>>> + >>>>> >>>>> here (based on statistical analysis of existing cases). Also the >>>>> params.opt docs >>>>> should be the "brief" one - but for simplicity simply make both docs >>>>> identical >>>>> (apart from the default value doc). I suggest >>>>> >>>>> "The factor which the loop vectorizer applies to the cost of statements >>>>> in an inner loop relative to the loop being vectorized." >>>>> >>>> >>>> Thanks for catching this and the suggestion! >>>> >>>> Bootstrapped/regtested on powerpc64le-linux-gnu P9, x86_64-redhat-linux >>>> and aarch64-linux-gnu. >>>> >>> >>> This breaks the build for arm targets: >>> /tmp/158661_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c: >>> In function 'unsigned int arm_add_stmt_cost(vec_info*, void*, int, >>> vect_cost_for_stmt, _stmt_v >>> ec_info*, tree, int, vect_cost_model_location)': >>> /tmp/158661_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:12230:4: >>> error: 'loop_vec_info' was not declared in this scope >>> loop_vec_info loop_vinfo = dyn_cast<loop_vec_info> (vinfo); >>> ^ >>> /tmp/158661_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/arm/arm.c:12230:18: >>> error: expected ';' before 'loop_vinfo' >>> loop_vec_info loop_vinfo = dyn_cast<loop_vec_info> (vinfo); >>> >>> Can you fix it? >>> >> >> Oops! Deeply sorry for that and thanks for the testing! >> >> I just found that unlike the other targets arm.c doesn't include >> "tree-vectorizer.h". The issue should be fixed with the below patch: >> >> gcc/ChangeLog: >> >> * config/arm/arm.c: Include head files tree-vectorizer.h and >> cfgloop.h. >> >> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c >> index caf4e56b9fe..6ed34fbf627 100644 >> --- a/gcc/config/arm/arm.c >> +++ b/gcc/config/arm/arm.c >> @@ -69,6 +69,8 @@ >> #include "gimplify.h" >> #include "gimple.h" >> #include "selftest.h" >> +#include "cfgloop.h" >> +#include "tree-vectorizer.h" >> >> /* This file should be included last. */ >> #include "target-def.h" >> >> >> Is it counted as a obvious patch? > > Please check if you can build a cc1 cross to arm, then yes. >
Thanks for the prompt review! Yes, it worked to build a cross cc1. I did a trivial adjustment to align with the exisiting include order like other ports by putting cfgloop.h just after cfghooks.h as below. Will commit this new. BR, Kewen --- diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index caf4e56b9fe..9377aaef342 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -32,6 +32,7 @@ #include "tree.h" #include "memmodel.h" #include "cfghooks.h" +#include "cfgloop.h" #include "df.h" #include "tm_p.h" #include "stringpool.h" @@ -69,6 +70,7 @@ #include "gimplify.h" #include "gimple.h" #include "selftest.h" +#include "tree-vectorizer.h" /* This file should be included last. */ #include "target-def.h"
