From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id 44EB1393C852 for ; Tue, 19 Jan 2021 16:15:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 44EB1393C852 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=ucw.cz Authentication-Results: sourceware.org; spf=none smtp.mailfrom=hubicka@kam.mff.cuni.cz Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id C26D22808A2; Tue, 19 Jan 2021 17:15:51 +0100 (CET) Date: Tue, 19 Jan 2021 17:15:51 +0100 From: Jan Hubicka To: Richard Biener Cc: Richard Sandiford , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] alias: Fix offset checks involving section anchors [PR92294] Message-ID: <20210119161551.GH66457@kam.mff.cuni.cz> References: <20210117230156.GA47705@kam.mff.cuni.cz> <20210118132911.GA9027@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-2 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-14.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, KAM_STOCKGEN, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Jan 2021 16:15:56 -0000 > On Mon, 18 Jan 2021, Richard Sandiford wrote: > > > Jan Hubicka writes: > > >> >> > > >> >> Well, in tree-ssa code we do assume these to be either disjoint objects > > >> >> or equal (in decl_refs_may_alias_p that continues in case > > >> >> compare_base_decls is -1). I am not sure if we win much by threating > > >> >> them differently on RTL level. I would preffer staying consistent here. > > >> > > >> Yeah, I see your point. My concern here was that the fallback case > > >> applies to SYMBOL_REFs without decls, which might not have been visible > > >> at the tree-ssa level. E.g. they might be ABI-defined symbols that have > > >> no known relation to source-level constructs. > > >> > > >> E.g. the small-data base symbol _gp on MIPS points at a fixed offset > > >> from the start of the small-data area (0x7ff0 IIRC). If the target > > >> generated rtl code that used _gp directly, we could wrongly assume > > >> that _gp+X can't alias BASE+Y when X != Y, even though the real test > > >> for small-data BASEs would be whether X + 0x7ff0 != Y. > > >> > > >> I don't think that could occur in tree-ssa. No valid C code would > > >> be able to refer directly to _gp in this way. > > >> > > >> On the other hand, I don't have a specific example of where this does > > >> go wrong, it's just a feeling that it might. I can drop it if you > > >> think that's better. > > > > > > I would lean towards not disabling optimization when we have no good > > > reason for that - we already did it bit too many times in aliasing code > > > and it is hard to figure out what optimizations are missed purposefully > > > and what are missed just as omission. > > > > > > We already comitted to a very conservative assumption that every > > > external symbol can be alias of another. I think we should have > > > originally required units that reffers to same memory location via > > > different symbols to declare it explicitly (i.e. make external alias to > > > external symbol), but we do not even allow external aliases (symtab > > > supports that though) and also it may depend on use of the module what > > > symbols are aliased. > > > > > > We also decided to disable TBAA for direct accesses to decls to allow > > > type punning using unions. > > > > > > This keeps the offset+range check to be only means of disambiguation. > > > While for modern programs global arrays are not common, for Fortran > > > stuff they are, so I would preffer to not cripple them even more. > > > (I am not sure how often the arrays are external though) > > > > OK, the version below drops the new -2 return value and tries to > > clarify the comments in compare_base_symbol_refs. > > > > Lightly tested on aarch64-linux-gnu so far. Does it look OK if > > full tests pass? > > OK from my side. OK too, thanks! Honza > > Richard. > > > Thanks, > > Richard > > > > ---- > > > > memrefs_conflict_p assumes that: > > > > [XB + XO, XB + XO + XS) > > > > does not alias > > > > [YB + YO, YB + YO + YS) > > > > whenever: > > > > [XO, XO + XS) > > > > does not intersect > > > > [YO, YO + YS) > > > > In other words, the accesses can alias only if XB == YB at runtime. > > > > However, this doesn't cope correctly with section anchors. > > For example, if XB is an anchor symbol and YB is at offset > > XO from the anchor, then: > > > > [XB + XO, XB + XO + XS) > > > > overlaps > > > > [YB, YB + YS) > > > > whatever the value of XO is. In other words, when doing the > > alias check for two symbols whose local definitions are in > > the same block, we should apply the known difference between > > their block offsets to the intersection test above. > > > > gcc/ > > PR rtl-optimization/92294 > > * alias.c (compare_base_symbol_refs): Take an extra parameter > > and add the distance between two symbols to it. Enshrine in > > comments that -1 means "either 0 or 1, but we can't tell > > which at compile time". > > (memrefs_conflict_p): Update call accordingly. > > (rtx_equal_for_memref_p): Likewise. Take the distance between symbols > > into account. > > --- > > gcc/alias.c | 47 +++++++++++++++++++++++++++++++---------------- > > 1 file changed, 31 insertions(+), 16 deletions(-) > > > > diff --git a/gcc/alias.c b/gcc/alias.c > > index 8d3575e4e27..69e1eb89ac6 100644 > > --- a/gcc/alias.c > > +++ b/gcc/alias.c > > @@ -159,7 +159,8 @@ static tree decl_for_component_ref (tree); > > static int write_dependence_p (const_rtx, > > const_rtx, machine_mode, rtx, > > bool, bool, bool); > > -static int compare_base_symbol_refs (const_rtx, const_rtx); > > +static int compare_base_symbol_refs (const_rtx, const_rtx, > > + HOST_WIDE_INT * = NULL); > > > > static void memory_modified_1 (rtx, const_rtx, void *); > > > > @@ -1837,7 +1838,11 @@ rtx_equal_for_memref_p (const_rtx x, const_rtx y) > > return label_ref_label (x) == label_ref_label (y); > > > > case SYMBOL_REF: > > - return compare_base_symbol_refs (x, y) == 1; > > + { > > + HOST_WIDE_INT distance = 0; > > + return (compare_base_symbol_refs (x, y, &distance) == 1 > > + && distance == 0); > > + } > > > > case ENTRY_VALUE: > > /* This is magic, don't go through canonicalization et al. */ > > @@ -2172,10 +2177,20 @@ compare_base_decls (tree base1, tree base2) > > return ret; > > } > > > > -/* Same as compare_base_decls but for SYMBOL_REF. */ > > +/* Compare SYMBOL_REFs X_BASE and Y_BASE. > > + > > + - Return 1 if Y_BASE - X_BASE is constant, adding that constant > > + to *DISTANCE if DISTANCE is nonnull. > > + > > + - Return 0 if no accesses based on X_BASE can alias Y_BASE. > > + > > + - Return -1 if one of the two results applies, but we can't tell > > + which at compile time. Update DISTANCE in the same way as > > + for a return value of 1, for the case in which that holds. */ > > > > static int > > -compare_base_symbol_refs (const_rtx x_base, const_rtx y_base) > > +compare_base_symbol_refs (const_rtx x_base, const_rtx y_base, > > + HOST_WIDE_INT *distance) > > { > > tree x_decl = SYMBOL_REF_DECL (x_base); > > tree y_decl = SYMBOL_REF_DECL (y_base); > > @@ -2192,8 +2207,8 @@ compare_base_symbol_refs (const_rtx x_base, const_rtx y_base) > > std::swap (x_decl, y_decl); > > std::swap (x_base, y_base); > > } > > - /* We handle specially only section anchors and assume that other > > - labels may overlap with user variables in an arbitrary way. */ > > + /* We handle specially only section anchors. Other symbols are > > + either equal (via aliasing) or refer to different objects. */ > > if (!SYMBOL_REF_HAS_BLOCK_INFO_P (y_base)) > > return -1; > > /* Anchors contains static VAR_DECLs and CONST_DECLs. We are safe > > @@ -2222,14 +2237,13 @@ compare_base_symbol_refs (const_rtx x_base, const_rtx y_base) > > { > > if (SYMBOL_REF_BLOCK (x_base) != SYMBOL_REF_BLOCK (y_base)) > > return 0; > > - if (SYMBOL_REF_BLOCK_OFFSET (x_base) == SYMBOL_REF_BLOCK_OFFSET (y_base)) > > - return binds_def ? 1 : -1; > > - if (SYMBOL_REF_ANCHOR_P (x_base) != SYMBOL_REF_ANCHOR_P (y_base)) > > - return -1; > > - return 0; > > + if (distance) > > + *distance += (SYMBOL_REF_BLOCK_OFFSET (y_base) > > + - SYMBOL_REF_BLOCK_OFFSET (x_base)); > > + return binds_def ? 1 : -1; > > } > > - /* In general we assume that memory locations pointed to by different labels > > - may overlap in undefined ways. */ > > + /* Either the symbols are equal (via aliasing) or they refer to > > + different objects. */ > > return -1; > > } > > > > @@ -2513,11 +2527,12 @@ memrefs_conflict_p (poly_int64 xsize, rtx x, poly_int64 ysize, rtx y, > > > > if (GET_CODE (x) == SYMBOL_REF && GET_CODE (y) == SYMBOL_REF) > > { > > - int cmp = compare_base_symbol_refs (x,y); > > + HOST_WIDE_INT distance = 0; > > + int cmp = compare_base_symbol_refs (x, y, &distance); > > > > /* If both decls are the same, decide by offsets. */ > > if (cmp == 1) > > - return offset_overlap_p (c, xsize, ysize); > > + return offset_overlap_p (c + distance, xsize, ysize); > > /* 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 > > @@ -2526,7 +2541,7 @@ memrefs_conflict_p (poly_int64 xsize, rtx x, poly_int64 ysize, rtx y, > > 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 (!cmp || !offset_overlap_p (c + distance, xsize, ysize)) > > return 0; > > /* Decls may or may not be different and offsets overlap....*/ > > return -1; > > > > -- > Richard Biener > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, > Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)