On Fri, Jun 7, 2013 at 2:05 AM, Richard Biener <richard.guent...@gmail.com> wrote: > On Thu, Jun 6, 2013 at 5:10 PM, 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? > > I wonder what's the point in the gimple_check_call_matching_types check > in the profiling case. It's at least no longer > > /* Perform sanity check on the indirect call target. Due to race conditions, > false function target may be attributed to an indirect call site. If the > call expression type mismatches with the target function's type, > expand_call > may ICE. > > because since the introduction of gimple_call_fntype we will _not_ ICE. > > Thus I argue that check_ic_target should be even removed instead of > enhancing it! >
Another reason is what Dehao had mentioned -- wrong target leads to useless transformation. > How does IC profiling determine the called target? That is, what does it > do when the target is not always the same? (because the checking code > talks about race conditions for example) The race condition is the happening at instrumentation time -- the indirect call counters are not thread local. We have seen this a lot in the past that a totally bogus target is attributed to a indirect callsite. thanks, David > > Richard. > > >> 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