From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id 643B9385E448; Wed, 14 Feb 2024 15:07:27 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 643B9385E448 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1707923247; bh=pQRzvn5urfpBsBCYs+XbYCRdUdgYgrZJ3f4VQFPnNtY=; h=From:To:Subject:Date:In-Reply-To:References:From; b=JDSHg3JjOESqeIpxp1SUgtYE6eNrNOMmQTzk2D3VIAxxtL9bFfEaPXMkacuU3MzYl A/Z9ywT0CUloUDcGYlPvFR27w6L90wQejjTjvt83gpmu7gezuN/90WPMOCWyNaFg5N E0APTyxDTnhLL6ZKH/zEwxV2KGrmBFoPbCyjfPAc= From: "hubicka at ucw dot cz" To: gcc-bugs@gcc.gnu.org Subject: [Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64 Date: Wed, 14 Feb 2024 15:07:22 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: tree-optimization X-Bugzilla-Version: 14.0 X-Bugzilla-Keywords: wrong-code X-Bugzilla-Severity: normal X-Bugzilla-Who: hubicka at ucw dot cz X-Bugzilla-Status: NEW X-Bugzilla-Resolution: X-Bugzilla-Priority: P2 X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org X-Bugzilla-Target-Milestone: 12.4 X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 List-Id: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D113787 --- Comment #17 from Jan Hubicka --- > > I guess PTA gets around by tracking points-to set also for non-pointer > > types and consequently it also gives up on any such addition. >=20 > It does. But note it does _not_ for POINTER_PLUS where it treats > the offset operand as non-pointer. >=20 > > I think it is ipa-prop.c::unadjusted_ptr_and_unit_offset. It accepts > > pointer_plus expression, but does not look through POINTER_PLUS. > > We can restrict it further, but tracking base pointer is quite useful, > > so it would be nice to not give up completely. >=20 > It looks like that function might treat that >=20 > ADDR_EXPR > >=20 > as integer_zerop base. It does >=20 > if (TREE_CODE (op) =3D=3D ADDR_EXPR)=20 > { > poly_int64 extra_offset =3D 0;=20 > tree base =3D get_addr_base_and_unit_offset (TREE_OPERAND (op, = 0), > &offset); > if (!base) > { > base =3D get_base_address (TREE_OPERAND (op, 0)); > if (TREE_CODE (base) !=3D MEM_REF) > break; > offset_known =3D false; > } > else > { > if (TREE_CODE (base) !=3D MEM_REF) > break; >=20 > with a variable offset we fall to the TREE_CODE (base) !=3D MEM_REF > and will have offset_known =3D=3D true. Not sure what it does with > the result though (it's not the address of a decl). >=20 > This function seems to oddly special-case !=3D MEM_REF ... (maybe > it wants to hande DECL_P () as finishing? Hmm the function was definitely not written with TARGET_MEM_REF in mind, since it was originally used for IPA passes only. We basically want to handle stuff like &decl->foo or &(ptr->foo) In the second case we want to continue the SSA walk to hopefully work out the origin of PTR. ipa-modref then looks if the base pointer is derived from function parameter or points to local or readonly memory to produce its summary. >=20 > Note get_addr_base_and_unit_offset will return NULL for > a TARGET_MEM_REF <&decl, ..., offset> but TARGET_MEM_REF > itself if the base isn't an ADDR_EXPR, irrespective of whether > the offset within it is constant or not. Hmm, interesting. I would expect it to interpret the emantics of TMR and return base. >=20 > Not sure if the above is a problem, but it seems the only caller > will just call points_to_local_or_readonly_memory_p on the > ADDR_EXPR where refs_local_or_readonly_memory_p via > points_to_local_or_readonly_memory_p will eventually do >=20 > /* See if memory location is clearly invalid. */ > if (integer_zerop (t)) > return flag_delete_null_pointer_checks; >=20 > and that might be a problem. As said, we rely on > ADDR_EXPR > to be an address computation > that's not subject to strict interpretation to allow IVOPTs > doing this kind of optimization w/o introducing some kind of > INTEGER_LEA <...>. I know that's a bit awkward but we should > make sure this is honored by IPA as well. >=20 > I'd say >=20 > diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc > index 74c9b4e1d1e..45a770cf940 100644 > --- a/gcc/ipa-fnsummary.cc > +++ b/gcc/ipa-fnsummary.cc > @@ -2642,7 +2642,8 @@ points_to_local_or_readonly_memory_p (tree t) > return true; > return !ptr_deref_may_alias_global_p (t, false); > } > - if (TREE_CODE (t) =3D=3D ADDR_EXPR) > + if (TREE_CODE (t) =3D=3D ADDR_EXPR > + && TREE_CODE (TREE_OPERAND (t, 0)) !=3D TARGET_MEM_REF) > return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0)); > return false; > } >=20 > might eventually work? Alternatively a bit less aggressive like > the following. >=20 > diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc > index 74c9b4e1d1e..7c79adf6440 100644 > --- a/gcc/ipa-fnsummary.cc > +++ b/gcc/ipa-fnsummary.cc > @@ -2642,7 +2642,9 @@ points_to_local_or_readonly_memory_p (tree t) > return true; > return !ptr_deref_may_alias_global_p (t, false); > } > - if (TREE_CODE (t) =3D=3D ADDR_EXPR) > + if (TREE_CODE (t) =3D=3D ADDR_EXPR > + && (TREE_CODE (TREE_OPERAND (t, 0)) !=3D TARGET_MEM_REF > + || TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) !=3D=20 > INTEGER_CST)) > return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0)); > return false; > } Yes, those both looks reasonable to me, perhaps less agressive would be better.=20 >=20 > A "nicer" solution might be to add a informational operand > to TARGET_MEM_REF, representing the base pointer to be used for > alias/points-to purposes. But if that's not invariant it might > keep some otherwise unnecessary definition stmts live. Yep, I see that forcing extra IV to track original semantics would be trouble here. I think that after iv-opts we should be done with more fancy propagation across loops. However, to avoid ipa-modref summary degradation, perhaps scheduling the pass before ivopts would make sense... Thanks, Honza=