diff mbox

Fix PR80032 - handle CLOBBER gimplification differently

Message ID alpine.LSU.2.20.1703171356130.30051@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener March 17, 2017, 1:42 p.m. UTC 
The following addresses PR80032 which reports that stack usage has gone
up since DCE got the ability to remove clobbers.  (Control-dependent-)DCE
basically treats clobbers as not necessary and after eliminating
unnecessary stmts sees if it can retain them and if not, removes them.
This ability was added to keep removing "empty" loops (well, now loops
with clobbers).  With control-dependent DCE this always will remove
code like

 if (cond)
   x = CLOBBER;

because as the clobber itself is not necessary the controlling stmt
isn't either and thus the BB with the clobber gets removed.  We can't
simply promote the CLOBBER to execute unconditionally because that
changes semantics.

But what we can do (I think) and what the patch does is at the time
we add the CLOBBER, add it to a point post-dominating the old insertion
point to avoid the above situation.  This will be a spurious one
in the case the condition was not true but at this point we know
that the variable wasn't live in that case.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

Ok?

Thanks,
Richard.

2017-03-17  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/80032
	* gimplify.c (gimple_push_cleanup): Add force_uncond parameter,
	if set force the cleanup to happen unconditionally.
	(gimplify_target_expr): Push inserted clobbers with force_uncond
	to avoid them being removed by control-dependent DCE.

	* g++.dg/opt/pr80032.C: New testcase.

