[GOOGLE] More strict checking for call args

[Xinliang David Li]

On Thu, Jun 13, 2013 at 1:43 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Sat, Jun 8, 2013 at 2:26 AM, Xinliang David Li <davidxl@google.com> wrote:
>> On Fri, Jun 7, 2013 at 6:47 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Fri, Jun 7, 2013 at 3:30 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Fri, Jun 7, 2013 at 2:05 AM, Richard Biener
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Thu, Jun 6, 2013 at 5:10 PM, Dehao Chen <dehao@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 <mjambor@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  <mjambor@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