Thanks for the review.
On 23/05/2016 11:35, "Richard Biener" <richard.guent...@gmail.com> wrote:
>
>@@ -6332,79 +6324,81 @@ vectorizable_live_operation (gimple *stmt,
> stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
> loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_info);
> struct loop *loop = LOOP_VINFO_LOOP (loop_vinfo);
>- tree op;
>- gimple *def_stmt;
>- ssa_op_iter iter;
>+ imm_use_iterator imm_iter;
>+ tree lhs, lhs_type, vec_lhs;
>+ tree vectype = STMT_VINFO_VECTYPE (stmt_info);
>+ int nunits = TYPE_VECTOR_SUBPARTS (vectype);
>+ int ncopies = LOOP_VINFO_VECT_FACTOR (loop_vinfo) / nunits;
>+ gimple *use_stmt;
>
> gcc_assert (STMT_VINFO_LIVE_P (stmt_info));
>
>+ if (STMT_VINFO_TYPE (stmt_info) == reduc_vec_info_type)
>+ return true;
>+
>
>This is an odd check - it says the stmt is handled by
>vectorizable_reduction. And your
>return claims it is handled by vectorizable_live_operation ...
Previously this check was made to decide whether to call
vectorizable_live_operation,
So it made sense to put this check inside the function.
But, yes, I agree that the return value of the function no longer makes
sense.
I can revert this.
>
>You removed the SIMD lane handling?
The SIMD lane handling effectively checked for a special case, then added
code which would extract the final value of the vector.
The new code I’ve added does the exact same thing for more generic cases,
so the SIMD check can be removed and it’ll still be vectorized correctly.
>
>@@ -303,6 +335,16 @@ vect_stmt_relevant_p (gimple *stmt, loop_vec_info
>loop_vinfo,
> }
> }
>
>+ if (*live_p && *relevant == vect_unused_in_scope
>+ && !is_simple_and_all_uses_invariant (stmt, loop_vinfo))
>+ {
>+ if (dump_enabled_p ())
>+ dump_printf_loc (MSG_NOTE, vect_location,
>+ "vec_stmt_relevant_p: live and not all uses "
>+ "invariant.\n");
>+ *relevant = vect_used_only_live;
>+ }
>
>But that's a missed invariant motion / code sinking opportunity then.
>Did you have a
>testcase for this?
I don’t have a test case :(
It made sense that this was the correct action to do on the failure
(rather than assert).
>
>@@ -618,57 +660,31 @@ vect_mark_stmts_to_be_vectorized (loop_vec_info
>loop_vinfo)
> }
>
> /* Examine the USEs of STMT. For each USE, mark the stmt that
>defines it
>- (DEF_STMT) as relevant/irrelevant and live/dead according to the
>- liveness and relevance properties of STMT. */
>+ (DEF_STMT) as relevant/irrelevant according to the relevance
>property
>+ of STMT. */
> stmt_vinfo = vinfo_for_stmt (stmt);
> relevant = STMT_VINFO_RELEVANT (stmt_vinfo);
>- live_p = STMT_VINFO_LIVE_P (stmt_vinfo);
>-
>- /* Generally, the liveness and relevance properties of STMT are
>- propagated as is to the DEF_STMTs of its USEs:
>- live_p <-- STMT_VINFO_LIVE_P (STMT_VINFO)
>- relevant <-- STMT_VINFO_RELEVANT (STMT_VINFO)
>-
>- One exception is when STMT has been identified as defining a
>reduction
>- variable; in this case we set the liveness/relevance as follows:
>- live_p = false
>- relevant = vect_used_by_reduction
>- This is because we distinguish between two kinds of relevant
>stmts -
>- those that are used by a reduction computation, and those that
>are
>- (also) used by a regular computation. This allows us later on to
>- identify stmts that are used solely by a reduction, and
>therefore the
>- order of the results that they produce does not have to be kept.
> */
>-
>- def_type = STMT_VINFO_DEF_TYPE (stmt_vinfo);
>- tmp_relevant = relevant;
>- switch (def_type)
>+
>+ switch (STMT_VINFO_DEF_TYPE (stmt_vinfo))
> {
>
>you removed this comment. Is it no longer valid? Can you please
>instead update it?
>This is a tricky area.
I’ll replace with a new comment.
>
>
>@@ -1310,17 +1325,14 @@ vect_init_vector (gimple *stmt, tree val, tree
>type, gimple_stmt_iterator *gsi)
> In case OP is an invariant or constant, a new stmt that creates a
>vector def
> needs to be introduced. VECTYPE may be used to specify a required
>type for
> vector invariant. */
>-
>-tree
>-vect_get_vec_def_for_operand (tree op, gimple *stmt, tree vectype)
>+static tree
>+vect_get_vec_def_for_operand_internal (tree op, gimple *stmt,
>+ loop_vec_info loop_vinfo, tree
>vectype)
> {
> tree vec_oprnd;
>...
>
>+tree
>+vect_get_vec_def_for_operand (tree op, gimple *stmt, tree vectype)
>+{
>+ stmt_vec_info stmt_vinfo = vinfo_for_stmt (stmt);
>+ loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_vinfo);
>+ return vect_get_vec_def_for_operand_internal (op, stmt, loop_vinfo,
>vectype);
>+}
>+
>+tree
>+vect_get_vec_def_for_operand_outside (tree op, loop_vec_info loop_vinfo)
>+{
>+ return vect_get_vec_def_for_operand_internal (op, NULL, loop_vinfo,
>NULL);
>+}
>
>I was trying to reduce the number of _get_vec_def variants, now you
>add another one :/
>
>I think as cleanup it would be nice to pass down loop_vinfo directly
>(or rather the
>more generic vec_info as needed by vect_is_simple_use). The stmt_vectype
>hack
>that was required for bools should ideally be dropped as well as the
>constant/external
>path inserting init stmts via vect_init_vector (we shouldn't require
>stmt for vect_get_vec_def_for_operand
>at all).
Ok, I’ll clean this up.
>
>Note that I think you mishandle SLP with your patch as
>vect_get_vec_def_for_operand does not
>work (reliably at least?) for pure-SLP defs.
>vectorizable_live_operation needs access to the SLP tree.
>I tried to merge vect_get_slp_defs (and its various variants) and
>vect_get_vec_def_for_opernad (and its various variants) last stage3
>but ran out of
>time with the gazillions of cleanup opportunities that were surfacing ;)
I’ll investigate this a bit more and get back to you.
>
>Still overall I like the patch. Can you trim it down by first not
>handling the is_simple_and_all_uses_invariant
>case?
Do you want two patches where the first patch adds the vect_used_only_live
relevant state but does not change which statements are vectorized
(vectorised_live_operation is not changed).
The second patch would allow live operations to be vectorized.
?
