| Message ID | 68396c67-fdcc-33ed-5463-a07502959865@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 18, 2016 at 2:38 AM, kugan <kugan.vivekanandarajah@linaro.org> wrote: > Hi, > > I was relying on ipa_get_callee_param_type to get type of parameter and then > convert arguments to this type while computing jump functions. However, in > cases like shown in PR78365, ipa_get_callee_param_type, instead of giving > up, would return the wrong type. I think the current uses of > ipa_get_callee_param_type are fine with this. > > Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it > cannot be found, then I would give up and set the jump function to varying. > > Bootstrapped and regression tested on x86_64-linux-gnu with no new > regressions. Is this OK for trunk? I think a better fix for mismatches would be to record expected "source" types in the jump functions themselves rather than trying to second-guess them. If not then please fix ipa_get_callee_param_type -- DECL_ARGUMENTS is indeed what we need to look at first to match the IL. TYPE_ARG_TYPES is merely a fallback (it doesn't have promotion applied -- see TREE_TYPE vs. DECL_ARG_TYPE on the PARM_DECLs). Richard. > Thanks, > Kugan > > gcc/testsuite/ChangeLog: > > 2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> > > PR IPA/78365 > * gcc.dg/torture/pr78365.c: New test. > > gcc/ChangeLog: > > 2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> > > PR IPA/78365 > * ipa-cp.c (propagate_constants_accross_call): get param type from > callees > DECL_ARGUMENTS if available. > * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise. > (ipcp_update_vr): Remove now redundant conversion of precision for > VR. > * ipa-prop.h: Make ipa_get_callee_param_type local again.
Hi, On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote: > Hi, > > I was relying on ipa_get_callee_param_type to get type of parameter and then > convert arguments to this type while computing jump functions. However, in > cases like shown in PR78365, ipa_get_callee_param_type, instead of giving > up, would return the wrong type. At what stage does this happen? During analysis (ipa_compute_jump_functions_for_edge) or at WPA (propagate_constants_accross_call)? Both? > I think the current uses of > ipa_get_callee_param_type are fine with this. > > Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it > cannot be found, then I would give up and set the jump function to varying. But DECL_ARGUMENTS is not available at WPA stage with LTO so your patch would make our IPA layer to optimize less with LTO. This was the reason to go through the hoops of TYPE_ARG_TYPES in the first place. If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to square one and indeed need to put the correct type in jump functions. If just preferring DECL_ARGUMENTS is enough, then changing ipa_get_callee_param_type to use that if is is available, as Richi suggested, would indeed be preferable. But if even falling back on it can cause errors, then I am not sure if it helps. In any event, thanks for diligently dealing with the fallout, Martin > > Bootstrapped and regression tested on x86_64-linux-gnu with no new > regressions. Is this OK for trunk? > > Thanks, > Kugan > > gcc/testsuite/ChangeLog: > > 2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> > > PR IPA/78365 > * gcc.dg/torture/pr78365.c: New test. > > gcc/ChangeLog: > > 2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> > > PR IPA/78365 > * ipa-cp.c (propagate_constants_accross_call): get param type from callees > DECL_ARGUMENTS if available. > * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise. > (ipcp_update_vr): Remove now redundant conversion of precision for VR. > * ipa-prop.h: Make ipa_get_callee_param_type local again.
On Wed, Nov 23, 2016 at 4:33 PM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote: >> Hi, >> >> I was relying on ipa_get_callee_param_type to get type of parameter and then >> convert arguments to this type while computing jump functions. However, in >> cases like shown in PR78365, ipa_get_callee_param_type, instead of giving >> up, would return the wrong type. > > At what stage does this happen? During analysis > (ipa_compute_jump_functions_for_edge) or at WPA > (propagate_constants_accross_call)? Both? Hmm, where does jump function compute require the callee type? In my view the jump function should record (expected-incoming-type) arg [OP X] for each call argument in its body. Thus required conversions are done at WPA propagation time. >> I think the current uses of >> ipa_get_callee_param_type are fine with this. >> >> Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it >> cannot be found, then I would give up and set the jump function to varying. > > But DECL_ARGUMENTS is not available at WPA stage with LTO so your > patch would make our IPA layer to optimize less with LTO. This was > the reason to go through the hoops of TYPE_ARG_TYPES in the first > place. > > If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to > square one and indeed need to put the correct type in jump functions. If DECL_ARGUMENTS is not available at WPA stage then I see no other way than to put the types on the jump functions. > If just preferring DECL_ARGUMENTS is enough, then changing > ipa_get_callee_param_type to use that if is is available, as Richi > suggested, would indeed be preferable. But if even falling back on it > can cause errors, then I am not sure if it helps. > > In any event, thanks for diligently dealing with the fallout, > > Martin > > >> >> Bootstrapped and regression tested on x86_64-linux-gnu with no new >> regressions. Is this OK for trunk? >> >> Thanks, >> Kugan >> >> gcc/testsuite/ChangeLog: >> >> 2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> >> >> PR IPA/78365 >> * gcc.dg/torture/pr78365.c: New test. >> >> gcc/ChangeLog: >> >> 2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> >> >> PR IPA/78365 >> * ipa-cp.c (propagate_constants_accross_call): get param type from callees >> DECL_ARGUMENTS if available. >> * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise. >> (ipcp_update_vr): Remove now redundant conversion of precision for VR. >> * ipa-prop.h: Make ipa_get_callee_param_type local again.
Hi, On 24/11/16 19:48, Richard Biener wrote: > On Wed, Nov 23, 2016 at 4:33 PM, Martin Jambor <mjambor@suse.cz> wrote: >> Hi, >> >> On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote: >>> Hi, >>> >>> I was relying on ipa_get_callee_param_type to get type of parameter and thenHi, >>> convert arguments to this type while computing jump functions. However, in >>> cases like shown in PR78365, ipa_get_callee_param_type, instead of giving >>> up, would return the wrong type. >> >> At what stage does this happen? During analysis >> (ipa_compute_jump_functions_for_edge) or at WPA >> (propagate_constants_accross_call)? Both? > > Hmm, where does jump function compute require the callee type? > In my view the jump function should record > > (expected-incoming-type) arg [OP X] > > for each call argument in its body. Thus required conversions are > done at WPA propagation time. > >>> I think the current uses of >>> ipa_get_callee_param_type are fine with this. >>> >>> Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it >>> cannot be found, then I would give up and set the jump function to varying. >> >> But DECL_ARGUMENTS is not available at WPA stage with LTO so your >> patch would make our IPA layer to optimize less with LTO. This was >> the reason to go through the hoops of TYPE_ARG_TYPES in the first >> place. >> >> If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to >> square one and indeed need to put the correct type in jump functions. > > If DECL_ARGUMENTS is not available at WPA stage then I see no other > way than to put the types on the jump functions. > OK. I will record the type in jump function and send a revised patch. Thanks, Kugan >> If just preferring DECL_ARGUMENTS is enough, then changing >> ipa_get_callee_param_type to use that if is is available, as Richi >> suggested, would indeed be preferable. But if even falling back on it >> can cause errors, then I am not sure if it helps. >> >> In any event, thanks for diligently dealing with the fallout, >> >> Martin >> >> >>> >>> Bootstrapped and regression tested on x86_64-linux-gnu with no new >>> regressions. Is this OK for trunk? >>> >>> Thanks, >>> Kugan >>> >>> gcc/testsuite/ChangeLog: >>> >>> 2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> >>> >>> PR IPA/78365 >>> * gcc.dg/torture/pr78365.c: New test. >>> >>> gcc/ChangeLog: >>> >>> 2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> >>> >>> PR IPA/78365 >>> * ipa-cp.c (propagate_constants_accross_call): get param type from callees >>> DECL_ARGUMENTS if available. >>> * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise. >>> (ipcp_update_vr): Remove now redundant conversion of precision for VR. >>> * ipa-prop.h: Make ipa_get_callee_param_type local again.
> >If DECL_ARGUMENTS is not available at WPA stage then I see no other > >way than to put the types on the jump functions. > > > OK. I will record the type in jump function and send a revised patch. It would be good to check how much of difference this makes to memory use of WPA for larger program (i.e. Firefox). I guess it doesn't need to be too bad given that most of those types are streamed for other reasons anyway. The type lookup always seemed like a kludge, so indeed saving types to jump functions makes sense to me. Thanks, Honza > > Thanks, > Kugan > > >>If just preferring DECL_ARGUMENTS is enough, then changing > >>ipa_get_callee_param_type to use that if is is available, as Richi > >>suggested, would indeed be preferable. But if even falling back on it > >>can cause errors, then I am not sure if it helps. > >> > >>In any event, thanks for diligently dealing with the fallout, > >> > >>Martin > >> > >> > >>> > >>>Bootstrapped and regression tested on x86_64-linux-gnu with no new > >>>regressions. Is this OK for trunk? > >>> > >>>Thanks, > >>>Kugan > >>> > >>>gcc/testsuite/ChangeLog: > >>> > >>>2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> > >>> > >>> PR IPA/78365 > >>> * gcc.dg/torture/pr78365.c: New test. > >>> > >>>gcc/ChangeLog: > >>> > >>>2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> > >>> > >>> PR IPA/78365 > >>> * ipa-cp.c (propagate_constants_accross_call): get param type from callees > >>> DECL_ARGUMENTS if available. > >>> * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise. > >>> (ipcp_update_vr): Remove now redundant conversion of precision for VR. > >>> * ipa-prop.h: Make ipa_get_callee_param_type local again.
On 24 November 2016 at 15:23, Jan Hubicka <hubicka@ucw.cz> wrote: >> >If DECL_ARGUMENTS is not available at WPA stage then I see no other >> >way than to put the types on the jump functions. >> > >> OK. I will record the type in jump function and send a revised patch. > > It would be good to check how much of difference this makes to memory use > of WPA for larger program (i.e. Firefox). I guess it doesn't need to be too > bad given that most of those types are streamed for other reasons anyway. > The type lookup always seemed like a kludge, so indeed saving types to > jump functions makes sense to me. Similarly should the types also be saved in jump-function for ipa-bits propagation ? Thanks, Prathamesh > > Thanks, > Honza >> >> Thanks, >> Kugan >> >> >>If just preferring DECL_ARGUMENTS is enough, then changing >> >>ipa_get_callee_param_type to use that if is is available, as Richi >> >>suggested, would indeed be preferable. But if even falling back on it >> >>can cause errors, then I am not sure if it helps. >> >> >> >>In any event, thanks for diligently dealing with the fallout, >> >> >> >>Martin >> >> >> >> >> >>> >> >>>Bootstrapped and regression tested on x86_64-linux-gnu with no new >> >>>regressions. Is this OK for trunk? >> >>> >> >>>Thanks, >> >>>Kugan >> >>> >> >>>gcc/testsuite/ChangeLog: >> >>> >> >>>2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> >> >>> >> >>> PR IPA/78365 >> >>> * gcc.dg/torture/pr78365.c: New test. >> >>> >> >>>gcc/ChangeLog: >> >>> >> >>>2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> >> >>> >> >>> PR IPA/78365 >> >>> * ipa-cp.c (propagate_constants_accross_call): get param type from callees >> >>> DECL_ARGUMENTS if available. >> >>> * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise. >> >>> (ipcp_update_vr): Remove now redundant conversion of precision for VR. >> >>> * ipa-prop.h: Make ipa_get_callee_param_type local again.
On Thu, Nov 24, 2016 at 11:15 AM, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote: > On 24 November 2016 at 15:23, Jan Hubicka <hubicka@ucw.cz> wrote: >>> >If DECL_ARGUMENTS is not available at WPA stage then I see no other >>> >way than to put the types on the jump functions. >>> > >>> OK. I will record the type in jump function and send a revised patch. >> >> It would be good to check how much of difference this makes to memory use >> of WPA for larger program (i.e. Firefox). I guess it doesn't need to be too >> bad given that most of those types are streamed for other reasons anyway. >> The type lookup always seemed like a kludge, so indeed saving types to >> jump functions makes sense to me. > Similarly should the types also be saved in jump-function for ipa-bits > propagation ? I think for all propagation (also simple constant propagation). Richard. > Thanks, > Prathamesh >> >> Thanks, >> Honza >>> >>> Thanks, >>> Kugan >>> >>> >>If just preferring DECL_ARGUMENTS is enough, then changing >>> >>ipa_get_callee_param_type to use that if is is available, as Richi >>> >>suggested, would indeed be preferable. But if even falling back on it >>> >>can cause errors, then I am not sure if it helps. >>> >> >>> >>In any event, thanks for diligently dealing with the fallout, >>> >> >>> >>Martin >>> >> >>> >> >>> >>> >>> >>>Bootstrapped and regression tested on x86_64-linux-gnu with no new >>> >>>regressions. Is this OK for trunk? >>> >>> >>> >>>Thanks, >>> >>>Kugan >>> >>> >>> >>>gcc/testsuite/ChangeLog: >>> >>> >>> >>>2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> >>> >>> >>> >>> PR IPA/78365 >>> >>> * gcc.dg/torture/pr78365.c: New test. >>> >>> >>> >>>gcc/ChangeLog: >>> >>> >>> >>>2016-11-18 Kugan Vivekanandarajah <kuganv@linaro.org> >>> >>> >>> >>> PR IPA/78365 >>> >>> * ipa-cp.c (propagate_constants_accross_call): get param type from callees >>> >>> DECL_ARGUMENTS if available. >>> >>> * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise. >>> >>> (ipcp_update_vr): Remove now redundant conversion of precision for VR. >>> >>> * ipa-prop.h: Make ipa_get_callee_param_type local again.
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index 2ec671f..924c846 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -2200,6 +2200,9 @@ propagate_constants_accross_call (struct cgraph_edge *cs) struct ipa_edge_args *args; bool ret = false; int i, args_count, parms_count; + struct cgraph_node *calee = cs->callee; + tree fndecl = calee ? calee->decl : NULL_TREE; + tree parm = fndecl ? DECL_ARGUMENTS (fndecl) : NULL_TREE; callee = cs->callee->function_symbol (&availability); if (!callee->definition) @@ -2247,7 +2250,6 @@ 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) @@ -2265,10 +2267,12 @@ propagate_constants_accross_call (struct cgraph_edge *cs) if (opt_for_fn (callee->decl, flag_ipa_vrp)) ret |= propagate_vr_accross_jump_function (cs, jump_func, dest_plats, - param_type); + parm ? + TREE_TYPE (parm) : NULL_TREE); else ret |= dest_plats->m_value_range.set_to_bottom (); } + parm = parm ? DECL_CHAIN (parm) : NULL_TREE; } for (; i < parms_count; i++) ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info, i)); diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index 6321fdd..0f102c6c 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -1651,7 +1651,7 @@ determine_locally_known_aggregate_parts (gcall *call, tree arg, /* Return the Ith param type of callee associated with call graph edge E. */ -tree +static tree ipa_get_callee_param_type (struct cgraph_edge *e, int i) { int n; @@ -1695,6 +1695,9 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi, gcall *call = cs->call_stmt; int n, arg_num = gimple_call_num_args (call); bool useful_context = false; + struct cgraph_node *calee = cs->callee; + tree fndecl = calee ? calee->decl : NULL_TREE; + tree parm = fndecl ? DECL_ARGUMENTS (fndecl) : NULL_TREE; if (arg_num == 0 || args->jump_functions) return; @@ -1751,8 +1754,8 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi, { wide_int min, max; value_range_type type; - if (TREE_CODE (arg) == SSA_NAME - && param_type + if (parm + && TREE_CODE (arg) == SSA_NAME && (type = get_range_info (arg, &min, &max)) && (type == VR_RANGE || type == VR_ANTI_RANGE)) { @@ -1764,7 +1767,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi, vr.equiv = NULL; extract_range_from_unary_expr (&jfunc->m_vr, NOP_EXPR, - param_type, + TREE_TYPE (parm), &vr, TREE_TYPE (arg)); if (jfunc->m_vr.type == VR_RANGE || jfunc->m_vr.type == VR_ANTI_RANGE) @@ -1775,6 +1778,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi, else gcc_assert (!jfunc->vr_known); } + parm = parm ? DECL_CHAIN (parm) : NULL_TREE; if (INTEGRAL_TYPE_P (TREE_TYPE (arg)) && (TREE_CODE (arg) == SSA_NAME || TREE_CODE (arg) == INTEGER_CST)) @@ -5699,8 +5703,6 @@ ipcp_update_vr (struct cgraph_node *node) if (vr[i].known && (vr[i].type == VR_RANGE || vr[i].type == VR_ANTI_RANGE)) { - tree type = TREE_TYPE (ddef); - unsigned prec = TYPE_PRECISION (type); if (INTEGRAL_TYPE_P (TREE_TYPE (ddef))) { if (dump_file) @@ -5713,11 +5715,7 @@ ipcp_update_vr (struct cgraph_node *node) print_decs (vr[i].max, dump_file); fprintf (dump_file, "]\n"); } - set_range_info (ddef, vr[i].type, - wide_int_storage::from (vr[i].min, prec, - TYPE_SIGN (type)), - wide_int_storage::from (vr[i].max, prec, - TYPE_SIGN (type))); + set_range_info (ddef, vr[i].type, vr[i].min, vr[i].max); } else if (POINTER_TYPE_P (TREE_TYPE (ddef)) && vr[i].type == VR_ANTI_RANGE diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index 0e75cf4..4eeae88 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -818,7 +818,6 @@ 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/torture/pr78365.c b/gcc/testsuite/gcc.dg/torture/pr78365.c index e69de29..5180a01 100644 --- a/gcc/testsuite/gcc.dg/torture/pr78365.c +++ b/gcc/testsuite/gcc.dg/torture/pr78365.c @@ -0,0 +1,21 @@ +/* { dg-do compile } */ + +int a, b, c; +char d; +static void fn1 (void *, int); +int fn2 (int); + +void fn1 (cc, yh) void *cc; +char yh; +{ + char y; + a = fn2(c - b + 1); + for (; y <= yh; y++) + ; +} + +void fn3() +{ + fn1((void *)fn3, 1); + fn1((void *)fn3, d - 1); +}