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 E901C383B43C for ; Wed, 11 Aug 2021 12:38:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E901C383B43C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=ucw.cz Authentication-Results: sourceware.org; spf=none smtp.mailfrom=kam.mff.cuni.cz Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 8798A280616; Wed, 11 Aug 2021 14:38:16 +0200 (CEST) Date: Wed, 11 Aug 2021 14:38:16 +0200 From: Jan Hubicka To: Alexandre Oliva Cc: gcc-patches@gcc.gnu.org Subject: Re: ipa-modref: merge flags when adding escape Message-ID: <20210811123816.GA33954@kam.mff.cuni.cz> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Wed, 11 Aug 2021 12:38:21 -0000 > While working on some function splitting changes, I've got a > miscompilation in stagefeedback that I've tracked down to a > complicated scenario: > > - ipa-modref miscomputes a function parameter as having EAF_DIRECT, > because it's dereferenced and passed on to another function, but > add_escape_point does not update the flags for the dereferenced > SSA_NAME passed as a parameter, and the EAF_UNUSED in the value that > first initializes it, that remains unchanged throughout, causes > deref_flags to set EAF_DIRECT, among other flags. > > - structalias, seeing the EAF_DIRECT in the parameter for that > function, refrains from mak[ing]_transitive_closure_constraints for > a pointer passed in that parameter. > > - tree dse2 concludes the initializer of the pointed-to variable is a > dead store and removes it. > > The test depends on gimple passes's processing of functions in a > certain order to expose parm flag miscomputed by ipa-modref. A > different order may enable the non-ipa modref2 pass to compute flags > differently and avoid the problem. > > I've arranged for add_escape_point to merge flags, as the non-ipa path > does. I've also caught and fixed an error in the dumping of escaping > flags. > > The problem affects at least trunk and gcc-11. I've so far bootstrapped > GCC 11, and I'm now regstrapping trunk. Ok to install if it passes? > > > for gcc/ChangeLog > > * ipa-modref.c (modref_lattice::add_escape_point): Merge > min_flags into flags. > (modref_lattice::dump): Fix escape_point's min_flags dumping. > > for gcc/testsuite/ChangeLog > > * c-c++-common/modref-dse.c: New. Hi, thank you for looking into the bug and sorry for taking so long to respond. The fix you propose is bit too generous, since it essentially disable IPA bits of the ipa-modref (it will resort to worst case solution w/o any IPA propagation). In IPA mode the proper flags are supposed to be determined by propagation via "escape points". The bug is bit subtle caused by optimization that avoids recording flags for escape points where we know that we do not care. This is tested by comparing min_flags (which is the known conservative estimation used by local analysis) with flags of the value being determined. If these flags are subset of min_flags there is nothing to gain. While merging lattices there is case where direct escape becomes indirect and in this case I forgot to update min_flags to dereferenced version which in turn makes the escape point to be skipped. This is improved patch I have bootstrapped/regtested x86_64-linux and I am collecting stats for (it should have minimal effect on overal effectivity of modref). Honza gcc/ChangeLog: 2021-08-11 Jan Hubicka Alexandre Oliva * ipa-modref.c (modref_lattice::dump): Fix escape_point's min_flags dumping. (modref_lattice::merge_deref): Fix handling of indirect scape points. (update_escape_summary_1): Likewise. (update_escape_summary): Likewise. (ipa_merge_modref_summary_after_inlining): Likewise. gcc/testsuite/ChangeLog: 2021-08-11 Alexandre Oliva * c-c++-common/modref-dse.c: New test. diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c index ef5e62beb0e..dccaf658720 100644 --- a/gcc/ipa-modref.c +++ b/gcc/ipa-modref.c @@ -1392,7 +1392,7 @@ modref_lattice::dump (FILE *out, int indent) const fprintf (out, "%*s Arg %i (%s) min flags", indent, "", escape_points[i].arg, escape_points[i].direct ? "direct" : "indirect"); - dump_eaf_flags (out, flags, false); + dump_eaf_flags (out, escape_points[i].min_flags, false); fprintf (out, " in call "); print_gimple_stmt (out, escape_points[i].call, 0); } @@ -1489,10 +1489,18 @@ modref_lattice::merge_deref (const modref_lattice &with, bool ignore_stores) if (!flags) return changed; for (unsigned int i = 0; i < with.escape_points.length (); i++) - changed |= add_escape_point (with.escape_points[i].call, - with.escape_points[i].arg, - with.escape_points[i].min_flags, - false); + { + int min_flags = with.escape_points[i].min_flags; + + if (with.escape_points[i].direct) + min_flags = deref_flags (min_flags, ignore_stores); + else if (ignore_stores) + min_flags |= EAF_NOCLOBBER | EAF_NOESCAPE | EAF_NODIRECTESCAPE; + changed |= add_escape_point (with.escape_points[i].call, + with.escape_points[i].arg, + min_flags, + false); + } return changed; } @@ -2992,7 +3000,8 @@ struct escape_map static void update_escape_summary_1 (cgraph_edge *e, - vec > &map) + vec > &map, + bool ignore_stores) { escape_summary *sum = escape_summaries->get (e); if (!sum) @@ -3010,6 +3019,9 @@ update_escape_summary_1 (cgraph_edge *e, continue; FOR_EACH_VEC_ELT (map[ee->parm_index], j, em) { + int min_flags = ee->min_flags; + if (ee->direct && !em->direct) + min_flags = deref_flags (min_flags, ignore_stores); struct escape_entry entry = {em->parm_index, ee->arg, ee->min_flags, ee->direct & em->direct}; @@ -3024,18 +3036,19 @@ update_escape_summary_1 (cgraph_edge *e, static void update_escape_summary (cgraph_node *node, - vec > &map) + vec > &map, + bool ignore_stores) { if (!escape_summaries) return; for (cgraph_edge *e = node->indirect_calls; e; e = e->next_callee) - update_escape_summary_1 (e, map); + update_escape_summary_1 (e, map, ignore_stores); for (cgraph_edge *e = node->callees; e; e = e->next_callee) { if (!e->inline_failed) - update_escape_summary (e->callee, map); + update_escape_summary (e->callee, map, ignore_stores); else - update_escape_summary_1 (e, map); + update_escape_summary_1 (e, map, ignore_stores); } } @@ -3160,7 +3173,7 @@ ipa_merge_modref_summary_after_inlining (cgraph_edge *edge) if (needed) emap[ee->arg].safe_push (entry); } - update_escape_summary (edge->callee, emap); + update_escape_summary (edge->callee, emap, ignore_stores); for (i = 0; (int)i < max_escape + 1; i++) emap[i].release (); if (sum) diff --git a/gcc/testsuite/c-c++-common/modref-dse.c b/gcc/testsuite/c-c++-common/modref-dse.c new file mode 100644 index 00000000000..5f64e8f4b59 --- /dev/null +++ b/gcc/testsuite/c-c++-common/modref-dse.c @@ -0,0 +1,38 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-dse2-details" } */ +/* { dg-final { scan-tree-dump-not "Deleted dead store" "dse2" } } */ + +struct foo { unsigned long bar; }; + +unsigned y; + +static int __attribute__ ((__noinline__, __noclone__)) +wrapped (struct foo *p, int i); + +static int wrapper (struct foo *p); + +static int __attribute__ ((__noclone__)) +wrapper (struct foo *p) { + return wrapped (p, 1); +} + +static int __attribute__ ((__noinline__, __noclone__)) +dind (struct foo **pp); + +int __attribute__ ((__noclone__, __no_reorder__)) +xfn () { + struct foo x = { 0xBADC0FFE }; + struct foo *p = &x; + return dind (&p); +} + +static int __attribute__ ((__noinline__, __no_reorder__)) +wrapped (struct foo *p, int i) { + return p->bar + i == y++; +} + +static int __attribute__ ((__noinline__, __noclone__, __no_reorder__)) +dind (struct foo **pp) { + wrapper (*pp); + return 0; +}