[PATCH][AArch64] Change aarch64 vector cost to match vectorizer

[PATCH][AArch64] Change aarch64 vector cost to match vectorizer

[Pawel Kupidura]

  Hello,

I've applied changes for AArch64 vectorizer config suggested by Richard 
Biener (here https://gcc.gnu.org/ml/gcc/2015-04/msg00086.html) and 
checked the effect on spec2006 and geekbench benchmarks.

Benchmarks were compiled with and without patch, in both cases with and 
without '-mcpu=cortex-a57'.

As it turns out, there is no effect of this change on generated code. 
With cortex-a57 flag, mentioned loop is not vectorized before and after 
the patch. Binaries for benchmarks test cases are identical. Without the 
flag loop is vectorized in both cases.

However, this change makes aarch64 vector costs consistent vectorizer 
code, while approach based on loop nesting level clearly underestimates 
cost of those statements.

Thanks,
Pawel

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 10df325..ffafc3f 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,7 @@
+2015-07-27  Pawel Kupidura <pawel.kupidura@arm.com>
+
+    * config/aarch64/aarch64.c: Changed inner loop statement cost
+    to be consistent with vectorizer code.
+
  2015-07-26  Uros Bizjak <ubizjak@gmail.com>

      * config/alpha/alpha.c: Use SUBREG_P predicate.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 020f63c..3b6f8c5 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -7079,15 +7079,9 @@ aarch64_add_stmt_cost (void *data, int count, 
enum vect_cost_for_stmt kind,

        /* Statements in an inner loop relative to the loop being
       vectorized are weighted more heavily.  The value here is
-     a function (linear for now) of the loop nest level.  */
+     arbitrary and could potentially be improved with analysis.  */
        if (where == vect_body && stmt_info && stmt_in_inner_loop_p 
(stmt_info))
-    {
-      loop_vec_info loop_info = STMT_VINFO_LOOP_VINFO (stmt_info);
-      struct loop *loop =  LOOP_VINFO_LOOP (loop_info);
-      unsigned nest_level = loop_depth (loop);
-
-      count *= nest_level;
-    }
+    count *= 50; /* FIXME */

        retval = (unsigned) (count * stmt_cost);
        cost[where] += retval;

Re: [PATCH][AArch64] Change aarch64 vector cost to match vectorizer

[James Greenhalgh]

On Mon, Jul 27, 2015 at 02:22:41PM +0100, Pawel Kupidura wrote:
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 10df325..ffafc3f 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,7 @@
> +2015-07-27  Pawel Kupidura <pawel.kupidura@arm.com>

Two spaces between your name and your email address, like so:

2015-07-27  Pawel Kupidura  <pawel.kupidura@arm.com>

> +
> +    * config/aarch64/aarch64.c: Changed inner loop statement cost
> +    to be consistent with vectorizer code.
> +

s/Changed/Change

>   2015-07-26  Uros Bizjak <ubizjak@gmail.com>
> 
>       * config/alpha/alpha.c: Use SUBREG_P predicate.
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 020f63c..3b6f8c5 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -7079,15 +7079,9 @@ aarch64_add_stmt_cost (void *data, int count, 
> enum vect_cost_for_stmt kind,
> 
>         /* Statements in an inner loop relative to the loop being
>        vectorized are weighted more heavily.  The value here is
> -     a function (linear for now) of the loop nest level.  */
> +     arbitrary and could potentially be improved with analysis.  */

Your mail client has mangled the tabs in this diff, so the patch will
not apply in this form. Could you try posting again having resolved the
issues with your mail client?

>         if (where == vect_body && stmt_info && stmt_in_inner_loop_p 
> (stmt_info))
> -    {
> -      loop_vec_info loop_info = STMT_VINFO_LOOP_VINFO (stmt_info);
> -      struct loop *loop =  LOOP_VINFO_LOOP (loop_info);
> -      unsigned nest_level = loop_depth (loop);
> -
> -      count *= nest_level;
> -    }
> +    count *= 50; /* FIXME */

Likewise here.

Thanks,
James

Re: [PATCH][AArch64] Change aarch64 vector cost to match vectorizer

[Pawel Kupidura]

On 03/08/15 17:26, James Greenhalgh wrote:
> On Mon, Jul 27, 2015 at 02:22:41PM +0100, Pawel Kupidura wrote:
>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>> index 10df325..ffafc3f 100644
>> --- a/gcc/ChangeLog
>> +++ b/gcc/ChangeLog
>> @@ -1,3 +1,7 @@
>> +2015-07-27  Pawel Kupidura<pawel.kupidura@arm.com>
>
> Two spaces between your name and your email address, like so:
>
> 2015-07-27  Pawel Kupidura<pawel.kupidura@arm.com>
>
>> +
>> +    * config/aarch64/aarch64.c: Changed inner loop statement cost
>> +    to be consistent with vectorizer code.
>> +
>
> s/Changed/Change
>
>>    2015-07-26  Uros Bizjak<ubizjak@gmail.com>
>>
>>        * config/alpha/alpha.c: Use SUBREG_P predicate.
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index 020f63c..3b6f8c5 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -7079,15 +7079,9 @@ aarch64_add_stmt_cost (void *data, int count,
>> enum vect_cost_for_stmt kind,
>>
>>          /* Statements in an inner loop relative to the loop being
>>         vectorized are weighted more heavily.  The value here is
>> -     a function (linear for now) of the loop nest level.  */
>> +     arbitrary and could potentially be improved with analysis.  */
>
> Your mail client has mangled the tabs in this diff, so the patch will
> not apply in this form. Could you try posting again having resolved the
> issues with your mail client?
>
>>          if (where == vect_body&&  stmt_info&&  stmt_in_inner_loop_p
>> (stmt_info))
>> -    {
>> -      loop_vec_info loop_info = STMT_VINFO_LOOP_VINFO (stmt_info);
>> -      struct loop *loop =  LOOP_VINFO_LOOP (loop_info);
>> -      unsigned nest_level = loop_depth (loop);
>> -
>> -      count *= nest_level;
>> -    }
>> +    count *= 50; /* FIXME */
>
> Likewise here.
>
> Thanks,
> James
>
Hi,

I'm sorry about the issues with formatting, it should be fixed now. 
Here's corrected version with diff to current trunk.

Thanks,
Pawel Kupidura

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index fdc4a7e..d1c6663 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,7 @@
+2015-08-04  Pawel Kupidura  <pawel.kupidura@arm.com>
+	* config/aarch64/aarch64.c: Change inner loop statement cost
+	to be consistent with other targets.
+
  2015-08-03  Abe Skolnik  <a.skolnik@samsung.com>

  	* tree-if-conv.c: Fix various typos in comments.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 2b1ae36..173a385 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -7086,15 +7086,9 @@ aarch64_add_stmt_cost (void *data, int count, 
enum vect_cost_for_stmt kind,

        /* Statements in an inner loop relative to the loop being
  	 vectorized are weighted more heavily.  The value here is
-	 a function (linear for now) of the loop nest level.  */
+	 arbitrary and could potentially be improved with analysis.  */
        if (where == vect_body && stmt_info && stmt_in_inner_loop_p 
(stmt_info))
-	{
-	  loop_vec_info loop_info = STMT_VINFO_LOOP_VINFO (stmt_info);
-	  struct loop *loop =  LOOP_VINFO_LOOP (loop_info);
-	  unsigned nest_level = loop_depth (loop);
-
-	  count *= nest_level;
-	}
+	count *= 50; /*  FIXME  */

        retval = (unsigned) (count * stmt_cost);
        cost[where] += retval;

Re: [PATCH][AArch64] Change aarch64 vector cost to match vectorizer

[James Greenhalgh]

On Tue, Aug 04, 2015 at 11:06:11AM +0100, Pawel Kupidura wrote:
> Hi,
> 
> I'm sorry about the issues with formatting, it should be fixed now. 
> Here's corrected version with diff to current trunk.

Hi Pawel,

I'm still having trouble getting this patch to apply, I'm not sure whether
it is the format=flowed in your mail headers, or the quoted-printable
encoding, or something else. Certainly when I open your emails I see :

        if (where == vect_body && stmt_info && stmt_in_inner_loop_p 
(stmt_info))

The content of the patch is OK to commit, but it would be good to
have a copy on list that can be easily applied.

Thanks,
James

> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index fdc4a7e..d1c6663 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,7 @@
> +2015-08-04  Pawel Kupidura  <pawel.kupidura@arm.com>
> +	* config/aarch64/aarch64.c: Change inner loop statement cost
> +	to be consistent with other targets.
> +
>   2015-08-03  Abe Skolnik  <a.skolnik@samsung.com>
> 
>   	* tree-if-conv.c: Fix various typos in comments.
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 2b1ae36..173a385 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -7086,15 +7086,9 @@ aarch64_add_stmt_cost (void *data, int count, 
> enum vect_cost_for_stmt kind,
> 
>         /* Statements in an inner loop relative to the loop being
>   	 vectorized are weighted more heavily.  The value here is
> -	 a function (linear for now) of the loop nest level.  */
> +	 arbitrary and could potentially be improved with analysis.  */
>         if (where == vect_body && stmt_info && stmt_in_inner_loop_p 
> (stmt_info))
> -	{
> -	  loop_vec_info loop_info = STMT_VINFO_LOOP_VINFO (stmt_info);
> -	  struct loop *loop =  LOOP_VINFO_LOOP (loop_info);
> -	  unsigned nest_level = loop_depth (loop);
> -
> -	  count *= nest_level;
> -	}
> +	count *= 50; /*  FIXME  */
> 
>         retval = (unsigned) (count * stmt_cost);
>         cost[where] += retval;

Re: [PATCH][AArch64] Change aarch64 vector cost to match vectorizer

[Pawel Kupidura]

On 04/08/15 11:48, James Greenhalgh wrote:
> On Tue, Aug 04, 2015 at 11:06:11AM +0100, Pawel Kupidura wrote:
>> Hi,
>>
>> I'm sorry about the issues with formatting, it should be fixed now. 
>> Here's corrected version with diff to current trunk.
> 
> Hi Pawel,
> 
> I'm still having trouble getting this patch to apply, I'm not sure whether
> it is the format=flowed in your mail headers, or the quoted-printable
> encoding, or something else. Certainly when I open your emails I see :
> 
>         if (where == vect_body && stmt_info && stmt_in_inner_loop_p 
> (stmt_info))
> 
> The content of the patch is OK to commit, but it would be good to
> have a copy on list that can be easily applied.
> 
> Thanks,
> James
> 
>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>> index fdc4a7e..d1c6663 100644
>> --- a/gcc/ChangeLog
>> +++ b/gcc/ChangeLog
>> @@ -1,3 +1,7 @@
>> +2015-08-04  Pawel Kupidura  <pawel.kupidura@arm.com>
>> +	* config/aarch64/aarch64.c: Change inner loop statement cost
>> +	to be consistent with other targets.
>> +
>>   2015-08-03  Abe Skolnik  <a.skolnik@samsung.com>
>>
>>   	* tree-if-conv.c: Fix various typos in comments.
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index 2b1ae36..173a385 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -7086,15 +7086,9 @@ aarch64_add_stmt_cost (void *data, int count, 
>> enum vect_cost_for_stmt kind,
>>
>>         /* Statements in an inner loop relative to the loop being
>>   	 vectorized are weighted more heavily.  The value here is
>> -	 a function (linear for now) of the loop nest level.  */
>> +	 arbitrary and could potentially be improved with analysis.  */
>>         if (where == vect_body && stmt_info && stmt_in_inner_loop_p 
>> (stmt_info))
>> -	{
>> -	  loop_vec_info loop_info = STMT_VINFO_LOOP_VINFO (stmt_info);
>> -	  struct loop *loop =  LOOP_VINFO_LOOP (loop_info);
>> -	  unsigned nest_level = loop_depth (loop);
>> -
>> -	  count *= nest_level;
>> -	}
>> +	count *= 50; /*  FIXME  */
>>
>>         retval = (unsigned) (count * stmt_cost);
>>         cost[where] += retval;

Hi,

The issue was flowed format forced by mail client. I've tested it and the patch should apply now. 

Thanks,
Pawel 

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 28a55d5..c8b94d6 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,7 @@
+2015-08-04  Pawel Kupidura  <pawel.kupidura@arm.com>
+	* config/aarch64/aarch64.c: Change inner loop statement cost
+	to be consistent with other targets.
+
 2015-08-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
 
 	* config/aarch64/aarch64.c (aarch64_tribools_ok_for_inlining_p):
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 6b418a7..5727bc7 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -7091,15 +7091,9 @@ aarch64_add_stmt_cost (void *data, int count, enum vect_cost_for_stmt kind,
 
       /* Statements in an inner loop relative to the loop being
 	 vectorized are weighted more heavily.  The value here is
-	 a function (linear for now) of the loop nest level.  */
+	 arbitrary and could potentially be improved with analysis.  */
       if (where == vect_body && stmt_info && stmt_in_inner_loop_p (stmt_info))
-	{
-	  loop_vec_info loop_info = STMT_VINFO_LOOP_VINFO (stmt_info);
-	  struct loop *loop =  LOOP_VINFO_LOOP (loop_info);
-	  unsigned nest_level = loop_depth (loop);
-
-	  count *= nest_level;
-	}
+	count *= 50; /*  FIXME  */
 
       retval = (unsigned) (count * stmt_cost);
       cost[where] += retval;

Re: [PATCH][AArch64] Change aarch64 vector cost to match vectorizer

[Richard Sandiford]

Pawel Kupidura <Pawel.Kupidura@arm.com> writes:
> Hi,
>
> The issue was flowed format forced by mail client. I've tested it and
> the patch should apply now.

Thanks, applied based on James's OK.

Richard