Re: [PATCH 1/2] Minor refactoring of tree-vect-patterns.c
On Mon, Apr 30, 2012 at 6:19 PM, Ulrich Weigand uweig...@de.ibm.com wrote:
Hello,
in working on a fix for PR 52633, I noticed that tree-vect-patterns.c now
contains a number of copies of rather similar code (of which my patch
would have added another copy), so it seems to make sense to do a bit of
refactoring first.
This patch introduced a new helper function vect_same_loop_or_bb_p
to decide whether a new statement is in the same loop or basic-block
(depending on whether we're currently doing loop-based or basic-block-based
vectorization) as an existing statement. The helper is then used everywhere
where this test is currently open-coded.
As a side-effect, the patch actually contains two bug fixes:
- The condition in some places
(!loop gimple_bb (def_stmt) != BB_VINFO_BB (bb_vinfo)
gimple_code (def_stmt) != GIMPLE_PHI)
doesn't seem to make sense. It allows PHI statements from random
other basic blocks -- but those will have an uninitialized
vinfo_for_stmt, so if that test ever hits, we're going to crash.
The check was introduced in this patch:
http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00191.html
which likewise introduces a similar check in vect_get_and_check_slp_defs:
(!loop gimple_bb (def_stmt) == BB_VINFO_BB (bb_vinfo)
gimple_code (def_stmt) != GIMPLE_PHI))
This makes sense: basic-block vectorization operates only on statements
in the current basic block, but *not* on the incoming PHIs.
It seems that the condition simply was incorrectly negated when moving
it over to the tree-vect-pattern.c uses.
- In vect_recog_widen_shift_pattern, the test for same loop / basic-block
is still missing. In my recent patch to PR 52870 I added the check to
vect_recog_widen_mult_pattern ... it is still missing in ...shift_pattern.
Tested on i386-linux-gnu and arm-linux-gnueabi with no regressions.
OK for mainline?
Ok.
Thanks,
Richard.
Bye,
Ulrich
ChangeLog:
* tree-vect-patterns.c (vect_same_loop_or_bb_p): New function.
(vect_handle_widen_op_by_const): Use it instead of open-coding test.
(vect_recog_widen_mult_pattern): Likewise.
(vect_operation_fits_smaller_type): Likewise.
(vect_recog_over_widening_pattern): Likewise.
(vect_recog_widen_shift_pattern): Add to vect_same_loop_or_bb_p test.
Index: gcc-head/gcc/tree-vect-patterns.c
===
--- gcc-head.orig/gcc/tree-vect-patterns.c 2012-04-26 15:53:48.0
+0200
+++ gcc-head/gcc/tree-vect-patterns.c 2012-04-26 19:46:12.0 +0200
@@ -84,6 +84,41 @@ new_pattern_def_seq (stmt_vec_info stmt_
append_pattern_def_seq (stmt_info, stmt);
}
+/* Check whether STMT2 is in the same loop or basic block as STMT1.
+ Which of the two applies depends on whether we're currently doing
+ loop-based or basic-block-based vectorization, as determined by
+ the vinfo_for_stmt for STMT1 (which must be defined).
+
+ If this returns true, vinfo_for_stmt for STMT2 is guaranteed
+ to be defined as well. */
+
+static bool
+vect_same_loop_or_bb_p (gimple stmt1, gimple stmt2)
+{
+ stmt_vec_info stmt_vinfo = vinfo_for_stmt (stmt1);
+ loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_vinfo);
+ bb_vec_info bb_vinfo = STMT_VINFO_BB_VINFO (stmt_vinfo);
+
+ if (!gimple_bb (stmt2))
+ return false;
+
+ if (loop_vinfo)
+ {
+ struct loop *loop = LOOP_VINFO_LOOP (loop_vinfo);
+ if (!flow_bb_inside_loop_p (loop, gimple_bb (stmt2)))
+ return false;
+ }
+ else
+ {
+ if (gimple_bb (stmt2) != BB_VINFO_BB (bb_vinfo)
+ || gimple_code (stmt2) == GIMPLE_PHI)
+ return false;
+ }
+
+ gcc_assert (vinfo_for_stmt (stmt2));
+ return true;
+}
+
/* Check whether NAME, an ssa-name used in USE_STMT,
is a result of a type promotion or demotion, such that:
DEF_STMT: NAME = NOP (name0)
@@ -400,16 +435,6 @@ vect_handle_widen_op_by_const (gimple st
{
tree new_type, new_oprnd, tmp;
gimple new_stmt;
- loop_vec_info loop_vinfo;
- struct loop *loop = NULL;
- bb_vec_info bb_vinfo;
- stmt_vec_info stmt_vinfo;
-
- stmt_vinfo = vinfo_for_stmt (stmt);
- loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_vinfo);
- bb_vinfo = STMT_VINFO_BB_VINFO (stmt_vinfo);
- if (loop_vinfo)
- loop = LOOP_VINFO_LOOP (loop_vinfo);
if (code != MULT_EXPR code != LSHIFT_EXPR)
return false;
@@ -425,12 +450,10 @@ vect_handle_widen_op_by_const (gimple st
return true;
}
- if (TYPE_PRECISION (type) (TYPE_PRECISION (*half_type) * 4)
- || !gimple_bb (def_stmt)
- || (loop !flow_bb_inside_loop_p (loop, gimple_bb (def_stmt)))
- || (!loop gimple_bb (def_stmt) != BB_VINFO_BB (bb_vinfo)
- gimple_code (def_stmt) != GIMPLE_PHI)
- || !vinfo_for_stmt (def_stmt))
+ if (TYPE_PRECISION (type) (TYPE_PRECISION (*half_type) * 4))
+ return
[PATCH 1/2] Minor refactoring of tree-vect-patterns.c
Hello,
in working on a fix for PR 52633, I noticed that tree-vect-patterns.c now
contains a number of copies of rather similar code (of which my patch
would have added another copy), so it seems to make sense to do a bit of
refactoring first.
This patch introduced a new helper function vect_same_loop_or_bb_p
to decide whether a new statement is in the same loop or basic-block
(depending on whether we're currently doing loop-based or basic-block-based
vectorization) as an existing statement. The helper is then used everywhere
where this test is currently open-coded.
As a side-effect, the patch actually contains two bug fixes:
- The condition in some places
(!loop gimple_bb (def_stmt) != BB_VINFO_BB (bb_vinfo)
gimple_code (def_stmt) != GIMPLE_PHI)
doesn't seem to make sense. It allows PHI statements from random
other basic blocks -- but those will have an uninitialized
vinfo_for_stmt, so if that test ever hits, we're going to crash.
The check was introduced in this patch:
http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00191.html
which likewise introduces a similar check in vect_get_and_check_slp_defs:
(!loop gimple_bb (def_stmt) == BB_VINFO_BB (bb_vinfo)
gimple_code (def_stmt) != GIMPLE_PHI))
This makes sense: basic-block vectorization operates only on statements
in the current basic block, but *not* on the incoming PHIs.
It seems that the condition simply was incorrectly negated when moving
it over to the tree-vect-pattern.c uses.
- In vect_recog_widen_shift_pattern, the test for same loop / basic-block
is still missing. In my recent patch to PR 52870 I added the check to
vect_recog_widen_mult_pattern ... it is still missing in ...shift_pattern.
Tested on i386-linux-gnu and arm-linux-gnueabi with no regressions.
OK for mainline?
Bye,
Ulrich
ChangeLog:
* tree-vect-patterns.c (vect_same_loop_or_bb_p): New function.
(vect_handle_widen_op_by_const): Use it instead of open-coding test.
(vect_recog_widen_mult_pattern): Likewise.
(vect_operation_fits_smaller_type): Likewise.
(vect_recog_over_widening_pattern): Likewise.
(vect_recog_widen_shift_pattern): Add to vect_same_loop_or_bb_p test.
Index: gcc-head/gcc/tree-vect-patterns.c
===
--- gcc-head.orig/gcc/tree-vect-patterns.c 2012-04-26 15:53:48.0
+0200
+++ gcc-head/gcc/tree-vect-patterns.c 2012-04-26 19:46:12.0 +0200
@@ -84,6 +84,41 @@ new_pattern_def_seq (stmt_vec_info stmt_
append_pattern_def_seq (stmt_info, stmt);
}
+/* Check whether STMT2 is in the same loop or basic block as STMT1.
+ Which of the two applies depends on whether we're currently doing
+ loop-based or basic-block-based vectorization, as determined by
+ the vinfo_for_stmt for STMT1 (which must be defined).
+
+ If this returns true, vinfo_for_stmt for STMT2 is guaranteed
+ to be defined as well. */
+
+static bool
+vect_same_loop_or_bb_p (gimple stmt1, gimple stmt2)
+{
+ stmt_vec_info stmt_vinfo = vinfo_for_stmt (stmt1);
+ loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_vinfo);
+ bb_vec_info bb_vinfo = STMT_VINFO_BB_VINFO (stmt_vinfo);
+
+ if (!gimple_bb (stmt2))
+return false;
+
+ if (loop_vinfo)
+{
+ struct loop *loop = LOOP_VINFO_LOOP (loop_vinfo);
+ if (!flow_bb_inside_loop_p (loop, gimple_bb (stmt2)))
+ return false;
+}
+ else
+{
+ if (gimple_bb (stmt2) != BB_VINFO_BB (bb_vinfo)
+ || gimple_code (stmt2) == GIMPLE_PHI)
+ return false;
+}
+
+ gcc_assert (vinfo_for_stmt (stmt2));
+ return true;
+}
+
/* Check whether NAME, an ssa-name used in USE_STMT,
is a result of a type promotion or demotion, such that:
DEF_STMT: NAME = NOP (name0)
@@ -400,16 +435,6 @@ vect_handle_widen_op_by_const (gimple st
{
tree new_type, new_oprnd, tmp;
gimple new_stmt;
- loop_vec_info loop_vinfo;
- struct loop *loop = NULL;
- bb_vec_info bb_vinfo;
- stmt_vec_info stmt_vinfo;
-
- stmt_vinfo = vinfo_for_stmt (stmt);
- loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_vinfo);
- bb_vinfo = STMT_VINFO_BB_VINFO (stmt_vinfo);
- if (loop_vinfo)
-loop = LOOP_VINFO_LOOP (loop_vinfo);
if (code != MULT_EXPR code != LSHIFT_EXPR)
return false;
@@ -425,12 +450,10 @@ vect_handle_widen_op_by_const (gimple st
return true;
}
- if (TYPE_PRECISION (type) (TYPE_PRECISION (*half_type) * 4)
- || !gimple_bb (def_stmt)
- || (loop !flow_bb_inside_loop_p (loop, gimple_bb (def_stmt)))
- || (!loop gimple_bb (def_stmt) != BB_VINFO_BB (bb_vinfo)
- gimple_code (def_stmt) != GIMPLE_PHI)
- || !vinfo_for_stmt (def_stmt))
+ if (TYPE_PRECISION (type) (TYPE_PRECISION (*half_type) * 4))
+return false;
+
+ if (!vect_same_loop_or_bb_p (stmt, def_stmt))
return false;
/* TYPE is 4 times bigger than HALF_TYPE, try widening operation for
@@ -564,16 +587,6 @@
