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?
BR,
Kewen
