On Thu, Jun 13, 2013 at 1:43 AM, Richard Biener <richard.guent...@gmail.com> wrote: > On Sat, Jun 8, 2013 at 2:26 AM, Xinliang David Li <davi...@google.com> wrote: >> 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. > > But that's not due to "race conditions" but rather AutoFDO which isn't even > in trunk, right?
not yet. Dehao is working on it. David > > Anyway, the discussion is probably moot - the patch is ok with me > and my argument would be we should use the function in less places. > > Thanks, > Richard. > >> 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
