On 28 November 2016 at 10:55, kugan <kugan.vivekanandara...@linaro.org> wrote: > Hi, > > On 24/11/16 19:48, Richard Biener wrote: >> >> On Wed, Nov 23, 2016 at 4:33 PM, Martin Jambor <mjam...@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. > > > Here is a patch that does this. To fox PR78365, in > ipa_get_callee_param_type, I am now checking DECL_ARGUMENTS first. I lto > bootstrapped and regression tested on x86_64-linux-gnu and ppc64le-linux > with no new regressions. I will build Firefox and measure the memory usage > as Honza suggested based on the feedback. Hi Kugan, In your patch in ipa_get_callee_param_type(): + tree t = e->callee ? DECL_ARGUMENTS (e->callee->decl) : NULL_TREE;
Perhaps this should be e->callee->function_symbol() ? Thanks, Prathamesh > > Thanks, > Kugan > > > > gcc/ChangeLog: > > 2016-11-28 Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org> > > PR IPA/78365 > * ipa-cp.c (propagate_vr_accross_jump_function): Remove param_type > argument and > use the one set in jump_func. > (propagate_constants_accross_call): Likewise. > * ipa-prop.c (ipa_get_callee_param_type): Chedk DECL_ARGUMENTS > first. > (ipa_compute_jump_functions_for_edge): Set param_type for jump_func. > (ipa_write_jump_function): Stream param_type. > (ipa_read_jump_function): Likewise. > > gcc/testsuite/ChangeLog: > > 2016-11-28 Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org> > > > PR IPA/78365 > * gcc.dg/torture/pr78365.c: New test. > > > >>> 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 <kug...@linaro.org> >>>> >>>> PR IPA/78365 >>>> * gcc.dg/torture/pr78365.c: New test. >>>> >>>> gcc/ChangeLog: >>>> >>>> 2016-11-18 Kugan Vivekanandarajah <kug...@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.
