Hi,
On Tue, Oct 25, 2016 at 10:18:25AM +1100, kugan wrote:
> Hi,
>
> Attached RFC patch handles unary pass-through jump functions for ipa-vrp
> such that in the following case:
>
> int bar (int j)
> {
> foo (~j);
> foo (abs (j));
> foo (j);
> return 0;
> }
Thanks for working on this. Although I cannot approve your patches, I
do have a few comments inline:
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 1dc5cb6..d0dc3d7 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -122,6 +122,7 @@ along with GCC; see the file COPYING3. If not see
> #include "ipa-inline.h"
> #include "ipa-utils.h"
> #include "tree-ssa-ccp.h"
> +#include "gimple.h"
>
> template <typename valtype> class ipcp_value;
>
> @@ -1221,7 +1222,12 @@ ipa_get_jf_pass_through_result (struct ipa_jump_func
> *jfunc, tree input)
>
> if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
> return input;
> - if (!is_gimple_ip_invariant (input))
> +
> + if (!is_gimple_ip_invariant (input)
> + /* TODO: Unary expressions are not handles in ipa constant
> + propagation. */
> + || (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> + == tcc_unary))
> return NULL_TREE;
>
> if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> @@ -1845,7 +1851,8 @@ propagate_bits_accross_jump_function (cgraph_edge *cs,
> int idx, ipa_jump_func *j
> static bool
> propagate_vr_accross_jump_function (cgraph_edge *cs,
> ipa_jump_func *jfunc,
> - struct ipcp_param_lattices *dest_plats)
> + struct ipcp_param_lattices *dest_plats,
> + tree param_type)
Please also add a brief description of the new parameter to the
function comment.
> {
> struct ipcp_param_lattices *src_lats;
> ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
> @@ -1863,18 +1870,43 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>
> if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
> return dest_lat->meet_with (src_lats->m_value_range);
> + else if (param_type
> + && (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> + == tcc_unary))
> + {
> + value_range vr;
> + memset (&vr, 0, sizeof (vr));
> + tree operand_type = ipa_get_type (caller_info, src_idx);
> +
> + if (src_lats->m_value_range.bottom_p ())
> + return false;
> +
> + extract_range_from_unary_expr (&vr,
> + ipa_get_jf_pass_through_operation
> (jfunc),
Here you get over 80 characters of text width. Tough to deal with, I
know, but please fix :-)
> + param_type,
> + &src_lats->m_value_range.m_vr,
> + operand_type);
> + return dest_lat->meet_with (&vr);
> + }
> }
> - else if (jfunc->type == IPA_JF_CONST)
> + else if (param_type
> + && jfunc->type == IPA_JF_CONST)
> {
> tree val = ipa_get_jf_constant (jfunc);
> if (TREE_CODE (val) == INTEGER_CST)
> {
> + value_range vr;
> if (TREE_OVERFLOW_P (val))
> val = drop_tree_overflow (val);
> - jfunc->vr_known = true;
> - jfunc->m_vr.type = VR_RANGE;
> - jfunc->m_vr.min = val;
> - jfunc->m_vr.max = val;
> +
> + vr.type = VR_RANGE;
> + vr.min = val;
> + vr.max = val;
> + vr.equiv = NULL;
> + extract_range_from_unary_expr (&jfunc->m_vr,
> + NOP_EXPR,
> + param_type,
> + &vr, TREE_TYPE (val));
Do I understand it correctly that extract_range_from_unary_expr deals
with any potential type conversions better (compared to what you did
before here)?
Side note: I wonder whether it is a good idea to store the
intermediary result to the jump function. I'd prefer if we did that
only after actually making transformation decisions, if at all. But
that is only a minor nit and not something that needs to be addressed
as a part of this change.
> return dest_lat->meet_with (&jfunc->m_vr);
> }
> }
> @@ -2220,6 +2252,7 @@ propagate_constants_accross_call (struct cgraph_edge
> *cs)
> {
> struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
> struct ipcp_param_lattices *dest_plats;
> + tree param_type = ipa_get_callee_param_type (cs, i);
>
> dest_plats = ipa_get_parm_lattices (callee_info, i);
> if (availability == AVAIL_INTERPOSABLE)
> @@ -2236,7 +2269,8 @@ propagate_constants_accross_call (struct cgraph_edge
> *cs)
> dest_plats);
> if (opt_for_fn (callee->decl, flag_ipa_vrp))
> ret |= propagate_vr_accross_jump_function (cs,
> - jump_func, dest_plats);
> + jump_func, dest_plats,
> + param_type);
> else
> ret |= dest_plats->m_value_range.set_to_bottom ();
> }
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 1629870..9147b27 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -446,6 +446,18 @@ ipa_set_jf_simple_pass_through (struct ipa_jump_func
> *jfunc, int formal_id,
> jfunc->value.pass_through.agg_preserved = agg_preserved;
> }
>
> +/* Set JFUNC to be an unary pass through jump function. */
> +
> +static void
> +ipa_set_jf_unary_pass_through (struct ipa_jump_func *jfunc, int formal_id,
> + enum tree_code operation)
> +{
> + jfunc->type = IPA_JF_PASS_THROUGH;
> + jfunc->value.pass_through.operand = NULL_TREE;
> + jfunc->value.pass_through.formal_id = formal_id;
> + jfunc->value.pass_through.operation = operation;
> + jfunc->value.pass_through.agg_preserved = false;
> +}
> /* Set JFUNC to be an arithmetic pass through jump function. */
>
> static void
> @@ -850,18 +862,17 @@ parm_preserved_before_stmt_p (struct ipa_func_body_info
> *fbi, int index,
> }
>
> /* If STMT is an assignment that loads a value from an parameter declaration,
> - return the index of the parameter in ipa_node_params which has not been
> - modified. Otherwise return -1. */
> + return the index of the parameter in ipa_node_params. Otherwise return
> -1. */
>
> static int
> -load_from_unmodified_param (struct ipa_func_body_info *fbi,
> - vec<ipa_param_descriptor> descriptors,
> - gimple *stmt)
> +load_from_param (struct ipa_func_body_info *fbi,
> + vec<ipa_param_descriptor> descriptors,
> + gimple *stmt)
This is the only thing I dislike in this patch, I would like to keep
the ability to really know that we are looking at a load of an
unmodified parameter for cases when it is required/useful (e.g. in
ipa_load_from_parm_agg which I suppose "works" because there are not
that many unary operations on pointers).
Please either provide two distinct functions (sharing the common parts
in a load_from_param_1 function) or add a parameter that will
differentiate between unmodified and unary-op-modified parameters.
One way could be that the that parameter would be pointer to an enum
tree_code in which the function will return the OP for you if non-NULL
and will insist on a GIMPLE_SINGLE_RHS if it is NULL.
> {
> int index;
> tree op1;
>
> - if (!gimple_assign_single_p (stmt))
> + if (!is_gimple_assign (stmt))
> return -1;
Even in the modifying case, I still think that you want to check any
operation embedded in the statement is GIMPLE_UNARY_RHS or
GIMPLE_SINGLE_RHS.
>
> op1 = gimple_assign_rhs1 (stmt);
> @@ -1025,7 +1036,7 @@ ipa_load_from_parm_agg (struct ipa_func_body_info *fbi,
> */
>
> gimple *def = SSA_NAME_DEF_STMT (TREE_OPERAND (base, 0));
> - index = load_from_unmodified_param (fbi, descriptors, def);
> + index = load_from_param (fbi, descriptors, def);
> }
>
> if (index >= 0)
> @@ -1109,6 +1120,7 @@ compute_complex_assign_jump_func (struct
> ipa_func_body_info *fbi,
After you add this new capability to the function, its comment will
need an update.
> tree op1, tc_ssa, base, ssa;
> bool reverse;
> int index;
> + gimple *stmt2 = stmt;
>
> op1 = gimple_assign_rhs1 (stmt);
>
> @@ -1117,13 +1129,16 @@ compute_complex_assign_jump_func (struct
> ipa_func_body_info *fbi,
> if (SSA_NAME_IS_DEFAULT_DEF (op1))
> index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1));
> else
> - index = load_from_unmodified_param (fbi, info->descriptors,
> - SSA_NAME_DEF_STMT (op1));
> + {
> + index = load_from_param (fbi, info->descriptors,
> + SSA_NAME_DEF_STMT (op1));
> + stmt2 = SSA_NAME_DEF_STMT (op1);
> + }
> tc_ssa = op1;
> }
> else
> {
> - index = load_from_unmodified_param (fbi, info->descriptors, stmt);
> + index = load_from_param (fbi, info->descriptors, stmt);
> tc_ssa = gimple_assign_lhs (stmt);
> }
>
> @@ -1147,6 +1162,13 @@ compute_complex_assign_jump_func (struct
> ipa_func_body_info *fbi,
> bool agg_p = parm_ref_data_pass_through_p (fbi, index, call, tc_ssa);
> ipa_set_jf_simple_pass_through (jfunc, index, agg_p);
> }
> + else if (is_gimple_assign (stmt2)
> + && (gimple_expr_code (stmt2) != NOP_EXPR)
> + && (TREE_CODE_CLASS (gimple_expr_code (stmt2)) == tcc_unary))
> + {
> + ipa_set_jf_unary_pass_through (jfunc, index,
> + gimple_assign_rhs_code (stmt2));
> + }
> return;
> }
>
> @@ -1595,7 +1617,7 @@ determine_locally_known_aggregate_parts (gcall *call,
> tree arg,
> }
> }
>
> -static tree
> +tree
> ipa_get_callee_param_type (struct cgraph_edge *e, int i)
> {
> int n;
> @@ -4663,6 +4685,10 @@ ipa_write_jump_function (struct output_block *ob,
> bp_pack_value (&bp, jump_func->value.pass_through.agg_preserved, 1);
> streamer_write_bitpack (&bp);
> }
> + else if (TREE_CODE_CLASS (jump_func->value.pass_through.operation) ==
> tcc_unary)
This is quite clearly over 80 characters wide.
Apart from the above, I'll be happy to see this in.
Thanks,
Martin
