[PATCH] vec: remove unreachable code

[PATCH] vec: remove unreachable code

[Andrea Corallo]

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

Hi all,

just a small patch removing a piece of unreachable code in
'vect_estimate_min_profitable_iters' given the condition
(LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo)) is always true as
checked just above.
 
Bootstrapped on aarch64-unknown-linux-gnu.

Okay for trunk?

  Andrea


[-- Attachment #2: 0001-vec-dead-code-removal-in-tree-vect-loop.c.patch --]
[-- Type: text/plain, Size: 1469 bytes --]

From 6bd96581410d9847ab78e6761178d4efbc9a5af2 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>
Date: Fri, 4 Sep 2020 09:56:59 +0100
Subject: [PATCH] vec: dead code removal in tree-vect-loop.c

gcc/ChangeLog

2020-09-04  Andrea Corallo  <andrea.corallo@arm.com>

	* tree-vect-loop.c (vect_estimate_min_profitable_iters): Remove
	dead code as LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo) is
	always verified.
---
 gcc/tree-vect-loop.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
index 362cdc4f1cb..38576382fe7 100644
--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -4101,17 +4101,10 @@ vect_estimate_min_profitable_iters (loop_vec_info loop_vinfo,
       if (outside_overhead > 0)
 	min_vec_niters = outside_overhead / saving_per_viter + 1;
 
-      if (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo))
-	{
-	  int threshold = (vec_inside_cost * min_vec_niters
-			   + vec_outside_cost
-			   + scalar_outside_cost);
-	  min_profitable_estimate = threshold / scalar_single_iter_cost + 1;
-	}
-      else
-	min_profitable_estimate = (min_vec_niters * assumed_vf
-				   + peel_iters_prologue
-				   + peel_iters_epilogue);
+      int threshold = (vec_inside_cost * min_vec_niters
+		       + vec_outside_cost
+		       + scalar_outside_cost);
+      min_profitable_estimate = threshold / scalar_single_iter_cost + 1;
     }
   else
     {
-- 
2.17.1

Re: [PATCH] vec: remove unreachable code

[Richard Biener]

On Fri, 4 Sep 2020, Andrea Corallo wrote:

> Hi all,
> 
> just a small patch removing a piece of unreachable code in
> 'vect_estimate_min_profitable_iters' given the condition
> (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo)) is always true as
> checked just above.
>  
> Bootstrapped on aarch64-unknown-linux-gnu.
> 
> Okay for trunk?

OK.

Richard.

Re: [PATCH] vec: remove unreachable code

[Andrea Corallo]

Richard Biener <rguenther@suse.de> writes:

> On Fri, 4 Sep 2020, Andrea Corallo wrote:
>
>> Hi all,
>> 
>> just a small patch removing a piece of unreachable code in
>> 'vect_estimate_min_profitable_iters' given the condition
>> (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo)) is always true as
>> checked just above.
>>  
>> Bootstrapped on aarch64-unknown-linux-gnu.
>> 
>> Okay for trunk?
>
> OK.
>
> Richard.

1 min 4 secs, by far the quickest review I've ever got :)

Installed as 09fa6acd8d9

Thanks!

  Andrea

Re: [PATCH] vec: remove unreachable code

[Kewen.Lin]

Hi Andrea,

on 2020/9/4 下午8:11, Andrea Corallo wrote:
> Hi all,
> 
> just a small patch removing a piece of unreachable code in
> 'vect_estimate_min_profitable_iters' given the condition
> (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo)) is always true as
> checked just above.
>  

FWIW, I had the same confusion when I saw the code at the first time,
the comments at outer if "??? The "if" arm ..." and the commit messages
seem to give some hints.  IIUC, the code in this outer "if" was
implemented to handle all cases but that time was stage 4, so it's
conservative to be guarded as fully-predicated only to avoid the
potential risks.  I was imagining that it would finally end up
by removing this outer if condition guard and the outer else.
But not sure why it didn't change in the following stage1.

Maybe Richard S. (the author) will have some comments.

BR,
Kewen

Re: [PATCH] vec: remove unreachable code

[Richard Sandiford]

