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 ;) 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 ... 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. 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 >
