ipa-modref: merge flags when adding escape

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.
---
 gcc/ipa-modref.c                        |    4 ++-
 gcc/testsuite/c-c++-common/modref-dse.c |   38 +++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/modref-dse.c

Ping?  https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573137.html

On Jun 18, 2021, Alexandre Oliva <oliva@adacore.com> wrote:

> 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.

> 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  <hubicka@ucw.cz>
	    Alexandre Oliva  <oliva@adacore.com>

	* 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  <oliva@adacore.com>

	* 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 <vec <escape_map>> &map)
+			 vec <vec <escape_map>> &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 <vec <escape_map>> &map)
+		       vec <vec <escape_map>> &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;
+}

On Aug 11, 2021, Jan Hubicka <hubicka@ucw.cz> wrote:

> This is improved patch

Thanks for the proper fix!