public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] vect: Replace hardcoded weight factor with param
@ 2021-05-19  6:20 Kewen.Lin
  2021-05-19  8:15 ` Richard Biener
  0 siblings, 1 reply; 10+ messages in thread
From: Kewen.Lin @ 2021-05-19  6:20 UTC (permalink / raw)
  To: GCC Patches
  Cc: Richard Biener, Richard Sandiford, Segher Boessenkool, Bill Schmidt

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

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?

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

	* doc/invoke.texi (vect-inner-loop-weight-factor): Document new
	parameter.
	* params.opt (vect-inner-loop-weight-factor): New.
	* config/aarch64/aarch64.c (aarch64_add_stmt_cost): Replace hardcoded
	weight factor 50 with param_vect_inner_loop_weight_factor.
	* config/arm/arm.c (arm_add_stmt_cost): Likewise.
	* config/i386/i386.c (ix86_add_stmt_cost): Likewise.
	* config/rs6000/rs6000.c (rs6000_add_stmt_cost): Likewise.
	* targhooks.c (default_add_stmt_cost): Likewise.
	* tree-vect-loop.c (vect_compute_single_scalar_iteration_cost):
	Likewise.

[-- Attachment #2: factor.diff --]
[-- Type: text/plain, Size: 4792 bytes --]

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 12625a4bee3..a7e765df3f9 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -15437,7 +15437,7 @@ aarch64_add_stmt_cost (class vec_info *vinfo, void *data, int count,
 	 arbitrary and could potentially be improved with analysis.  */
       if (where == vect_body && stmt_info
 	  && stmt_in_inner_loop_p (vinfo, stmt_info))
-	count *= 50; /*  FIXME  */
+	count *= param_vect_inner_loop_weight_factor; /*  FIXME  */
 
       retval = (unsigned) (count * stmt_cost);
       costs->region[where] += retval;
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 340f7c95d76..4fdaac607d6 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -12201,7 +12201,7 @@ arm_add_stmt_cost (vec_info *vinfo, void *data, int count,
 	 arbitrary and could potentially be improved with analysis.  */
       if (where == vect_body && stmt_info
 	  && stmt_in_inner_loop_p (vinfo, stmt_info))
-	count *= 50;  /* FIXME.  */
+	count *= param_vect_inner_loop_weight_factor;  /* FIXME.  */
 
       retval = (unsigned) (count * stmt_cost);
       cost[where] += retval;
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 7c41302c75b..5203fda94c1 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -22396,7 +22396,7 @@ ix86_add_stmt_cost (class vec_info *vinfo, void *data, int count,
      arbitrary and could potentially be improved with analysis.  */
   if (where == vect_body && stmt_info
       && stmt_in_inner_loop_p (vinfo, stmt_info))
-    count *= 50;  /* FIXME.  */
+    count *= param_vect_inner_loop_weight_factor;  /* FIXME.  */
 
   retval = (unsigned) (count * stmt_cost);
 
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 48b8efd732b..8ffbe0a2229 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -5348,7 +5348,7 @@ rs6000_add_stmt_cost (class vec_info *vinfo, void *data, int count,
 	 arbitrary and could potentially be improved with analysis.  */
       if (where == vect_body && stmt_info
 	  && stmt_in_inner_loop_p (vinfo, stmt_info))
-	count *= 50;  /* FIXME.  */
+	count *= param_vect_inner_loop_weight_factor;  /* FIXME.  */
 
       retval = (unsigned) (count * stmt_cost);
       cost_data->cost[where] += retval;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 8b70fdf580d..6e45e08ba3e 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -14221,6 +14221,10 @@ code to iterate.  2 allows partial vector loads and stores in all loops.
 The parameter only has an effect on targets that support partial
 vector loads and stores.
 
+@item vect-inner-loop-weight-factor
+The factor which loop vectorizer uses to over weight those statements in
+an inner loop relative to the loop being vectorized.
+
 @item avoid-fma-max-bits
 Maximum number of bits for which we avoid creating FMAs.
 
diff --git a/gcc/params.opt b/gcc/params.opt
index 7c7aa78992a..fb09353ec8c 100644
--- a/gcc/params.opt
+++ b/gcc/params.opt
@@ -1089,4 +1089,8 @@ Bound on number of runtime checks inserted by the vectorizer's loop versioning f
 Common Joined UInteger Var(param_vect_partial_vector_usage) Init(2) IntegerRange(0, 2) Param Optimization
 Controls how loop vectorizer uses partial vectors.  0 means never, 1 means only for loops whose need to iterate can be removed, 2 means for all loops.  The default value is 2.
 
+-param=vect-inner-loop-weight-factor=
+Common Joined UInteger Var(param_vect_inner_loop_weight_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.
+
 ; This comment is to ensure we retain the blank line above.
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index 952fad422eb..6292cbd9bc5 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1400,7 +1400,7 @@ default_add_stmt_cost (class vec_info *vinfo, void *data, int count,
       arbitrary and could potentially be improved with analysis.  */
   if (where == vect_body && stmt_info
       && stmt_in_inner_loop_p (vinfo, stmt_info))
-    count *= 50;  /* FIXME.  */
+    count *= param_vect_inner_loop_weight_factor;
 
   retval = (unsigned) (count * stmt_cost);
   cost[where] += retval;
diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
index 2aba503fef7..3bbd8ac569e 100644
--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -1237,7 +1237,7 @@ vect_compute_single_scalar_iteration_cost (loop_vec_info loop_vinfo)
   /* FORNOW.  */
   innerloop_iters = 1;
   if (loop->inner)
-    innerloop_iters = 50; /* FIXME */
+    innerloop_iters = param_vect_inner_loop_weight_factor;
 
   for (i = 0; i < nbbs; i++)
     {

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vect: Replace hardcoded weight factor with param
  2021-05-19  6:20 [PATCH] vect: Replace hardcoded weight factor with param Kewen.Lin
@ 2021-05-19  8:15 ` Richard Biener
  2021-05-19  9:46   ` [PATCH v2] " Kewen.Lin
  2021-05-19 13:32   ` [PATCH] " Segher Boessenkool
  0 siblings, 2 replies; 10+ messages in thread
