On Mon, Dec 6, 2021 at 4:03 PM Richard Biener <richard.guent...@gmail.com> wrote: > > 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.
So the issue seems that for # .MEM_22 = VDEF <.MEM_19> memcpy (&dst, &src, 1024); determine_global_memory_access determines the stmt doesn't write to global memory (well, yes!), but it fails to add the actual arguments to the set of clobbered vars. Honza, can you please look into this? Seems to be caused by g:008e7397dad971c03c08fc1b0a4a98fddccaaed8 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 > >
