vec: remove unreachable code

Message ID gkro8ml3foc.fsf@arm.com
State New
Headers show
Series
  • vec: remove unreachable code
Related show

Commit Message

Andrea Corallo Sept. 4, 2020, 12:11 p.m.
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
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(-)

-- 
2.17.1

Comments

Richard Biener Sept. 4, 2020, 12:12 p.m. | #1
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.
Andrea Corallo Sept. 4, 2020, 12:20 p.m. | #2
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
Harald Anlauf via Gcc-patches Sept. 4, 2020, 3:37 p.m. | #3
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
Richard Sandiford Sept. 7, 2020, 11:16 a.m. | #4
"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

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
     {