Index: gcc/gimplify.c
===================================================================
*** gcc/gimplify.c	(revision 246216)
--- gcc/gimplify.c	(working copy)
*************** gimplify_cleanup_point_expr (tree *expr_
*** 6288,6297 ****
  
  /* Insert a cleanup marker for gimplify_cleanup_point_expr.  CLEANUP
     is the cleanup action required.  EH_ONLY is true if the cleanup should
!    only be executed if an exception is thrown, not on normal exit.  */
  
  static void
! gimple_push_cleanup (tree var, tree cleanup, bool eh_only, gimple_seq *pre_p)
  {
    gimple *wce;
    gimple_seq cleanup_stmts = NULL;
--- 6288,6300 ----
  
  /* Insert a cleanup marker for gimplify_cleanup_point_expr.  CLEANUP
     is the cleanup action required.  EH_ONLY is true if the cleanup should
!    only be executed if an exception is thrown, not on normal exit.
!    If FORCE_UNCOND is true perform the cleanup unconditionally;  this is
!    only valid for clobbers.  */
  
  static void
! gimple_push_cleanup (tree var, tree cleanup, bool eh_only, gimple_seq *pre_p,
! 		     bool force_uncond = false)
  {
    gimple *wce;
    gimple_seq cleanup_stmts = NULL;
*************** gimple_push_cleanup (tree var, tree clea
*** 6301,6307 ****
    if (seen_error ())
      return;
  
!   if (gimple_conditional_context ())
      {
        /* If we're in a conditional context, this is more complex.  We only
  	 want to run the cleanup if we actually ran the initialization that
--- 6304,6311 ----
    if (seen_error ())
      return;
  
!   if (gimple_conditional_context ()
!       && ! force_uncond)
      {
        /* If we're in a conditional context, this is more complex.  We only
  	 want to run the cleanup if we actually ran the initialization that
*************** gimplify_target_expr (tree *expr_p, gimp
*** 6426,6436 ****
  						NULL);
  	      TREE_THIS_VOLATILE (clobber) = true;
  	      clobber = build2 (MODIFY_EXPR, TREE_TYPE (temp), temp, clobber);
! 	      if (cleanup)
! 		cleanup = build2 (COMPOUND_EXPR, void_type_node, cleanup,
! 				  clobber);
! 	      else
! 		cleanup = clobber;
  	    }
  	  if (asan_poisoned_variables && dbg_cnt (asan_use_after_scope))
  	    {
--- 6430,6436 ----
  						NULL);
  	      TREE_THIS_VOLATILE (clobber) = true;
  	      clobber = build2 (MODIFY_EXPR, TREE_TYPE (temp), temp, clobber);
! 	      gimple_push_cleanup (temp, clobber, false, pre_p, true);
  	    }
  	  if (asan_poisoned_variables && dbg_cnt (asan_use_after_scope))
  	    {

Comments

Jakub Jelinek March 20, 2017, 5:43 p.m. UTC | #1 
On Fri, Mar 17, 2017 at 02:42:27PM +0100, Richard Biener wrote:
> 2017-03-17  Richard Biener  <rguenther@suse.de>
> 
> 	PR tree-optimization/80032
> 	* gimplify.c (gimple_push_cleanup): Add force_uncond parameter,
> 	if set force the cleanup to happen unconditionally.
> 	(gimplify_target_expr): Push inserted clobbers with force_uncond
> 	to avoid them being removed by control-dependent DCE.
> 
> 	* g++.dg/opt/pr80032.C: New testcase.
> 
> !   if (gimple_conditional_context ()
> !       && ! force_uncond)

This ought to fit on one line.

Otherwise it looks good to me.  Does the
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80032#c8
regression happen with this patch or the earlier version of the patch?

> --- gcc/testsuite/g++.dg/opt/pr80032.C	(nonexistent)
> +++ gcc/testsuite/g++.dg/opt/pr80032.C	(working copy)
> @@ -0,0 +1,121 @@
> +// PR tree-optimization/80032
> +/* { dg-do compile } */
> +/* { dg-require-effective-target c++11 } */
> +/* { dg-options "-O2" } */

It is weird to mix // and /* */ comments, better just pick one style.
Also, // { dg-do compile { target c++11 } } is perhaps simpler.

> +/* If DCE removes too many CLOBBERs then stack usage goes through the
> +   roof as stack slots can no longer be shared.  */
> +/* { dg-additional-options "-Wstack-usage=200" { target x86_64-*-* i?86-*-* } } */

	Jakub
Richard Biener March 21, 2017, 8:24 a.m. UTC | #2 
On Mon, 20 Mar 2017, Jakub Jelinek wrote:

> On Fri, Mar 17, 2017 at 02:42:27PM +0100, Richard Biener wrote:
> > 2017-03-17  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR tree-optimization/80032
> > 	* gimplify.c (gimple_push_cleanup): Add force_uncond parameter,
> > 	if set force the cleanup to happen unconditionally.
> > 	(gimplify_target_expr): Push inserted clobbers with force_uncond
> > 	to avoid them being removed by control-dependent DCE.
> > 
> > 	* g++.dg/opt/pr80032.C: New testcase.
> > 
> > !   if (gimple_conditional_context ()
> > !       && ! force_uncond)
> 
> This ought to fit on one line.
> 
> Otherwise it looks good to me.  Does the
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80032#c8
> regression happen with this patch or the earlier version of the patch?

With the DCE patch only.

> > --- gcc/testsuite/g++.dg/opt/pr80032.C	(nonexistent)
> > +++ gcc/testsuite/g++.dg/opt/pr80032.C	(working copy)
> > @@ -0,0 +1,121 @@
> > +// PR tree-optimization/80032
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target c++11 } */
> > +/* { dg-options "-O2" } */
> 
> It is weird to mix // and /* */ comments, better just pick one style.
> Also, // { dg-do compile { target c++11 } } is perhaps simpler.
> 
> > +/* If DCE removes too many CLOBBERs then stack usage goes through the
> > +   roof as stack slots can no longer be shared.  */
> > +/* { dg-additional-options "-Wstack-usage=200" { target x86_64-*-* i?86-*-* } } */

I'll adjust and commit.

Thanks,
Richard.
diff mbox

Patch

Index: gcc/testsuite/g++.dg/opt/pr80032.C
===================================================================
Index: gcc/testsuite/g++.dg/opt/pr80032.C
===================================================================
--- gcc/testsuite/g++.dg/opt/pr80032.C	(nonexistent)
+++ gcc/testsuite/g++.dg/opt/pr80032.C	(working copy)
@@ -0,0 +1,121 @@ 
+// PR tree-optimization/80032
+/* { dg-do compile } */
+/* { dg-require-effective-target c++11 } */
+/* { dg-options "-O2" } */
+/* If DCE removes too many CLOBBERs then stack usage goes through the
+   roof as stack slots can no longer be shared.  */
+/* { dg-additional-options "-Wstack-usage=200" { target x86_64-*-* i?86-*-* } } */
+
+typedef unsigned a;
+namespace test {
+    enum b { c };
+    class ADataContainer;
+    class BitMask;
+    namespace api {
+	enum DataStore { candidate };
+    }
+    using d = api::DataStore;
+    namespace db {
+	class e;
+	class f;
+	class g;
+	class ManagedObjectConst {
+	public:
+	    ManagedObjectConst(const ManagedObjectConst &);
+	    bool isFieldDefault(a, d) const;
+	    ADataContainer &getFieldDefault(a, d) const;
+	    g *h;
+	    e *i;
+	    f *j;
+	};
+	struct FieldInfo {
+	    FieldInfo(ManagedObjectConst, a, d);
+	    ManagedObjectConst k;
+	};
+	b compare(const FieldInfo &, const ADataContainer &);
+	class ManagedObject : public ManagedObjectConst {};
+    }
+    using namespace db;
+    void FN(ManagedObject &k, const BitMask &) {
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+	if (!k.isFieldDefault(8, d::candidate) &&
+	    !compare(FieldInfo(k, 11, d::candidate),
+		     k.getFieldDefault(11, d::candidate)) == c)
+	  return;
+    }
+}