On Fri, Jun 7, 2013 at 6:47 AM, Richard Biener <richard.guent...@gmail.com> wrote: > On Fri, Jun 7, 2013 at 3:30 PM, Xinliang David Li <davi...@google.com> wrote: >> 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. > > Sure, but a not wrong in the sense of the predicate does not guarantee > a useful transformation either.
The case in reality is very rare -- most of the cases, the transformation is good. > >>> 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. > > So it simply uses whatever function was called last? Instead of > using the function that was called most of the time? It uses the most frequent target -- but the target id recorded for the most frequent target might be corrupted and got mapped to a false target during profile-use. David > > Richard. > >> 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