On Wed, Feb 19, 2020 at 3:20 PM Richard Biener <richard.guent...@gmail.com> wrote: > > On Wed, Feb 19, 2020 at 1:19 PM Richard Sandiford > <richard.sandif...@arm.com> wrote: > > > > What should we do about this? The PR is a wrong-code regression from > > GCC 9 and it doesn't look like we can remove the second offset_overlap_p > > check, given how many cases currently rely on it. > > Did you check whether we eventually disambiguate those via > rtx_refs_may_alias_p > (so using MEM_EXPR)? I would guess all but those that have no MEM_EXPR? > > Ideally we'd rely more on MEM_EXPR for alias disambiguation than the ad-hoc > ways of recovering "details" from the actual MEM. > > If numbers are still in the same ballpark when factoring in alias > disambiguations > run after memrefs_conflict_p calls then let's go with your patch, it > looks technically > correct if all the facts about section anchors are correct (where I > know not very much > about them or representational issues wrt aliasing).
See the patch attached to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49330 for how I did this kind of statistics for base_alias_check. But as said, counting !MEM_EXPR cases might be a good enough hint. Richard. > Thanks, > Richard. > > > Thanks, > > Richard > > > > Richard Sandiford <richard.sandif...@arm.com> writes: > > > Richard Biener <richard.guent...@gmail.com> writes: > > >> On Tue, Feb 4, 2020 at 6:44 PM Richard Sandiford > > >> <richard.sandif...@arm.com> wrote: > > >>> > > >>> Richard Sandiford <richard.sandif...@arm.com> writes: > > >>> > [...] > > >>> >> I'm not sure given the issues you've introduced if I could actually > > >>> >> fill out the matrix of answers without more underlying information. > > >>> >> ie, when can we get symbols without source level decls, > > >>> >> anchors+interposition issues, etc. > > >>> > > > >>> > OK. In that case, I wonder whether it would be safer to have a > > >>> > fourth state on top of the three above: > > >>> > > > >>> > - known distance apart > > >>> > - independent > > >>> > - known distance apart or independent > > >>> > - don't know > > >>> > > > >>> > with "don't know" being anything that involves bare symbols? > > >>> > > > >>> > Richard > > >>> > > >>> How does this look? Tested on aarch64-linux-gnu and > > >>> x86_64-linux-gnu. > > >>> > > >>> Full description from scratch: > > >>> > > >>> memrefs_conflict_p has a slightly odd structure. It first checks > > >>> whether two addresses based on SYMBOL_REFs refer to the same object, > > >>> with a tristate result: > > >>> > > >>> int cmp = compare_base_symbol_refs (x,y); > > >>> > > >>> If the addresses do refer to the same object, we can use offset-based > > >>> checks: > > >>> > > >>> /* If both decls are the same, decide by offsets. */ > > >>> if (cmp == 1) > > >>> return offset_overlap_p (c, xsize, ysize); > > >>> > > >>> But then, apart from the special case of forced address alignment, > > >>> we use an offset-based check even if we don't know whether the > > >>> addresses refer to the same object: > > >>> > > >>> /* Assume a potential overlap for symbolic addresses that went > > >>> through alignment adjustments (i.e., that have negative > > >>> sizes), because we can't know how far they are from each > > >>> other. */ > > >>> if (maybe_lt (xsize, 0) || maybe_lt (ysize, 0)) > > >>> return -1; > > >>> /* If decls are different or we know by offsets that there is no > > >>> overlap, > > >>> we win. */ > > >>> if (!cmp || !offset_overlap_p (c, xsize, ysize)) > > >>> return 0; > > >>> > > >>> This somewhat contradicts: > > >>> > > >>> /* In general we assume that memory locations pointed to by different > > >>> labels > > >>> may overlap in undefined ways. */ > > >> > > >> Sorry for not chiming in earlier but isn't the bug that > > >> > > >> > > >> > > >>> at the end of compare_base_symbol_refs. In other words, we're taking -1 > > >>> to mean that either (a) the symbols are equal (via aliasing) or (b) the > > >>> references access non-overlapping objects. > > >>> > > >>> But even assuming that's true for normal symbols, it doesn't cope > > >>> correctly with section anchors. If a symbol X at ANCHOR+OFFSET > > >>> is preemptible, either (a) X = ANDHOR+OFFSET or (b) X and ANCHOR > > >>> reference non-overlapping objects. > > >>> > > >>> And an offset-based comparison makes no sense for an anchor symbol > > >>> vs. a bare symbol with no decl. If the bare symbol is allowed to > > >>> alias other symbols then it can surely alias any symbol in the > > >>> anchor's block, so there are multiple anchor offsets that might > > >>> induce an alias. > > >> > > >> But then isn't the simple fix to honor the -1 and do > > >> > > >> diff --git a/gcc/alias.c b/gcc/alias.c > > >> index 3794f9b6a9e..bf13d37c0f7 100644 > > >> --- a/gcc/alias.c > > >> +++ b/gcc/alias.c > > >> @@ -2490,9 +2490,8 @@ memrefs_conflict_p (poly_int64 xsize, rtx x, > > >> poly_int64 ysize, rtx y, > > >> other. */ > > >> if (maybe_lt (xsize, 0) || maybe_lt (ysize, 0)) > > >> return -1; > > >> - /* If decls are different or we know by offsets that there is no > > >> overlap, > > >> - we win. */ > > >> - if (!cmp || !offset_overlap_p (c, xsize, ysize)) > > >> + /* If decls are different, we win. */ > > >> + if (cmp == 0) > > >> return 0; > > >> /* Decls may or may not be different and offsets overlap....*/ > > >> return -1; > > >> > > >> ? > > > > > > The code was deliberately written this way from the ouset though > > > (rather than it ending up like this through many cuts). It was > > > added in g:54363f8a92920f5559c83ddd53e480a27205e6b7: > > > > > > 2015-12-08 Jan Hubicka <hubi...@ucw.cz> > > > > > > PR ipa/61886 > > > PR middle-end/25140 > > > * tree-ssa-alias.c (ptr_deref_may_alias_decl_p): Use > > > compare_base_decls > > > (nonoverlapping_component_refs_of_decl_p): Update sanity check. > > > (decl_refs_may_alias_p): Use compare_base_decls. > > > * alias.c: Include cgraph.h > > > (get_alias_set): Add cut-off for recursion. > > > (rtx_equal_for_memref_p): Use rtx_equal_for_memref_p. > > > (compare_base_decls): New function. > > > (base_alias_check): Likewise. > > > (memrefs_conflict_p): Likewise. > > > (nonoverlapping_memrefs_p): Likewise. > > > * alias.h (compare_base_decls): Declare. > > > > > > which included: > > > > > > - if (rtx_equal_for_memref_p (x, y)) > > > + if (GET_CODE (x) == SYMBOL_REF && GET_CODE (y) == SYMBOL_REF) > > > + { > > > + tree x_decl = SYMBOL_REF_DECL (x); > > > + tree y_decl = SYMBOL_REF_DECL (y); > > > + int cmp; > > > + > > > + if (!x_decl || !y_decl) > > > + { > > > + /* Label and normal symbol are never the same. */ > > > + if (x_decl != y_decl) > > > + return 0; > > > + return offset_overlap_p (c, xsize, ysize); > > > + } > > > + else > > > + cmp = compare_base_decls (x_decl, y_decl); > > > + > > > + /* If both decls are the same, decide by offsets. */ > > > + if (cmp == 1) > > > + return offset_overlap_p (c, xsize, ysize); > > > + /* If decls are different or we know by offsets that there is no > > > overlap, > > > + we win. */ > > > + if (!cmp || !offset_overlap_p (c, xsize, ysize)) > > > + return 0; > > > + /* Decls may or may not be different and offsets overlap....*/ > > > + return -1; > > > + } > > > + else if (rtx_equal_for_memref_p (x, y)) > > > > > > The only significant change since them was to add compare_base_symbol_refs > > > (g:73e48cb322152bf504ced8694fa748544ecaa6eb): > > > > > > if (GET_CODE (x) == SYMBOL_REF && GET_CODE (y) == SYMBOL_REF) > > > { > > > - tree x_decl = SYMBOL_REF_DECL (x); > > > - tree y_decl = SYMBOL_REF_DECL (y); > > > - int cmp; > > > - > > > - if (!x_decl || !y_decl) > > > - { > > > - /* Label and normal symbol are never the same. */ > > > - if (x_decl != y_decl) > > > - return 0; > > > - return offset_overlap_p (c, xsize, ysize); > > > - } > > > - else > > > - cmp = compare_base_decls (x_decl, y_decl); > > > + int cmp = compare_base_symbol_refs (x,y); > > > > > >>> This patch therefore replaces the current tristate: > > >>> > > >>> - known equal > > >>> - known independent (two accesses can't alias) > > >>> - equal or independent > > >>> > > >>> with: > > >>> > > >>> - known distance apart > > >>> - known independent (two accesses can't alias) > > >>> - known distance apart or independent > > >>> - don't know > > >>> > > >>> For safety, the patch puts all bare symbols in the "don't know" > > >>> category. If that turns out to be too conservative, we at least > > >>> need that behaviour for combinations involving a bare symbol > > >>> and a section anchor. > > >> > > >> This sounds complicated (for this stage). Do you have any statistics as > > >> to how it affects actual alias queries (thus outcome in > > >> {true,...}_dependence) > > >> when you do the "simple" fix? > > > > > > For a stage3 build of gcc on aarch64-linux-gnu I get: > > > > > > ("positive" == conflict, "negative" == no conflict) > > > > > > 16191 cmp == 1, disjoint offsets : true negative > > > 19698 cmp == 1, overlapping offsets : true positive > > > 79363 cmp == -1, disjoint offsets : true negative > > > 6965 cmp == -1, disjoint offsets : false negative <== bug > > > 48 cmp == -1, overlapping offsets : true positive > > > 928 cmp == -1, overlapping offsets : false positive <== missed opt > > > 123193 total queries > > > > > > where the cmp == -1 cases are divided into true and false results > > > according to whether we get the same answer when taking the (hopefully) > > > correct offset into account. cmp == 0 and what would be cmp == -2 never > > > occured. > > > > > > So it looks like we're relying on the cmp == -1 offset_overlap_p > > > check to get true "no conflict" results for ~64% of all queries > > > (or ~83% of all true "no conflict" results). I expect this is > > > very dependent on the fact that the port uses section anchors. > > > > > > The number of buggy cases seems surprisingly high, but I've tried > > > to re-check it a couple of times and it looks to be accurate. > > > They come from cases where we compare things like: > > > > > > x: (symbol_ref:DI ("*.LANCHOR0") [flags 0x182]) > > > y: (symbol_ref:DI ("_ZL23yy_last_accepting_state") [flags 0x82] <var_decl > > > ... yy_last_accepting_state>) > > > c: -48 > > > > > > where yy_last_accepting_state is at offset 48 from the anchor. > > > I haven't checked where the buggy queries are coming from though; > > > might be debug-related and so hard to spot. > > > > > > Thanks, > > > Richard
