On 30 August 2016 at 10:50, Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org> wrote: > Hi Honza, > > Here is a re-based version which also addresses the review comments. > > On 21/07/16 22:54, Jan Hubicka wrote: >>> Maybe it is better to separate value range and alignment summary >>> writing/reading to different functions. Here is another updated >>> version which does this. >> >> Makes sense to me. Note that the alignment summary propagation can be either >> handled by doing bitwise constant propagation and/or extending our value >> ranges >> by stride (as described in >> http://www.lighterra.com/papers/valuerangeprop/Patterson1995-ValueRangeProp.pdf >> I would like it to go eventually away in favour of more generic solution. >> >>> -/* If DEST_PLATS already has aggregate items, check that aggs_by_ref >>> matches >>> +/* Propagate value range across jump function JFUNC that is associated with >>> + edge CS and update DEST_PLATS accordingly. */ >>> + >>> +static bool >>> +propagate_vr_accross_jump_function (cgraph_edge *cs, >>> + ipa_jump_func *jfunc, >>> + struct ipcp_param_lattices *dest_plats) >>> +{ >>> + struct ipcp_param_lattices *src_lats; >>> + ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range; >>> + >>> + if (dest_lat->bottom_p ()) >>> + return false; >>> + >>> + if (jfunc->type == IPA_JF_PASS_THROUGH) >>> + { >>> + struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller); >>> + int src_idx = ipa_get_jf_pass_through_formal_id (jfunc); >>> + src_lats = ipa_get_parm_lattices (caller_info, src_idx); >>> + >>> + if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR) >>> + return dest_lat->meet_with (src_lats->m_value_range); >> >> Clearly we can propagate thorugh expressions here (PLUS_EXPR). I have run >> into similar issue in loop code that builds simple generic expresisons >> (like (int)ssa_name+10) and it would be nice to have easy way to deterine >> their value range based on the knowledge of SSA_NAME's valur range. >> >> Bit this is fine for initial implementaiotn for sure. > > Indeed. I will do this as a follow up. > >>> >>> +/* Look up all VR information that we have discovered and copy it over >>> + to the transformation summary. */ >>> + >>> +static void >>> +ipcp_store_vr_results (void) >>> +{ >>> + cgraph_node *node; >>> + >>> + FOR_EACH_FUNCTION_WITH_GIMPLE_BODY (node) >>> + { >>> + ipa_node_params *info = IPA_NODE_REF (node); >>> + bool found_useful_result = false; >>> + >>> + if (!opt_for_fn (node->decl, flag_ipa_vrp)) >>> + { >>> + if (dump_file) >>> + fprintf (dump_file, "Not considering %s for VR discovery " >>> + "and propagate; -fipa-ipa-vrp: disabled.\n", >>> + node->name ()); >>> + continue; >> >> I belive you need to also prevent propagation through functions copmiled with >> -fno-ipa-vrp, not only prevent any transformations. > > Do you mean the following, I was following other implementations. > > @@ -2264,6 +2430,11 @@ propagate_constants_accross_call (struct > cgraph_edge *cs) > &dest_plats->bits_lattice); > ret |= propagate_aggs_accross_jump_function (cs, jump_func, > dest_plats); > + if (opt_for_fn (callee->decl, flag_ipa_vrp)) > + ret |= propagate_vr_accross_jump_function (cs, > + jump_func, dest_plats); > + else > + ret |= dest_plats->m_value_range.set_to_bottom (); > >>> +/* Update value range of formal parameters as described in >>> + ipcp_transformation_summary. */ >>> + >>> +static void >>> +ipcp_update_vr (struct cgraph_node *node) >>> +{ >>> + tree fndecl = node->decl; >>> + tree parm = DECL_ARGUMENTS (fndecl); >>> + tree next_parm = parm; >>> + ipcp_transformation_summary *ts = ipcp_get_transformation_summary (node); >>> + if (!ts || vec_safe_length (ts->m_vr) == 0) >>> + return; >>> + const vec<ipa_vr, va_gc> &vr = *ts->m_vr; >>> + unsigned count = vr.length (); >>> + >>> + for (unsigned i = 0; i < count; ++i, parm = next_parm) >>> + { >>> + if (node->clone.combined_args_to_skip >>> + && bitmap_bit_p (node->clone.combined_args_to_skip, i)) >>> + continue; >>> + gcc_checking_assert (parm); >>> + next_parm = DECL_CHAIN (parm); >>> + tree ddef = ssa_default_def (DECL_STRUCT_FUNCTION (node->decl), >>> parm); >>> + >>> + if (!ddef || !is_gimple_reg (parm)) >>> + continue; >>> + >>> + if (cgraph_local_p (node) >> The test of cgraph_local_p seems redundant here. The analysis phase should >> not determine >> anything if function is reachable non-locally. > > Removed it. > >>> +/* Info about value ranges. */ >>> + >>> +struct GTY(()) ipa_vr >>> +{ >>> + /* The data fields below are valid only if known is true. */ >>> + bool known; >>> + enum value_range_type type; >>> + tree min; >>> + tree max; >> What is the point of representing range as trees rather than wide ints. Can >> they >> be non-constant integer? > > Changed to wide_int after adding that support. > > LTO Bootstrapped and regression tested on x86_64-linux-gnu with no new > regressions, is this OK? Hi Kugan, Just noticed a small nit - why not reuse ipa_vr in ipa_jump_func instead of adding vr_known and m_vr ?
Thanks, Prathamesh > > Thanks > Kugan
