diff mbox series

tree-optimization/96881 - CD-DCE and CLOBBERs

Message ID 20220217102903.F254A13DD8@imap2.suse-dmz.suse.de
State New
Headers show
Series tree-optimization/96881 - CD-DCE and CLOBBERs | expand

Commit Message

Richard Biener Feb. 17, 2022, 10:29 a.m. UTC 
CD-DCE does not consider CLOBBERs as necessary in the attempt
to not prevent DCE of SSA defs it uses.  A side-effect of that
is that it also removes all its control dependences if they are
not made necessary by other means.  When we later try to preserve
as many CLOBBERs as possible we have to make sure we also
preserved the controlling conditions, otherwise a CLOBBER can
now appear on a path where it was not executed before, leading
to wrong code as seen in the testcase.

I've tried to continue to handle both direct and indirect
CLOBBERs optimistically, allowing CD-DCE to remove control
flow that just controls CLOBBERs but that regresses for
example the stack coalescing test g++.dg/opt/pr80032.C.
The pattern there is
  if (pred) D.2512 = CLOBBER; else D.2512 = CLOBBER;
basically we have all paths leading to the same clobber but
we could safely cut some branches which we do not realize
early enough.  This regression can be mitigated by no longer
considering direct CLOBBERs optimistically - the original
motivation for the CD-DCE handling wasn't removal of control
flow but SSA defs of the address.

Handling indirect vs. direct clobbers differently feels
somewhat wrong, still the patch goes with this solution.

Bootstrapped and tested on x86_64-unknown-linux-gnu, will
push later today unless I hear otherwise.

Richard.

2022-02-15  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/96881
	* tree-ssa-dce.cc (mark_stmt_if_obviously_necessary): Comment
	CLOBBER handling.
	(control_parents_preserved_p): New function.
	(eliminate_unnecessary_stmts): Check that we preserved control
	parents before retaining a CLOBBER.
	(perform_tree_ssa_dce): Pass down aggressive flag
	to eliminate_unnecessary_stmts.

	* g++.dg/torture/pr96881-1.C: New testcase.
	* g++.dg/torture/pr96881-2.C: Likewise.
---
 gcc/testsuite/g++.dg/torture/pr96881-1.C | 37 ++++++++++++++++++++++++
 gcc/testsuite/g++.dg/torture/pr96881-2.C | 37 ++++++++++++++++++++++++
 gcc/tree-ssa-dce.cc                      | 37 +++++++++++++++++++++---
 3 files changed, 107 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/torture/pr96881-1.C
 create mode 100644 gcc/testsuite/g++.dg/torture/pr96881-2.C

Comments