"Kewen.Lin" <linkw@linux.ibm.com> writes:
> Hi Andrea,
>
> on 2020/9/4 锟斤拷锟斤拷8:11, Andrea Corallo wrote:
>> Hi all,
>> 
>> just a small patch removing a piece of unreachable code in
>> 'vect_estimate_min_profitable_iters' given the condition
>> (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo)) is always true as
>> checked just above.
>>  
>
> FWIW, I had the same confusion when I saw the code at the first time,
> the comments at outer if "??? The "if" arm ..." and the commit messages
> seem to give some hints.  IIUC, the code in this outer "if" was
> implemented to handle all cases but that time was stage 4, so it's
> conservative to be guarded as fully-predicated only to avoid the
> potential risks.  I was imagining that it would finally end up
> by removing this outer if condition guard and the outer else.

Right.  So the idea was to show what the code would look like if we
removed the outer “if” condition.  We would then always calculate the
estimate based on the minimum number of profitable vector iterations
first, then convert that into a minimum number of profitable scalar
iterations.

That still seems like a promising future direction.  It would make the
estimate calculations more consistent and make it easier to compare the
cost of full-vector loops with the cost of partial-vector loops.

> But not sure why it didn't change in the following stage1.

Excess lameness on my part, sorry.

> Maybe Richard S. (the author) will have some comments.

TBH I'd prefer for the patch to be reverted.  Maybe we could add
a version of the existing:

  /* ??? The "if" arm is written to handle all cases; see below for what
     we would do for !LOOP_VINFO_USING_PARTIAL_VECTORS_P.  */

