This one should be submitted and discussed in trunk. thanks,
David On Thu, Jun 6, 2013 at 9:39 PM, Dehao Chen <de...@google.com> wrote: > I've prepared a patch check for args for indirect call value profiling. > > Testing on-going. Is it ok for trunk if testing is good? > > Thanks, > Dehao > > gcc/ChangeLog: > 2013-06-06 Dehao Chen <de...@google.com> > > * tree-flow.h (gimple_check_call_matching_types): Add new argument. > * gimple-low.c (gimple_check_call_matching_types): Likewise. > (gimple_check_call_args): Likewise. > * value-prof.c (check_ic_target): Likewise. > * ipa-inline.c (early_inliner): Likewise. > * ipa-prop.c (update_indirect_edges_after_inlining): Likewise. > * cgraph.c (cgraph_create_edge_1): Likewise. > (cgraph_make_edge_direct): Likewise. > > > > On Thu, Jun 6, 2013 at 9:14 AM, Xinliang David Li <davi...@google.com> wrote: >> gimple_check_call_matching_types is called by check_ic_target. Instead >> of moving the check out of this guard function, may be enhancing the >> interface to allow it to guard with different strictness? >> >> David >> >> On Thu, Jun 6, 2013 at 8:10 AM, Dehao Chen <de...@google.com> wrote: >>> Hi, Martin, >>> >>> Yes, your patch can fix my case. Thanks a lot for the fix. >>> >>> With the fix, value profiling will still promote the wrong indirect >>> call target. Though it will not be inlining, but it results in an >>> additional check. How about in check_ic_target, after calling >>> gimple_check_call_matching_types, we also check if number of args >>> match number of params in target->symbol.decl? >>> >>> Thanks, >>> Dehao >>> >>> >>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjam...@suse.cz> wrote: >>>> >>>> Hi, >>>> >>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote: >>>> > attached is a testcase that would cause problem when source has changed: >>>> > >>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD >>>> > $ ./a.out >>>> > $ g++ test.cc -O2 -fprofile-use >>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815 >>>> > } >>>> > ^ >>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int) >>>> > ../../gcc/vec.h:815 >>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int) >>>> > ../../gcc/vec.h:1244 >>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int) >>>> > ../../gcc/vec.h:815 >>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int) >>>> > ../../gcc/vec.h:1244 >>>> > 0xf24464 ipa_get_indirect_edge_target_1 >>>> > ../../gcc/ipa-cp.c:1535 >>>> > 0x971b9a estimate_edge_devirt_benefit >>>> > ../../gcc/ipa-inline-analysis.c:2757 >>>> >>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1. >>>> Since it is called also from inlining, we can have parameter count >>>> mismatches... and in fact in non-virtual paths of that function we do >>>> check that we don't. Because all callers have to pass known_vals >>>> describing all formal parameters of the inline tree root, we should >>>> apply the fix below (I've only just started running a bootstrap and >>>> testsuite on x86_64, though). >>>> >>>> OTOH, while I understand that FDO can change inlining sufficiently so >>>> that this error occurs, IMHO this should not be caused by outdated >>>> profiles but there is somewhere a parameter mismatch in the source. >>>> >>>> Dehao, can you please check that this patch helps? >>>> >>>> Richi, if it does and the patch passes bootstrap and tests, is it OK >>>> for trunk and 4.8 branch? >>>> >>>> Thanks and sorry for the trouble, >>>> >>>> Martin >>>> >>>> >>>> 2013-06-06 Martin Jambor <mjam...@suse.cz> >>>> >>>> * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that >>>> param_index is >>>> within bounds at the beginning of the function. >>>> >>>> Index: src/gcc/ipa-cp.c >>>> =================================================================== >>>> --- src.orig/gcc/ipa-cp.c >>>> +++ src/gcc/ipa-cp.c >>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c >>>> tree otr_type; >>>> tree t; >>>> >>>> - if (param_index == -1) >>>> + if (param_index == -1 >>>> + || known_vals.length () <= (unsigned int) param_index) >>>> return NULL_TREE; >>>> >>>> if (!ie->indirect_info->polymorphic) >>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c >>>> t = NULL; >>>> } >>>> else >>>> - t = (known_vals.length () > (unsigned int) param_index >>>> - ? known_vals[param_index] : NULL); >>>> + t = NULL; >>>> >>>> if (t && >>>> TREE_CODE (t) == ADDR_EXPR