public inbox for gcc-bugs@sourceware.org help / color / mirror / Atom feed
From: "hubicka at ucw dot cz" <gcc-bugzilla@gcc.gnu.org> 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 [thread overview] Message-ID: <bug-113787-4-QIO69Rxq89@http.gcc.gnu.org/bugzilla/> (raw) In-Reply-To: <bug-113787-4@http.gcc.gnu.org/bugzilla/> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 --- Comment #17 from Jan Hubicka <hubicka at ucw dot cz> --- > > 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. > > It does. But note it does _not_ for POINTER_PLUS where it treats > the offset operand as non-pointer. > > > 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. > > It looks like that function might treat that > > ADDR_EXPR <TARGET_MEM_REF <0B, ...>> > > as integer_zerop base. It does > > if (TREE_CODE (op) == ADDR_EXPR) > { > poly_int64 extra_offset = 0; > tree base = get_addr_base_and_unit_offset (TREE_OPERAND (op, 0), > &offset); > if (!base) > { > base = get_base_address (TREE_OPERAND (op, 0)); > if (TREE_CODE (base) != MEM_REF) > break; > offset_known = false; > } > else > { > if (TREE_CODE (base) != MEM_REF) > break; > > with a variable offset we fall to the TREE_CODE (base) != MEM_REF > and will have offset_known == true. Not sure what it does with > the result though (it's not the address of a decl). > > This function seems to oddly special-case != 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. > > 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. > > 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 > > /* See if memory location is clearly invalid. */ > if (integer_zerop (t)) > return flag_delete_null_pointer_checks; > > and that might be a problem. As said, we rely on > ADDR_EXPR <TARGET_MEM_REF <...> > 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. > > I'd say > > 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) == ADDR_EXPR) > + if (TREE_CODE (t) == ADDR_EXPR > + && TREE_CODE (TREE_OPERAND (t, 0)) != TARGET_MEM_REF) > return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0)); > return false; > } > > might eventually work? Alternatively a bit less aggressive like > the following. > > 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) == ADDR_EXPR) > + if (TREE_CODE (t) == ADDR_EXPR > + && (TREE_CODE (TREE_OPERAND (t, 0)) != TARGET_MEM_REF > + || TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) != > 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. > > 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
next prev parent reply other threads:[~2024-02-14 15:07 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-02-06 13:40 [Bug tree-optimization/113787] New: [14 " acoplan at gcc dot gnu.org 2024-02-06 13:49 ` [Bug tree-optimization/113787] " jakub at gcc dot gnu.org 2024-02-06 13:56 ` pinskia at gcc dot gnu.org 2024-02-06 13:57 ` acoplan at gcc dot gnu.org 2024-02-06 14:07 ` acoplan at gcc dot gnu.org 2024-02-06 14:13 ` [Bug tree-optimization/113787] [12/13/14 " jakub at gcc dot gnu.org 2024-02-06 14:19 ` pinskia at gcc dot gnu.org 2024-02-06 14:23 ` acoplan at gcc dot gnu.org 2024-02-06 15:41 ` hubicka at gcc dot gnu.org 2024-02-06 16:18 ` pinskia at gcc dot gnu.org 2024-02-07 8:48 ` rguenth at gcc dot gnu.org 2024-02-07 8:49 ` rguenth at gcc dot gnu.org 2024-02-08 14:40 ` acoplan at gcc dot gnu.org 2024-02-13 9:03 ` hubicka at gcc dot gnu.org 2024-02-13 9:21 ` rguenther at suse dot de 2024-02-13 18:21 ` hubicka at ucw dot cz 2024-02-14 8:19 ` rguenther at suse dot de 2024-02-14 15:07 ` Jan Hubicka 2024-02-14 15:07 ` hubicka at ucw dot cz [this message] 2024-02-14 15:09 ` rguenther at suse dot de 2024-02-14 15:18 ` hubicka at ucw dot cz 2024-05-16 9:07 ` [Bug tree-optimization/113787] [12/13/14/15 " acoplan at gcc dot gnu.org 2024-05-16 13:34 ` cvs-commit at gcc dot gnu.org 2024-05-16 13:39 ` [Bug tree-optimization/113787] [12/13/14 " hubicka at gcc dot gnu.org
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=bug-113787-4-QIO69Rxq89@http.gcc.gnu.org/bugzilla/ \ --to=gcc-bugzilla@gcc.gnu.org \ --cc=gcc-bugs@gcc.gnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).