comment above:

  else if (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo))
    {

E.g.:

  /* ??? This "else if" arm is written to handle all cases; see below for
     what we would do for !LOOP_VINFO_USING_PARTIAL_VECTORS_P.  */

As:

      /* This is a repeat of the code above, but with + SOC rather
	 than - SOC.  */

says, this block is essentially repeating the logic from the earlier block,
so at the time it didn't seem like a good idea to duplicate the commentary.

Either way, the two blocks should be kept in sync, so if we want to remove
the path from this block, we should remove it from the earlier block too.
But my preference would be to keep the path in both blocks.

I'm not foolish enough to promise that I'll test/benchmark removing the
outer “if” condition in this stage 1 cycle, but I'll at least try to find
time…

Thanks,
Richard

[PATCH] vec: Revert "dead code removal in tree-vect-loop.c" and add a comment.

[Andrea Corallo]

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

Hi Richard,

this reverts the discussed patch and adds what was suggested as a
comment.

Apologies for the inconvenience, okay for trunk?

Thanks

  Andrea


[-- Attachment #2: 0001-vec-Revert-dead-code-removal-in-tree-vect-loop.c-and.patch --]
[-- Type: text/plain, Size: 1920 bytes --]

From 84dfa2d461692f445f45b3c3498f9bedba5c3848 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>
Date: Mon, 7 Sep 2020 13:45:47 +0100
Subject: [PATCH] vec: Revert "dead code removal in tree-vect-loop.c" and add a
 comment.

gcc/ChangeLog

2020-09-07  Andrea Corallo  <andrea.corallo@arm.com>

	* tree-vect-loop.c (vect_estimate_min_profitable_iters): Revert
	dead-code removal introduced by 09fa6acd8d9 + add a comment to
	clarify.
---
 gcc/tree-vect-loop.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
index 9a82573604b..5ba4e594df9 100644
--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -4103,6 +4103,8 @@ vect_estimate_min_profitable_iters (loop_vec_info loop_vinfo,
 
   if (vec_outside_cost <= 0)
     min_profitable_estimate = 0;
+  /* ??? This "else if" arm is written to handle all cases; see below for
+     what we would do for !LOOP_VINFO_USING_PARTIAL_VECTORS_P.  */
   else if (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo))
     {
       /* This is a repeat of the code above, but with + SOC rather
@@ -4115,10 +4117,17 @@ vect_estimate_min_profitable_iters (loop_vec_info loop_vinfo,
       if (outside_overhead > 0)
 	min_vec_niters = outside_overhead / saving_per_viter + 1;
 
-      int threshold = (vec_inside_cost * min_vec_niters
-		       + vec_outside_cost
-		       + scalar_outside_cost);
-      min_profitable_estimate = threshold / scalar_single_iter_cost + 1;
+      if (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo))
+	{
+	  int threshold = (vec_inside_cost * min_vec_niters
+			   + vec_outside_cost
+			   + scalar_outside_cost);
+	  min_profitable_estimate = threshold / scalar_single_iter_cost + 1;
+	}
+      else
+	min_profitable_estimate = (min_vec_niters * assumed_vf
+				   + peel_iters_prologue
+				   + peel_iters_epilogue);
     }
   else
     {
-- 
2.17.1

Re: [PATCH] vec: Revert "dead code removal in tree-vect-loop.c" and add a comment.

[Richard Sandiford]

Andrea Corallo <andrea.corallo@arm.com> writes:
> Hi Richard,
>
> this reverts the discussed patch and adds what was suggested as a
> comment.
>
> Apologies for the inconvenience, okay for trunk?

OK, thanks!  And sorry for not commenting/documenting this very well.

Richard

>
> Thanks
>
>   Andrea
>
> From 84dfa2d461692f445f45b3c3498f9bedba5c3848 Mon Sep 17 00:00:00 2001
> From: Andrea Corallo <andrea.corallo@arm.com>
> Date: Mon, 7 Sep 2020 13:45:47 +0100
> Subject: [PATCH] vec: Revert "dead code removal in tree-vect-loop.c" and add a
>  comment.
>
> gcc/ChangeLog
>
> 2020-09-07  Andrea Corallo  <andrea.corallo@arm.com>
>
> 	* tree-vect-loop.c (vect_estimate_min_profitable_iters): Revert
> 	dead-code removal introduced by 09fa6acd8d9 + add a comment to
> 	clarify.
> ---
>  gcc/tree-vect-loop.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
> index 9a82573604b..5ba4e594df9 100644
> --- a/gcc/tree-vect-loop.c
> +++ b/gcc/tree-vect-loop.c
> @@ -4103,6 +4103,8 @@ vect_estimate_min_profitable_iters (loop_vec_info loop_vinfo,
>  
>    if (vec_outside_cost <= 0)
>      min_profitable_estimate = 0;
> +  /* ??? This "else if" arm is written to handle all cases; see below for
> +     what we would do for !LOOP_VINFO_USING_PARTIAL_VECTORS_P.  */
>    else if (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo))
>      {
>        /* This is a repeat of the code above, but with + SOC rather
> @@ -4115,10 +4117,17 @@ vect_estimate_min_profitable_iters (loop_vec_info loop_vinfo,
>        if (outside_overhead > 0)
>  	min_vec_niters = outside_overhead / saving_per_viter + 1;
>  
> -      int threshold = (vec_inside_cost * min_vec_niters
> -		       + vec_outside_cost
> -		       + scalar_outside_cost);
> -      min_profitable_estimate = threshold / scalar_single_iter_cost + 1;
> +      if (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo))
> +	{
> +	  int threshold = (vec_inside_cost * min_vec_niters
> +			   + vec_outside_cost
> +			   + scalar_outside_cost);
> +	  min_profitable_estimate = threshold / scalar_single_iter_cost + 1;
> +	}
> +      else
> +	min_profitable_estimate = (min_vec_niters * assumed_vf
> +				   + peel_iters_prologue
> +				   + peel_iters_epilogue);
>      }
>    else
>      {

Re: [PATCH] vec: Revert "dead code removal in tree-vect-loop.c" and add a comment.

[Andrea Corallo]

Richard Sandiford <richard.sandiford@arm.com> writes:

> Andrea Corallo <andrea.corallo@arm.com> writes:
>> Hi Richard,
>>
>> this reverts the discussed patch and adds what was suggested as a
>> comment.
>>
>> Apologies for the inconvenience, okay for trunk?
>
> OK, thanks!  And sorry for not commenting/documenting this very well.
>
> Richard

Installed as e147bb0faad.

Thanks
  Andrea