Message ID | 1549ba52-153b-6bf1-28f6-5a1d2a2562fd@linaro.org |
---|---|
State | New |
Headers | show |
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
> gcc/testsuite/ChangeLog: > > 2016-10-25 Kugan Vivekanandarajah <kuganv@linaro.org> > > * gcc.dg/ipa/vrp7.c: New test. > > > gcc/ChangeLog: > > 2016-10-25 Kugan Vivekanandarajah <kuganv@linaro.org> > > * ipa-cp.c (ipa_get_jf_pass_through_result): Skip unary expressions. > (propagate_vr_accross_jump_function): Handle unary expressions. > (propagate_constants_accross_call): Pass param type to > propagate_vr_accross_jump_function. > * ipa-prop.c (load_from_param): Renamed from load_from_unmodified_param. > Also handles unary expr. > (ipa_set_jf_unary_pass_through): New. > (ipa_load_from_parm_agg): Renamed load_from_unmodified_param. > (compute_complex_assign_jump_func): Handle unary expressions. > (ipa_write_jump_function): Likewise. > (ipa_read_jump_function): Likewise. > * ipa-prop.h: export ipa_get_callee_param_type. > 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. */ handled. I would expect them to be already folded here? I would expect that hanlding unary expressions in constant propagation is no harder than for VRP? > + || (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) New param needs comment. Patch is OK with changes Martin suggested. Honza
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) { 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), + 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)); 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) { int index; tree op1; - if (!gimple_assign_single_p (stmt)) + if (!is_gimple_assign (stmt)) return -1; 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, 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) + { + streamer_write_uhwi (ob, jump_func->value.pass_through.formal_id); + } else { stream_write_tree (ob, jump_func->value.pass_through.operand, true); @@ -4742,6 +4768,11 @@ ipa_read_jump_function (struct lto_input_block *ib, bool agg_preserved = bp_unpack_value (&bp, 1); ipa_set_jf_simple_pass_through (jump_func, formal_id, agg_preserved); } + else if (TREE_CODE_CLASS (operation) == tcc_unary) + { + int formal_id = streamer_read_uhwi (ib); + ipa_set_jf_unary_pass_through (jump_func, formal_id, operation); + } else { tree operand = stream_read_tree (ib, data_in); diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index 4eeae88..0e75cf4 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -818,6 +818,7 @@ ipa_parm_adjustment *ipa_get_adjustment_candidate (tree **, bool *, ipa_parm_adjustment_vec, bool); void ipa_release_body_info (struct ipa_func_body_info *); +tree ipa_get_callee_param_type (struct cgraph_edge *e, int i); /* From tree-sra.c: */ tree build_ref_for_offset (location_t, tree, HOST_WIDE_INT, bool, tree, diff --git a/gcc/testsuite/gcc.dg/ipa/vrp7.c b/gcc/testsuite/gcc.dg/ipa/vrp7.c index e69de29..e4e0bc6 100644 --- a/gcc/testsuite/gcc.dg/ipa/vrp7.c +++ b/gcc/testsuite/gcc.dg/ipa/vrp7.c @@ -0,0 +1,32 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-ipa-cp-details" } */ + +volatile int cond; +int abs (int); + +static __attribute__((noinline, noclone)) +int foo (int i) +{ + if (i < 5) + __builtin_abort (); + return 0; +} + +static __attribute__((noinline, noclone)) +int bar (int j) +{ + foo (~j); + foo (abs (j)); + foo (j); + return 0; +} + +int main () +{ + for (unsigned int i = 0; i < 10; ++i) + bar (i); + + return 0; +} + +/* { dg-final { scan-ipa-dump-times "Setting value range of param 0 \\\[-10, 9\\\]" 1 "cp" } } */