Jan Hubicka Feb. 17, 2022, 10:58 a.m. UTC | #1 
> +/* Returns whether the control parents of BB are preserved.  */
> +
> +static bool
> +control_parents_preserved_p (basic_block bb)
> +{
> +  /* If we marked the control parents from BB they are preserved.  */
> +  if (bitmap_bit_p (visited_control_parents, bb->index))
> +    return true;
> +
> +  /* But they can also end up being marked from elsewhere.  */
> +  bitmap_iterator bi;
> +  unsigned edge_number;
> +  EXECUTE_IF_SET_IN_BITMAP (cd->get_edges_dependent_on (bb->index),
> +			    0, edge_number, bi)
> +    {
> +      basic_block cd_bb = cd->get_edge_src (edge_number);
> +      if (cd_bb != bb
> +	  && !bitmap_bit_p (last_stmt_necessary, cd_bb->index))
> +	return false;
> +    }
> +  return true;
I suppose you can also set visited_control_parents bit here so the loop
is not re-done for every clobber in a BB.

Honza
Richard Biener Feb. 17, 2022, 11 a.m. UTC | #2 
On Thu, 17 Feb 2022, Jan Hubicka wrote:

> > +/* Returns whether the control parents of BB are preserved.  */
> > +
> > +static bool
> > +control_parents_preserved_p (basic_block bb)
> > +{
> > +  /* If we marked the control parents from BB they are preserved.  */
> > +  if (bitmap_bit_p (visited_control_parents, bb->index))
> > +    return true;
> > +
> > +  /* But they can also end up being marked from elsewhere.  */
> > +  bitmap_iterator bi;
> > +  unsigned edge_number;
> > +  EXECUTE_IF_SET_IN_BITMAP (cd->get_edges_dependent_on (bb->index),
> > +			    0, edge_number, bi)
> > +    {
> > +      basic_block cd_bb = cd->get_edge_src (edge_number);
> > +      if (cd_bb != bb
> > +	  && !bitmap_bit_p (last_stmt_necessary, cd_bb->index))
> > +	return false;
> > +    }
> > +  return true;
> I suppose you can also set visited_control_parents bit here so the loop
> is not re-done for every clobber in a BB.

Good idea, will do that.

Richard.
diff mbox series

Patch

diff --git a/gcc/testsuite/g++.dg/torture/pr96881-1.C b/gcc/testsuite/g++.dg/torture/pr96881-1.C
new file mode 100644
index 00000000000..1c182e6a8b3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr96881-1.C
@@ -0,0 +1,37 @@ 
+/* { dg-do run } */
+
+struct S { int s; ~S () {} } s;
+
+void __attribute__((noipa))
+foo (struct S *s, int flag)
+{
+  s->s = 1;
+  // We have to makes sure to not make the inlined CLOBBER
+  // unconditional but we have to remove it to be able
+  // to elide the branch
+  if (!flag)
+    return;
+  s->~S();
+}
+
+void __attribute__((noipa))
+bar (struct S *s, int flag)
+{
+  s->s = 1;
+  // CD-DCE chooses an arbitrary path, try to present it
+  // with all variants
+  if (flag)
+    s->~S();
+}
+
+int
+main ()
+{
+  foo (&s, 0);
+  if (s.s != 1)
+    __builtin_abort ();
+  bar (&s, 0);
+  if (s.s != 1)
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/torture/pr96881-2.C b/gcc/testsuite/g++.dg/torture/pr96881-2.C
new file mode 100644
index 00000000000..35c788791e8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr96881-2.C
@@ -0,0 +1,37 @@ 
+/* { dg-do run } */
+
+struct S { int s; ~S () {} } s;
+
+void __attribute__((noipa))
+foo (int flag)
+{
+  s.s = 1;
+  // We have to makes sure to not make the inlined CLOBBER
+  // unconditional but we have to remove it to be able
+  // to elide the branch
+  if (!flag)
+    return;
+  s.~S();
+}
+
+void __attribute__((noipa))
+bar (int flag)
+{
+  s.s = 1;
+  // CD-DCE chooses an arbitrary path, try to present it
+  // with all variants
+  if (flag)
+    s.~S();
+}
+
+int
+main ()
+{
+  foo (0);
+  if (s.s != 1)
+    __builtin_abort ();
+  bar (0);
+  if (s.s != 1)
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/tree-ssa-dce.cc b/gcc/tree-ssa-dce.cc
index f1034878eaf..ea8c6c98b81 100644
--- a/gcc/tree-ssa-dce.cc
+++ b/gcc/tree-ssa-dce.cc
@@ -284,7 +284,10 @@  mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
       break;
 
     case GIMPLE_ASSIGN:
-      if (gimple_clobber_p (stmt))
+      /* Mark indirect CLOBBERs to be lazily removed if their SSA operands
+	 do not prevail.  That also makes control flow leading to them
+	 not necessary in aggressive mode.  */
+      if (gimple_clobber_p (stmt) && !zero_ssa_operands (stmt, SSA_OP_USE))
 	return;
       break;
 
@@ -1268,11 +1271,34 @@  maybe_optimize_arith_overflow (gimple_stmt_iterator *gsi,
   gimplify_and_update_call_from_tree (gsi, result);
 }
 
+/* Returns whether the control parents of BB are preserved.  */
+
+static bool
+control_parents_preserved_p (basic_block bb)
+{
+  /* If we marked the control parents from BB they are preserved.  */
+  if (bitmap_bit_p (visited_control_parents, bb->index))
+    return true;
+
+  /* But they can also end up being marked from elsewhere.  */
+  bitmap_iterator bi;
+  unsigned edge_number;
+  EXECUTE_IF_SET_IN_BITMAP (cd->get_edges_dependent_on (bb->index),
+			    0, edge_number, bi)
+    {
+      basic_block cd_bb = cd->get_edge_src (edge_number);
+      if (cd_bb != bb
+	  && !bitmap_bit_p (last_stmt_necessary, cd_bb->index))
+	return false;
+    }
+  return true;
+}
+
 /* Eliminate unnecessary statements. Any instruction not marked as necessary
    contributes nothing to the program, and can be deleted.  */
 
 static bool
-eliminate_unnecessary_stmts (void)
+eliminate_unnecessary_stmts (bool aggressive)
 {
   bool something_changed = false;
   basic_block bb;
@@ -1366,7 +1392,10 @@  eliminate_unnecessary_stmts (void)
 			  break;
 			}
 		    }
-		  if (!dead)
+		  if (!dead
+		      /* When doing CD-DCE we have to ensure all controls
+			 of the stmt are still live.  */
+		      && (!aggressive || control_parents_preserved_p (bb)))
 		    {
 		      bitmap_clear (debug_seen);
 		      continue;
@@ -1876,7 +1905,7 @@  perform_tree_ssa_dce (bool aggressive)
   propagate_necessity (aggressive);
   BITMAP_FREE (visited);
 
-  something_changed |= eliminate_unnecessary_stmts ();
+  something_changed |= eliminate_unnecessary_stmts (aggressive);
   something_changed |= cfg_altered;
 
   /* We do not update postdominators, so free them unconditionally.  */