From: Richard Biener @ 2021-05-19  8:15 UTC (permalink / raw)
  To: Kewen.Lin
  Cc: GCC Patches, Richard Sandiford, Segher Boessenkool, Bill Schmidt

On Wed, May 19, 2021 at 8:20 AM Kewen.Lin <linkw@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,
Richard.

> BR,
> Kewen
> -------
> gcc/ChangeLog:
>
>         * doc/invoke.texi (vect-inner-loop-weight-factor): Document new
>         parameter.
>         * params.opt (vect-inner-loop-weight-factor): New.
>         * config/aarch64/aarch64.c (aarch64_add_stmt_cost): Replace hardcoded
>         weight factor 50 with param_vect_inner_loop_weight_factor.
>         * config/arm/arm.c (arm_add_stmt_cost): Likewise.
>         * config/i386/i386.c (ix86_add_stmt_cost): Likewise.
>         * config/rs6000/rs6000.c (rs6000_add_stmt_cost): Likewise.
>         * targhooks.c (default_add_stmt_cost): Likewise.
>         * tree-vect-loop.c (vect_compute_single_scalar_iteration_cost):
>         Likewise.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] vect: Replace hardcoded weight factor with param
  2021-05-19  8:15 ` Richard Biener
@ 2021-05-19  9:46   ` Kewen.Lin
  2021-05-19 10:01     ` Richard Biener
  2021-05-19 13:32   ` [PATCH] " Segher Boessenkool
  1 sibling, 1 reply; 10+ messages in thread
