Hi
On Mon, Nov 04 2019, Richard Biener wrote:
> On Fri, 1 Nov 2019, Martin Jambor wrote:
>
>> Hi,
>>
>> I have spent some more time looking into PR 70929, how
>> gimple_check_call_matching_types behaves when LTO-building Firefox to
>> see what could replace it or if we perhaps could remove it.
>>
>> TL;DR:
>> I believe it can and should be removed, possibly except the use in
>> value-prof.c where I replaced it with a clearly imprecise predicate to
>> catch cases where the profile info is corrupted and since I had it, I
>> also ended up using it for speculative devirtualization, in case it got
>> its guess somehow wrong (but I have not seen either of the two cases
>> happening). See the patch below.
>>
>>
>> More details:
>> With LTO the predicate can always be fooled and so we cannot rely on it
>> as means to prevent ICEs, if the user calls incompatible functions, the
>> compiler should try to fix it with folding, VCEing or just using zero
>> constructors in the most bogus of cases.
>>
>> And trying to make the predicate more clever can be difficult. When
>> LTO-building Firefox (without PGO), gimple_check_call_matching_types
>> returns false 8133 times and those cases can be divided into:
>>
>> - 2507x the callee was __builtin_constant_p
>> - 17x the callee was __builtin_signbit
>> - 1388x the callee was __builtin_unreachable
>>
>> - 4215x would pass the suggested test in comment 5 of the bug. I
>> examined quite a few and all were exactly the problem discussed in
>> this PR - they were all deemed incompatible because one parameter
>> was a reference to a TREE_ADDRESSABLE class.
>>
>> - 6x both predicates returned false for a target found by speculative
>> devirtualization. I tend to think they were both wrong because...
>>
>> ...the predicate from comment #5 of the bug is not a good substitute
>> because it returns false for perfectly fine virtual calls when the type
>> of the call is a method belonging to an ancestor of the class to which
>> the actual callee belongs. Thousands of calls to AddRef did not pass
>> the test.
>>
>> Without finding any real case for having the predicate, I decided to
>> remove its use from everywhere except for check_ic_target because its
>> comment says:
>>
>> /* 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. Here we only do very minimal sanity check just to make compiler
>> happy.
>>Returns true if TARGET is considered ok for call CALL_STMT. */
>>
>> and if some such race conditions really happen and can be detected if
>> e.g. the number of parameters is clearly off, the compiler should
>> probably discard the target. But I did not want to keep the original
>> clumsy predicate and therefore decided to extract the non-problematic
>> bits of useless_type_conversion_p into:
>>
>> /* Check the type of FNDECL and return true if it is likely compatible with
>> the
>>callee type in CALL. The check is imprecise, the intended use of this
>>function is that when optimizations like value profiling and speculative
>>devirtualization somehow guess a clearly wrong target of an indirect call,
>>they can discard it. */
>>
>> bool
>> gimple_call_types_likely_match_p (gcall *call, tree fndecl)
>> {
>> tree call_type = gimple_call_fntype (call);
>> tree decl_type = TREE_TYPE (fndecl);
>>
>> /* If one is a function and the other a method, that's a mismatch. */
>> if (TREE_CODE (call_type) != TREE_CODE (decl_type))
>> return false;
>> /* If the return types are not compatible, bail out. */
>> if (!useless_type_conversion_p (TREE_TYPE (call_type),
>>TREE_TYPE (decl_type)))
>> return false;
>> /* If the call was to an unprototyped function, all bets are off. */
>> if (!prototype_p (call_type))
>> return true;
>>
>> /* If the unqualified argument types are compatible, the types match. */
>> if (TYPE_ARG_TYPES (call_type) == TYPE_ARG_TYPES (decl_type))
>> return true;
>>
>> tree call_parm, decl_parm;
>> for (call_parm = TYPE_ARG_TYPES (call_type),
>> decl_parm = TYPE_ARG_TYPES (decl_type);
>>call_parm && decl_parm;
>>call_parm = TREE_CHAIN (call_parm),
>> decl_parm = TREE_CHAIN (decl_parm))
>> if (!useless_type_conversion_p
>> (TYPE_MAIN_VARIANT (TREE_VALUE (call_parm)),
>> TYPE_MAIN_VARIANT (TREE_VALUE (decl_parm
>> return false;
>>
>> /* If there is a mismatch in the number of arguments the functions
>> are not compatible. */
>> if (call_parm || decl_parm)
>> return false;
>>
>> return true;
>> }
>>
>> Crucially, the function is missing the part that does:
>>
>> /* Method types should belong to a compatible base class. */
>> if (TREE_CODE (inner_type) ==
