Richard Biener <richard.guent...@gmail.com> writes:
> On Mon, Dec 6, 2021 at 11:10 AM Richard Sandiford
> <richard.sandif...@arm.com> wrote:
>>
>> Richard Biener <richard.guent...@gmail.com> writes:
>> > On Sun, Dec 5, 2021 at 10:59 PM Richard Sandiford via Gcc-patches
>> > <gcc-patches@gcc.gnu.org> wrote:
>> >>
>> >> When compiling an optabs.ii at -O2 with a release-checking build,
>> >> we spent a lot of time in call_may_clobber_ref_p. One of the
>> >> first things the function tries is the get_modref_function_summary
>> >> approach, but that also seems to be the most expensive check.
>> >> At least for the optabs.ii test, most of the things g_m_f_s could
>> >> handle could also be handled by points-to analysis, which is much
>> >> cheaper.
>> >>
>> >> This patch therefore rearranges the tests in approximate order
>> >> of cheapness. One wart is that points-to analysis can't be
>> >> used on its own for this purpose; it has to be used in
>> >> conjunction with check_fnspec. This is because the argument
>> >> information in the fnspec is not reflected in the points-to
>> >> information, which instead contains overly optimistic (rather
>> >> than conservatively pessimistic) information.
>> >>
>> >> Specifically, find_func_aliases_for_builtin_call short-circuits
>> >> the handle_rhs_call/handle_call_arg logic and doesn't itself add
>> >> any compensating information about the arguments. This means that:
>> >>
>> >> free (foo)
>> >>
>> >> does not have an points-to information for foo (it has an empty
>> >> set instead).
>> >
>> > Huh? The gimple_call_use/clobber_sets should be conservative
>> > and stand on their own. Can you share a testcase that breaks when
>> > returning early if independent_pt? Does this problem also exist
>> > on the branch?
>>
>> With the attached patch to disable get_modref_function_summary,
>> do the PT query ahead of the fnspec query, and assert if the fnspec
>> query would have found an alias, I get the following results for
>> execute.exp on x86_64-linux-gnu (full log attached):
>>
>> # of expected passes 44554
>> # of unexpected failures 1710
>> # of unresolved testcases 855
>> # of unsupported tests 62
>>
>> E.g. for execute/20000703-1.c, the assertion trips for:
>>
>> 3786 if (stmt_may_clobber_ref_p_1 (def_stmt, ref, tbaa_p))
>> (gdb) call debug (def_stmt)
>> __builtin_memset (p_3(D), 0, 28);
>> (gdb) call debug (ref.base)
>> *p_3(D)
>> (gdb) call pt_solution_includes (&((gcall *) def_stmt)->call_clobbered,
>> ref.base)
>> $3 = false
>
> huh, well - *p_3(D) isn't a DECL and pt_solution_includes just works for
> decls.
> In fact it should eventually ICE even ;)
Oops, yes. That's what comes of trying to be fancy and doing things
in a gdb session rather than printfs. :-) It is of course the:
else if ((TREE_CODE (base) == MEM_REF
|| TREE_CODE (base) == TARGET_MEM_REF)
&& TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
{
struct ptr_info_def *pi = SSA_NAME_PTR_INFO (TREE_OPERAND (base, 0));
if (pi
&& !pt_solutions_intersect (gimple_call_clobber_set (call), &pi->pt))
{
path that fails the assert.
> That said, nothing should
> really iniitalize
> the call_clobber set besides pt_solution_reset at GIMPLE stmt allocation time
> or IPA PTA when enabled. pt_solutuon_reset sets ->anything to true so the
> function should return true ...
Yeah, it works OK in the reset state.
> It looks like Honza added computation of these sets at some point, also maybe
> forgetting to set some bits here.
>
> I prefer to investigate / fix this before refactoring the uses in this way.
> Can
> you file a bugreport please? I can reproduce the memcpy FAIL with
> just moving check_fnspec down.
OK, filed as PR103584 (for the list).
Thanks,
Richard
>
> Thanks,
> Richard.
>
>> Setting ASSERTS to 0 gives:
>>
>> FAIL: gcc.c-torture/execute/memcpy-1.c -O2 execution test
>> FAIL: gcc.c-torture/execute/memcpy-1.c -O2 -flto -fno-use-linker-plugin
>> -flto-partition=none execution test
>> FAIL: gcc.c-torture/execute/memcpy-1.c -O2 -flto -fuse-linker-plugin
>> -fno-fat-lto-objects execution test
>> FAIL: gcc.c-torture/execute/memcpy-1.c -O3 -fomit-frame-pointer
>> -funroll-loops -fpeel-loops -ftracer -finline-functions execution test
>> FAIL: gcc.c-torture/execute/memcpy-1.c -O3 -g execution test
>> FAIL: gcc.c-torture/execute/memcpy-1.c -Os execution test
>> FAIL: gcc.c-torture/execute/memset-3.c -O3 -fomit-frame-pointer
>> -funroll-loops -fpeel-loops -ftracer -finline-functions execution test
>> FAIL: gcc.c-torture/execute/memset-3.c -O3 -g execution test
>>
>> # of expected passes 23138
>> # of unexpected failures 8
>> # of unsupported tests 24
>>
>> (same for -m32).
>>
>> Originally I saw it as a bootstrap failure building stage 2
>> build-side libcpp, due to a bogus uninitialised warning.
>>
>> But alongside the memset and free examples, there's:
>>
>> /* All the following functions do not return pointers, do not
>> modify the points-to sets of memory reachable from their
>> arguments and do not add to the ESCAPED solution. */
>> case BUILT_IN_SINCOS:
>> case BUILT_IN_SINCOSF:
>> ...
>> return true;
>>
>> (always an empty set), whereas check_fnspec takes errno into account.
>>
>> > I wonder if, at this point, we should split the patch up to do the
>> > trivial move of the ao_ref_base dependent and volatile checks
>> > and leave this more complicated detail for a second patch?
>>
>> Yeah, that'd work, but in practice it was the PT part that was the
>> high-value part. IIRC the early base checks caught less than 1% of
>> cases (I can rerun tonight to get exact numbers).
>>
>> Thanks,
>> Richard
>>