From: Kewen.Lin @ 2021-05-19  9:46 UTC (permalink / raw)
  To: Richard Biener
  Cc: GCC Patches, Richard Sandiford, Segher Boessenkool, Bill Schmidt

[-- Attachment #1: Type: text/plain, Size: 2458 bytes --]

Hi Richi,

on 2021/5/19 下午4:15, Richard Biener wrote:
> On Wed, May 19, 2021 at 8:20 AM Kewen.Lin <linkw@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?

btw, the testing on the previous patch passed, new round testing was
just kicked off.

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

	* doc/invoke.texi (vect-inner-loop-cost-factor): Document new
	parameter.
	* params.opt (vect-inner-loop-cost-factor): New.
	* targhooks.c (default_add_stmt_cost): Replace hardcoded factor
	50 with LOOP_VINFO_INNER_LOOP_COST_FACTOR, include head file
	tree-vectorizer.h and its required ones.
	* config/aarch64/aarch64.c (aarch64_add_stmt_cost): Replace
	hardcoded factor 50 with LOOP_VINFO_INNER_LOOP_COST_FACTOR.
	* config/arm/arm.c (arm_add_stmt_cost): Likewise.
	* config/i386/i386.c (ix86_add_stmt_cost): Likewise.
	* config/rs6000/rs6000.c (rs6000_add_stmt_cost): Likewise.
	* tree-vect-loop.c (vect_compute_single_scalar_iteration_cost):
	Likewise.
	(_loop_vec_info::_loop_vec_info): Init inner_loop_cost_factor.
	* tree-vectorizer.h (_loop_vec_info): Add inner_loop_cost_factor.
	(LOOP_VINFO_INNER_LOOP_COST_FACTOR): New macro.


[-- Attachment #2: factorv2.diff --]
[-- Type: text/plain, Size: 6876 bytes --]

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 12625a4bee3..be883b61059 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -15437,7 +15437,10 @@ aarch64_add_stmt_cost (class vec_info *vinfo, void *data, int count,
 	 arbitrary and could potentially be improved with analysis.  */
       if (where == vect_body && stmt_info
 	  && stmt_in_inner_loop_p (vinfo, stmt_info))
-	count *= 50; /*  FIXME  */
+	{
+	  gcc_assert (loop_vinfo);
+	  count *= LOOP_VINFO_INNER_LOOP_COST_FACTOR (loop_vinfo); /*  FIXME  */
+	}
 
       retval = (unsigned) (count * stmt_cost);
       costs->region[where] += retval;
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 340f7c95d76..223faa49b11 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -12201,7 +12201,11 @@ arm_add_stmt_cost (vec_info *vinfo, void *data, int count,
 	 arbitrary and could potentially be improved with analysis.  */
       if (where == vect_body && stmt_info
 	  && stmt_in_inner_loop_p (vinfo, stmt_info))
-	count *= 50;  /* FIXME.  */
+	{
+	  loop_vec_info loop_vinfo = dyn_cast<loop_vec_info> (vinfo);
+	  gcc_assert (loop_vinfo);
+	  count *= LOOP_VINFO_INNER_LOOP_COST_FACTOR (loop_vinfo); /* FIXME.  */
+	}
 
       retval = (unsigned) (count * stmt_cost);
       cost[where] += retval;
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 7c41302c75b..43b1fb0de0b 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -22396,7 +22396,11 @@ ix86_add_stmt_cost (class vec_info *vinfo, void *data, int count,
      arbitrary and could potentially be improved with analysis.  */
   if (where == vect_body && stmt_info
       && stmt_in_inner_loop_p (vinfo, stmt_info))
-    count *= 50;  /* FIXME.  */
+    {
+      loop_vec_info loop_vinfo = dyn_cast<loop_vec_info> (vinfo);
+      gcc_assert (loop_vinfo);
+      count *= LOOP_VINFO_INNER_LOOP_COST_FACTOR (loop_vinfo); /* FIXME.  */
+    }
 
   retval = (unsigned) (count * stmt_cost);
 
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 48b8efd732b..859da8bd0ed 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -5348,7 +5348,11 @@ rs6000_add_stmt_cost (class vec_info *vinfo, void *data, int count,
 	 arbitrary and could potentially be improved with analysis.  */
       if (where == vect_body && stmt_info
 	  && stmt_in_inner_loop_p (vinfo, stmt_info))
-	count *= 50;  /* FIXME.  */
+	{
+	  loop_vec_info loop_vinfo = dyn_cast<loop_vec_info> (vinfo);
+	  gcc_assert (loop_vinfo);
+	  count *= LOOP_VINFO_INNER_LOOP_COST_FACTOR (loop_vinfo); /* FIXME.  */
+	}
 
       retval = (unsigned) (count * stmt_cost);
       cost_data->cost[where] += retval;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 8b70fdf580d..2234801cab4 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -14221,6 +14221,10 @@ code to iterate.  2 allows partial vector loads and stores in all loops.
 The parameter only has an effect on targets that support partial
 vector loads and stores.
 
+@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.
+
 @item avoid-fma-max-bits
 Maximum number of bits for which we avoid creating FMAs.
 
diff --git a/gcc/params.opt b/gcc/params.opt
index 7c7aa78992a..a35c2abe359 100644
--- a/gcc/params.opt
+++ b/gcc/params.opt
@@ -1089,4 +1089,8 @@ Bound on number of runtime checks inserted by the vectorizer's loop versioning f
 Common Joined UInteger Var(param_vect_partial_vector_usage) Init(2) IntegerRange(0, 2) Param Optimization
 Controls how loop vectorizer uses partial vectors.  0 means never, 1 means only for loops whose need to iterate can be removed, 2 means for all loops.  The default value is 2.
 
+-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.
+
 ; This comment is to ensure we retain the blank line above.
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index 952fad422eb..b595b7838af 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -90,6 +90,9 @@ along with GCC; see the file COPYING3.  If not see
 #include "attribs.h"
 #include "asan.h"
 #include "emit-rtl.h"
+#include "gimple.h"
+#include "cfgloop.h"
+#include "tree-vectorizer.h"
 
 bool
 default_legitimate_address_p (machine_mode mode ATTRIBUTE_UNUSED,
@@ -1400,7 +1403,11 @@ default_add_stmt_cost (class vec_info *vinfo, void *data, int count,
       arbitrary and could potentially be improved with analysis.  */
   if (where == vect_body && stmt_info
       && stmt_in_inner_loop_p (vinfo, stmt_info))
-    count *= 50;  /* FIXME.  */
+    {
+      loop_vec_info loop_vinfo = dyn_cast<loop_vec_info> (vinfo);
+      gcc_assert (loop_vinfo);
+      count *= LOOP_VINFO_INNER_LOOP_COST_FACTOR (loop_vinfo);
+    }
 
   retval = (unsigned) (count * stmt_cost);
   cost[where] += retval;
diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
index 2aba503fef7..106c91964b5 100644
--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -836,6 +836,7 @@ _loop_vec_info::_loop_vec_info (class loop *loop_in, vec_info_shared *shared)
     single_scalar_iteration_cost (0),
     vec_outside_cost (0),
     vec_inside_cost (0),
+    inner_loop_cost_factor (param_vect_inner_loop_cost_factor),
     vectorizable (false),
     can_use_partial_vectors_p (param_vect_partial_vector_usage != 0),
     using_partial_vectors_p (false),
@@ -1237,7 +1238,7 @@ vect_compute_single_scalar_iteration_cost (loop_vec_info loop_vinfo)
   /* FORNOW.  */
   innerloop_iters = 1;
   if (loop->inner)
-    innerloop_iters = 50; /* FIXME */
+    innerloop_iters = LOOP_VINFO_INNER_LOOP_COST_FACTOR (loop_vinfo);
 
   for (i = 0; i < nbbs; i++)
     {
diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
index 9861d9e8810..b8ba63cc8e2 100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -689,6 +689,10 @@ public:
   /* The cost of the vector loop body.  */
   int vec_inside_cost;
 
+  /* The factor used to over weight those statements in an inner loop
+     relative to the loop being vectorized.  */
+  unsigned int inner_loop_cost_factor;
+
   /* Is the loop vectorizable? */
   bool vectorizable;
 
@@ -807,6 +811,7 @@ public:
 #define LOOP_VINFO_SINGLE_SCALAR_ITERATION_COST(L) (L)->single_scalar_iteration_cost
 #define LOOP_VINFO_ORIG_LOOP_INFO(L)       (L)->orig_loop_info
 #define LOOP_VINFO_SIMD_IF_COND(L)         (L)->simd_if_cond
+#define LOOP_VINFO_INNER_LOOP_COST_FACTOR(L) (L)->inner_loop_cost_factor
 
 #define LOOP_VINFO_FULLY_MASKED_P(L)		\
   (LOOP_VINFO_USING_PARTIAL_VECTORS_P (L)	\

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] vect: Replace hardcoded weight factor with param
  2021-05-19  9:46   ` [PATCH v2] " Kewen.Lin
@ 2021-05-19 10:01     ` Richard Biener
  2021-05-20  8:52       ` Kewen.Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2021-05-19 10:01 UTC (permalink / raw)
  To: Kewen.Lin
  Cc: GCC Patches, Richard Sandiford, Segher Boessenkool, Bill Schmidt

On Wed, May 19, 2021 at 11:47 AM Kewen.Lin <linkw@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 <linkw@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."

OK with that change.

Richard.

> btw, the testing on the previous patch passed, new round testing was
> just kicked off.
>
> BR,
> Kewen
> ------
> gcc/ChangeLog:
>
>         * doc/invoke.texi (vect-inner-loop-cost-factor): Document new
>         parameter.
>         * params.opt (vect-inner-loop-cost-factor): New.
>         * targhooks.c (default_add_stmt_cost): Replace hardcoded factor
>         50 with LOOP_VINFO_INNER_LOOP_COST_FACTOR, include head file
>         tree-vectorizer.h and its required ones.
>         * config/aarch64/aarch64.c (aarch64_add_stmt_cost): Replace
>         hardcoded factor 50 with LOOP_VINFO_INNER_LOOP_COST_FACTOR.
>         * config/arm/arm.c (arm_add_stmt_cost): Likewise.
>         * config/i386/i386.c (ix86_add_stmt_cost): Likewise.
>         * config/rs6000/rs6000.c (rs6000_add_stmt_cost): Likewise.
>         * tree-vect-loop.c (vect_compute_single_scalar_iteration_cost):
>         Likewise.
>         (_loop_vec_info::_loop_vec_info): Init inner_loop_cost_factor.
>         * tree-vectorizer.h (_loop_vec_info): Add inner_loop_cost_factor.
>         (LOOP_VINFO_INNER_LOOP_COST_FACTOR): New macro.
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vect: Replace hardcoded weight factor with param
  2021-05-19  8:15 ` Richard Biener
  2021-05-19  9:46   ` [PATCH v2] " Kewen.Lin
@ 2021-05-19 13:32   ` Segher Boessenkool
  1 sibling, 0 replies; 10+ messages in thread
From: Segher Boessenkool @ 2021-05-19 13:32 UTC (permalink / raw)
  To: Richard Biener; +Cc: Kewen.Lin, GCC Patches, Richard Sandiford, Bill Schmidt

On Wed, May 19, 2021 at 10:15:49AM +0200, Richard Biener wrote:
> On Wed, May 19, 2021 at 8:20 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
> "weight_factor" is kind-of double-speak

"Weighting factor" (with -ing) is a standard term actually.  (But
cost_factor of course is better and avoids all that :-) )


Segher

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] vect: Replace hardcoded weight factor with param
  2021-05-19 10:01     ` Richard Biener
@ 2021-05-20  8:52       ` Kewen.Lin
  2021-05-20  9:30         ` Christophe Lyon
  0 siblings, 1 reply; 10+ messages in thread
From: Kewen.Lin @ 2021-05-20  8:52 UTC (permalink / raw)
  To: Richard Biener
  Cc: GCC Patches, Richard Sandiford, Segher Boessenkool, Bill Schmidt

on 2021/5/19 下午6:01, Richard Biener wrote:
> On Wed, May 19, 2021 at 11:47 AM Kewen.Lin <linkw@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 <linkw@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.

Committed in r12-939 as the suggested wordings.   

BR,
Kewen

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] vect: Replace hardcoded weight factor with param
  2021-05-20  8:52       ` Kewen.Lin
@ 2021-05-20  9:30         ` Christophe Lyon
  2021-05-20 10:09           ` Kewen.Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Christophe Lyon @ 2021-05-20  9:30 UTC (permalink / raw)
  To: Kewen.Lin
  Cc: Richard Biener, Richard Sandiford, Bill Schmidt, GCC Patches,
	Segher Boessenkool

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 <linkw@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 <linkw@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?

Thanks,

Christophe


> Committed in r12-939 as the suggested wordings.


>
> BR,
> Kewen

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] vect: Replace hardcoded weight factor with param
  2021-05-20  9:30         ` Christophe Lyon
@ 2021-05-20 10:09           ` Kewen.Lin
  2021-05-20 10:25             ` Richard Biener
  0 siblings, 1 reply; 10+ messages in thread
From: Kewen.Lin @ 2021-05-20 10:09 UTC (permalink / raw)
  To: Christophe Lyon
  Cc: Richard Biener, Richard Sandiford, Bill Schmidt, GCC Patches,
	Segher Boessenkool

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 <linkw@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 <linkw@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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] vect: Replace hardcoded weight factor with param
  2021-05-20 10:09           ` Kewen.Lin
@ 2021-05-20 10:25             ` Richard Biener
  2021-05-20 10:44               ` Kewen.Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2021-05-20 10:25 UTC (permalink / raw)
  To: Kewen.Lin
  Cc: Christophe Lyon, Richard Sandiford, Bill Schmidt, GCC Patches,
	Segher Boessenkool

On Thu, May 20, 2021 at 12:09 PM Kewen.Lin <linkw@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 <linkw@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 <linkw@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.

> BR,
> Kewen

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] vect: Replace hardcoded weight factor with param
  2021-05-20 10:25             ` Richard Biener
@ 2021-05-20 10:44               ` Kewen.Lin
  0 siblings, 0 replies; 10+ messages in thread
From: Kewen.Lin @ 2021-05-20 10:44 UTC (permalink / raw)
  To: Richard Biener
  Cc: Christophe Lyon, Richard Sandiford, Bill Schmidt, GCC Patches,
	Segher Boessenkool

on 2021/5/20 下午6:25, Richard Biener wrote:
> On Thu, May 20, 2021 at 12:09 PM Kewen.Lin <linkw@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 <linkw@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 <linkw@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"




^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-05-20 10:45 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-19  6:20 [PATCH] vect: Replace hardcoded weight factor with param Kewen.Lin
2021-05-19  8:15 ` Richard Biener
2021-05-19  9:46   ` [PATCH v2] " Kewen.Lin
2021-05-19 10:01     ` Richard Biener
2021-05-20  8:52       ` Kewen.Lin
2021-05-20  9:30         ` Christophe Lyon
2021-05-20 10:09           ` Kewen.Lin
2021-05-20 10:25             ` Richard Biener
2021-05-20 10:44               ` Kewen.Lin
2021-05-19 13:32   ` [PATCH] " Segher Boessenkool

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).