public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Avoid DSE/DCE of pure call that throws
@ 2021-05-03 13:30 Richard Biener
  2021-05-03 13:34 ` Jan Hubicka
  2021-05-03 14:04 ` Eric Botcazou
  0 siblings, 2 replies; 9+ messages in thread
From: Richard Biener @ 2021-05-03 13:30 UTC (permalink / raw)
  To: gcc-patches; +Cc: ebotcazou, jakub, jeffreyalaw

There is -fdelete-dead-exceptions now and we're tracking
nothrow and const/pure bits separately and I do remember that
const or pure does _not_ imply nothrow.

Now, in the light of the PR100382 fix which added a
stmt_unremovable_because_of_non_call_eh_p guard to DSEs "DCE"
I wondered how -fdelete-dead-exceptions applies to calls and
whether stmt_unremovable_because_of_non_call_eh_p doing

  return (fun->can_throw_non_call_exceptions
          && !fun->can_delete_dead_exceptions
          && stmt_could_throw_p (fun, stmt));

really should conditionalize itself on
fun->can_throw_non_call_exceptions.  In fact DCE happily elides
pure function calls that throw without a LHS (probably a
consistency bug).  The following testcase shows this:

int x, y;
int __attribute__((pure,noinline)) foo () { if (x) throw; return y; }

int main()
{
  int a[2];
  x = 1;
  try {
    int res = foo ();
    a[0] = res;
  } catch (...) {
      return 0;
  }
  return 1;
}

note that if you wrap foo () into another noinline
wrap_foo () { foo (); return 1; } function then we need to make
sure to not DCE this call either even though it only throws
externally.  Now the question is whether this testcase is valid
(it should not abort).  The documentation of 'pure' must be read
as that 'pure' is not valid for 'foo' since throwing an exception
is obviously an observable effect on the state of the program
(in particular for the testcase at hand).  But for example
IPA pure const does not cause us to infer nothrow on pure
declared functions and the C++ program

extern int __attribute__((pure)) foo();
int bar()
{
  try
    {
      return foo ();
    }
  catch (...)
    {
      return -1;
    }
}

is compiled with full EH in place. (clang elides all of it, btw)

The set of changes below do not succeed in it passing but at least
the throwing call prevails but somehow EH info is hosed and you'll
get

> ./pr100382.exe
terminate called without an active exception
Aborted (core dumped)

So - what is it?  Are pure functions allowed to throw or not?

2021-05-03  Richard Biener  <rguenther@suse.de>

	* calls.c (expand_call): Preserve possibly throwing calls.
	* cfgexpand.c (expand_call_stmt): When a call can throw signal
	RTL expansion there are side-effects.
	* tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Simplify.
	* tree-ssa-dse.c (pass_dse::execute): Preserve exceptions unless
	-fdelete-dead-exceptions.

	* g++.dg/tree-ssa/pr100382.C: New testcase.
---
 gcc/calls.c                              |  1 +
 gcc/cfgexpand.c                          |  5 ++++-
 gcc/testsuite/g++.dg/tree-ssa/pr100382.C | 25 ++++++++++++++++++++++++
 gcc/tree-ssa-dce.c                       | 21 +++-----------------
 gcc/tree-ssa-dse.c                       |  3 ++-
 5 files changed, 35 insertions(+), 20 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr100382.C

diff --git a/gcc/calls.c b/gcc/calls.c
index 883d08ba5f2..f3da1839dc5 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -3808,6 +3808,7 @@ expand_call (tree exp, rtx target, int ignore)
      side-effects.  */
   if ((flags & (ECF_CONST | ECF_PURE))
       && (!(flags & ECF_LOOPING_CONST_OR_PURE))
+      && (flags & ECF_NOTHROW)
       && (ignore || target == const0_rtx
 	  || TYPE_MODE (rettype) == VOIDmode))
     {
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index a6b48d3e48f..556a0b22ed6 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2795,7 +2795,10 @@ expand_call_stmt (gcall *stmt)
       CALL_EXPR_ARG (exp, i) = arg;
     }
 
-  if (gimple_has_side_effects (stmt))
+  if (gimple_has_side_effects (stmt)
+      /* ???  Downstream in expand_expr_real_1 we assume that expressions
+	 w/o side-effects do not throw so work around this here.  */
+      || stmt_could_throw_p (cfun, stmt))
     TREE_SIDE_EFFECTS (exp) = 1;
 
   if (gimple_call_nothrow_p (stmt))
diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr100382.C b/gcc/testsuite/g++.dg/tree-ssa/pr100382.C
new file mode 100644
index 00000000000..b9948d3034a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-ssa/pr100382.C
@@ -0,0 +1,25 @@
+// { dg-do run }
+// { dg-options "-O2" }
+
+int x, y;
+int __attribute__((pure,noinline)) foo () { if (x) throw; return y; }
+
+int __attribute__((noinline)) bar()
+{
+  int a[2];
+  x = 1;
+  try {
+    int res = foo ();
+    a[0] = res;
+  } catch (...) {
+      return 0;
+  }
+  return 1;
+}
+
+int main()
+{
+  if (bar ())
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index 096cfc8721d..ff0389af36f 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -250,14 +250,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
 	    && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee))
 	  return;
 
-	/* Most, but not all function calls are required.  Function calls that
-	   produce no result and have no side effects (i.e. const pure
-	   functions) are unnecessary.  */
-	if (gimple_has_side_effects (stmt))
-	  {
-	    mark_stmt_necessary (stmt, true);
-	    return;
-	  }
 	/* IFN_GOACC_LOOP calls are necessary in that they are used to
 	   represent parameter (i.e. step, bound) of a lowered OpenACC
 	   partitioned loop.  But this kind of partitioned loop might not
@@ -269,8 +261,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
 	    mark_stmt_necessary (stmt, true);
 	    return;
 	  }
-	if (!gimple_call_lhs (stmt))
-	  return;
 	break;
       }
 
@@ -312,19 +302,14 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
   /* If the statement has volatile operands, it needs to be preserved.
      Same for statements that can alter control flow in unpredictable
      ways.  */
-  if (gimple_has_volatile_ops (stmt) || is_ctrl_altering_stmt (stmt))
-    {
-      mark_stmt_necessary (stmt, true);
-      return;
-    }
-
-  if (stmt_may_clobber_global_p (stmt))
+  if (gimple_has_side_effects (stmt) || is_ctrl_altering_stmt (stmt))
     {
       mark_stmt_necessary (stmt, true);
       return;
     }
 
-  if (gimple_vdef (stmt) && keep_all_vdefs_p ())
+  if ((gimple_vdef (stmt) && keep_all_vdefs_p ())
+      || stmt_may_clobber_global_p (stmt))
     {
       mark_stmt_necessary (stmt, true);
       return;
diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
index dfa6d314727..386667b9f7f 100644
--- a/gcc/tree-ssa-dse.c
+++ b/gcc/tree-ssa-dse.c
@@ -1219,7 +1219,8 @@ pass_dse::execute (function *fun)
 		 dead SSA defs.  */
 	      if (has_zero_uses (DEF_FROM_PTR (def_p))
 		  && !gimple_has_side_effects (stmt)
-		  && !stmt_unremovable_because_of_non_call_eh_p (cfun, stmt))
+		  && (!stmt_could_throw_p (fun, stmt)
+		      || fun->can_delete_dead_exceptions))
 		{
 		  if (dump_file && (dump_flags & TDF_DETAILS))
 		    {
-- 
2.26.2

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Avoid DSE/DCE of pure call that throws
  2021-05-03 13:30 [PATCH] Avoid DSE/DCE of pure call that throws Richard Biener
@ 2021-05-03 13:34 ` Jan Hubicka
  2021-05-03 14:02   ` Richard Biener
  2021-05-03 14:11   ` Michael Matz
  2021-05-03 14:04 ` Eric Botcazou
  1 sibling, 2 replies; 9+ messages in thread
From: Jan Hubicka @ 2021-05-03 13:34 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, jakub, ebotcazou

> note that if you wrap foo () into another noinline
> wrap_foo () { foo (); return 1; } function then we need to make
> sure to not DCE this call either even though it only throws
> externally.  Now the question is whether this testcase is valid
> (it should not abort).  The documentation of 'pure' must be read
> as that 'pure' is not valid for 'foo' since throwing an exception
> is obviously an observable effect on the state of the program
> (in particular for the testcase at hand).  But for example
> IPA pure const does not cause us to infer nothrow on pure
> declared functions and the C++ program
> 
> extern int __attribute__((pure)) foo();
> int bar()
> {
>   try
>     {
>       return foo ();
>     }
>   catch (...)
>     {
>       return -1;
>     }
> }
> 
> is compiled with full EH in place. (clang elides all of it, btw)
> 
> The set of changes below do not succeed in it passing but at least
> the throwing call prevails but somehow EH info is hosed and you'll
> get
> 
> > ./pr100382.exe
> terminate called without an active exception
> Aborted (core dumped)
> 
> So - what is it?  Are pure functions allowed to throw or not?

I was one adding pure functions and it was really intended to be same
thing as const+reading memory.  So does const function throw or not?
Internally we definitly want to make this separate - with symbol
summaries it is now reasonably easy to do.  I was thinking of doing
similar summary as modref handles to pass down separate info tracked by
pure-const patch rather than trying to re-synthetize it to rather random
attributes we have now...

Honza
> 
> 2021-05-03  Richard Biener  <rguenther@suse.de>
> 
> 	* calls.c (expand_call): Preserve possibly throwing calls.
> 	* cfgexpand.c (expand_call_stmt): When a call can throw signal
> 	RTL expansion there are side-effects.
> 	* tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Simplify.
> 	* tree-ssa-dse.c (pass_dse::execute): Preserve exceptions unless
> 	-fdelete-dead-exceptions.
> 
> 	* g++.dg/tree-ssa/pr100382.C: New testcase.
> ---
>  gcc/calls.c                              |  1 +
>  gcc/cfgexpand.c                          |  5 ++++-
>  gcc/testsuite/g++.dg/tree-ssa/pr100382.C | 25 ++++++++++++++++++++++++
>  gcc/tree-ssa-dce.c                       | 21 +++-----------------
>  gcc/tree-ssa-dse.c                       |  3 ++-
>  5 files changed, 35 insertions(+), 20 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> 
> diff --git a/gcc/calls.c b/gcc/calls.c
> index 883d08ba5f2..f3da1839dc5 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -3808,6 +3808,7 @@ expand_call (tree exp, rtx target, int ignore)
>       side-effects.  */
>    if ((flags & (ECF_CONST | ECF_PURE))
>        && (!(flags & ECF_LOOPING_CONST_OR_PURE))
> +      && (flags & ECF_NOTHROW)
>        && (ignore || target == const0_rtx
>  	  || TYPE_MODE (rettype) == VOIDmode))
>      {
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index a6b48d3e48f..556a0b22ed6 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -2795,7 +2795,10 @@ expand_call_stmt (gcall *stmt)
>        CALL_EXPR_ARG (exp, i) = arg;
>      }
>  
> -  if (gimple_has_side_effects (stmt))
> +  if (gimple_has_side_effects (stmt)
> +      /* ???  Downstream in expand_expr_real_1 we assume that expressions
> +	 w/o side-effects do not throw so work around this here.  */
> +      || stmt_could_throw_p (cfun, stmt))
>      TREE_SIDE_EFFECTS (exp) = 1;
>  
>    if (gimple_call_nothrow_p (stmt))
> diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr100382.C b/gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> new file mode 100644
> index 00000000000..b9948d3034a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> @@ -0,0 +1,25 @@
> +// { dg-do run }
> +// { dg-options "-O2" }
> +
> +int x, y;
> +int __attribute__((pure,noinline)) foo () { if (x) throw; return y; }
> +
> +int __attribute__((noinline)) bar()
> +{
> +  int a[2];
> +  x = 1;
> +  try {
> +    int res = foo ();
> +    a[0] = res;
> +  } catch (...) {
> +      return 0;
> +  }
> +  return 1;
> +}
> +
> +int main()
> +{
> +  if (bar ())
> +    __builtin_abort ();
> +  return 0;
> +}
> diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
> index 096cfc8721d..ff0389af36f 100644
> --- a/gcc/tree-ssa-dce.c
> +++ b/gcc/tree-ssa-dce.c
> @@ -250,14 +250,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
>  	    && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee))
>  	  return;
>  
> -	/* Most, but not all function calls are required.  Function calls that
> -	   produce no result and have no side effects (i.e. const pure
> -	   functions) are unnecessary.  */
> -	if (gimple_has_side_effects (stmt))
> -	  {
> -	    mark_stmt_necessary (stmt, true);
> -	    return;
> -	  }
>  	/* IFN_GOACC_LOOP calls are necessary in that they are used to
>  	   represent parameter (i.e. step, bound) of a lowered OpenACC
>  	   partitioned loop.  But this kind of partitioned loop might not
> @@ -269,8 +261,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
>  	    mark_stmt_necessary (stmt, true);
>  	    return;
>  	  }
> -	if (!gimple_call_lhs (stmt))
> -	  return;
>  	break;
>        }
>  
> @@ -312,19 +302,14 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
>    /* If the statement has volatile operands, it needs to be preserved.
>       Same for statements that can alter control flow in unpredictable
>       ways.  */
> -  if (gimple_has_volatile_ops (stmt) || is_ctrl_altering_stmt (stmt))
> -    {
> -      mark_stmt_necessary (stmt, true);
> -      return;
> -    }
> -
> -  if (stmt_may_clobber_global_p (stmt))
> +  if (gimple_has_side_effects (stmt) || is_ctrl_altering_stmt (stmt))
>      {
>        mark_stmt_necessary (stmt, true);
>        return;
>      }
>  
> -  if (gimple_vdef (stmt) && keep_all_vdefs_p ())
> +  if ((gimple_vdef (stmt) && keep_all_vdefs_p ())
> +      || stmt_may_clobber_global_p (stmt))
>      {
>        mark_stmt_necessary (stmt, true);
>        return;
> diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
> index dfa6d314727..386667b9f7f 100644
> --- a/gcc/tree-ssa-dse.c
> +++ b/gcc/tree-ssa-dse.c
> @@ -1219,7 +1219,8 @@ pass_dse::execute (function *fun)
>  		 dead SSA defs.  */
>  	      if (has_zero_uses (DEF_FROM_PTR (def_p))
>  		  && !gimple_has_side_effects (stmt)
> -		  && !stmt_unremovable_because_of_non_call_eh_p (cfun, stmt))
> +		  && (!stmt_could_throw_p (fun, stmt)
> +		      || fun->can_delete_dead_exceptions))
>  		{
>  		  if (dump_file && (dump_flags & TDF_DETAILS))
>  		    {
> -- 
> 2.26.2

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Avoid DSE/DCE of pure call that throws
  2021-05-03 13:34 ` Jan Hubicka
@ 2021-05-03 14:02   ` Richard Biener
  2021-05-03 14:11   ` Michael Matz
  1 sibling, 0 replies; 9+ messages in thread
From: Richard Biener @ 2021-05-03 14:02 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: gcc-patches, jakub, ebotcazou

On Mon, 3 May 2021, Jan Hubicka wrote:

> > note that if you wrap foo () into another noinline
> > wrap_foo () { foo (); return 1; } function then we need to make
> > sure to not DCE this call either even though it only throws
> > externally.  Now the question is whether this testcase is valid
> > (it should not abort).  The documentation of 'pure' must be read
> > as that 'pure' is not valid for 'foo' since throwing an exception
> > is obviously an observable effect on the state of the program
> > (in particular for the testcase at hand).  But for example
> > IPA pure const does not cause us to infer nothrow on pure
> > declared functions and the C++ program
> > 
> > extern int __attribute__((pure)) foo();
> > int bar()
> > {
> >   try
> >     {
> >       return foo ();
> >     }
> >   catch (...)
> >     {
> >       return -1;
> >     }
> > }
> > 
> > is compiled with full EH in place. (clang elides all of it, btw)
> > 
> > The set of changes below do not succeed in it passing but at least
> > the throwing call prevails but somehow EH info is hosed and you'll
> > get
> > 
> > > ./pr100382.exe
> > terminate called without an active exception
> > Aborted (core dumped)
> > 
> > So - what is it?  Are pure functions allowed to throw or not?
> 
> I was one adding pure functions and it was really intended to be same
> thing as const+reading memory.  So does const function throw or not?
> Internally we definitly want to make this separate - with symbol
> summaries it is now reasonably easy to do.  I was thinking of doing
> similar summary as modref handles to pass down separate info tracked by
> pure-const patch rather than trying to re-synthetize it to rather random
> attributes we have now...

Yes, we handle 'const' as throwing (well, we don't optimize it as
not throwing...).  But since 'const' is a subset of 'pure' functions
if pure functions cannot throw then const functions can not either.

extern int __attribute__((const)) foo();
int bar()
{
  try
    {
      return foo ();
    }
  catch (...)
    {
      return -1;
    }
}

has EH emitted as well.

Richard.

> Honza
> > 
> > 2021-05-03  Richard Biener  <rguenther@suse.de>
> > 
> > 	* calls.c (expand_call): Preserve possibly throwing calls.
> > 	* cfgexpand.c (expand_call_stmt): When a call can throw signal
> > 	RTL expansion there are side-effects.
> > 	* tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Simplify.
> > 	* tree-ssa-dse.c (pass_dse::execute): Preserve exceptions unless
> > 	-fdelete-dead-exceptions.
> > 
> > 	* g++.dg/tree-ssa/pr100382.C: New testcase.
> > ---
> >  gcc/calls.c                              |  1 +
> >  gcc/cfgexpand.c                          |  5 ++++-
> >  gcc/testsuite/g++.dg/tree-ssa/pr100382.C | 25 ++++++++++++++++++++++++
> >  gcc/tree-ssa-dce.c                       | 21 +++-----------------
> >  gcc/tree-ssa-dse.c                       |  3 ++-
> >  5 files changed, 35 insertions(+), 20 deletions(-)
> >  create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> > 
> > diff --git a/gcc/calls.c b/gcc/calls.c
> > index 883d08ba5f2..f3da1839dc5 100644
> > --- a/gcc/calls.c
> > +++ b/gcc/calls.c
> > @@ -3808,6 +3808,7 @@ expand_call (tree exp, rtx target, int ignore)
> >       side-effects.  */
> >    if ((flags & (ECF_CONST | ECF_PURE))
> >        && (!(flags & ECF_LOOPING_CONST_OR_PURE))
> > +      && (flags & ECF_NOTHROW)
> >        && (ignore || target == const0_rtx
> >  	  || TYPE_MODE (rettype) == VOIDmode))
> >      {
> > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> > index a6b48d3e48f..556a0b22ed6 100644
> > --- a/gcc/cfgexpand.c
> > +++ b/gcc/cfgexpand.c
> > @@ -2795,7 +2795,10 @@ expand_call_stmt (gcall *stmt)
> >        CALL_EXPR_ARG (exp, i) = arg;
> >      }
> >  
> > -  if (gimple_has_side_effects (stmt))
> > +  if (gimple_has_side_effects (stmt)
> > +      /* ???  Downstream in expand_expr_real_1 we assume that expressions
> > +	 w/o side-effects do not throw so work around this here.  */
> > +      || stmt_could_throw_p (cfun, stmt))
> >      TREE_SIDE_EFFECTS (exp) = 1;
> >  
> >    if (gimple_call_nothrow_p (stmt))
> > diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr100382.C b/gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> > new file mode 100644
> > index 00000000000..b9948d3034a
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> > @@ -0,0 +1,25 @@
> > +// { dg-do run }
> > +// { dg-options "-O2" }
> > +
> > +int x, y;
> > +int __attribute__((pure,noinline)) foo () { if (x) throw; return y; }
> > +
> > +int __attribute__((noinline)) bar()
> > +{
> > +  int a[2];
> > +  x = 1;
> > +  try {
> > +    int res = foo ();
> > +    a[0] = res;
> > +  } catch (...) {
> > +      return 0;
> > +  }
> > +  return 1;
> > +}
> > +
> > +int main()
> > +{
> > +  if (bar ())
> > +    __builtin_abort ();
> > +  return 0;
> > +}
> > diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
> > index 096cfc8721d..ff0389af36f 100644
> > --- a/gcc/tree-ssa-dce.c
> > +++ b/gcc/tree-ssa-dce.c
> > @@ -250,14 +250,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
> >  	    && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee))
> >  	  return;
> >  
> > -	/* Most, but not all function calls are required.  Function calls that
> > -	   produce no result and have no side effects (i.e. const pure
> > -	   functions) are unnecessary.  */
> > -	if (gimple_has_side_effects (stmt))
> > -	  {
> > -	    mark_stmt_necessary (stmt, true);
> > -	    return;
> > -	  }
> >  	/* IFN_GOACC_LOOP calls are necessary in that they are used to
> >  	   represent parameter (i.e. step, bound) of a lowered OpenACC
> >  	   partitioned loop.  But this kind of partitioned loop might not
> > @@ -269,8 +261,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
> >  	    mark_stmt_necessary (stmt, true);
> >  	    return;
> >  	  }
> > -	if (!gimple_call_lhs (stmt))
> > -	  return;
> >  	break;
> >        }
> >  
> > @@ -312,19 +302,14 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
> >    /* If the statement has volatile operands, it needs to be preserved.
> >       Same for statements that can alter control flow in unpredictable
> >       ways.  */
> > -  if (gimple_has_volatile_ops (stmt) || is_ctrl_altering_stmt (stmt))
> > -    {
> > -      mark_stmt_necessary (stmt, true);
> > -      return;
> > -    }
> > -
> > -  if (stmt_may_clobber_global_p (stmt))
> > +  if (gimple_has_side_effects (stmt) || is_ctrl_altering_stmt (stmt))
> >      {
> >        mark_stmt_necessary (stmt, true);
> >        return;
> >      }
> >  
> > -  if (gimple_vdef (stmt) && keep_all_vdefs_p ())
> > +  if ((gimple_vdef (stmt) && keep_all_vdefs_p ())
> > +      || stmt_may_clobber_global_p (stmt))
> >      {
> >        mark_stmt_necessary (stmt, true);
> >        return;
> > diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
> > index dfa6d314727..386667b9f7f 100644
> > --- a/gcc/tree-ssa-dse.c
> > +++ b/gcc/tree-ssa-dse.c
> > @@ -1219,7 +1219,8 @@ pass_dse::execute (function *fun)
> >  		 dead SSA defs.  */
> >  	      if (has_zero_uses (DEF_FROM_PTR (def_p))
> >  		  && !gimple_has_side_effects (stmt)
> > -		  && !stmt_unremovable_because_of_non_call_eh_p (cfun, stmt))
> > +		  && (!stmt_could_throw_p (fun, stmt)
> > +		      || fun->can_delete_dead_exceptions))
> >  		{
> >  		  if (dump_file && (dump_flags & TDF_DETAILS))
> >  		    {
> > -- 
> > 2.26.2
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Avoid DSE/DCE of pure call that throws
  2021-05-03 13:30 [PATCH] Avoid DSE/DCE of pure call that throws Richard Biener
  2021-05-03 13:34 ` Jan Hubicka
@ 2021-05-03 14:04 ` Eric Botcazou
  2021-05-03 14:08   ` Richard Biener
  1 sibling, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2021-05-03 14:04 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, jakub, jeffreyalaw

> So - what is it?  Are pure functions allowed to throw or not?

We keep going back to this every N years and I'm not sure why...  In Ada the 
answer is definitely yes, we have entire library units marked Pure and thus 
containing bunch of pure functions but they can throw like any other function.

-- 
Eric Botcazou



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Avoid DSE/DCE of pure call that throws
  2021-05-03 14:04 ` Eric Botcazou
@ 2021-05-03 14:08   ` Richard Biener
  2021-05-04  9:46     ` Eric Botcazou
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Biener @ 2021-05-03 14:08 UTC (permalink / raw)
  To: Eric Botcazou; +Cc: gcc-patches, jakub, jeffreyalaw

On Mon, 3 May 2021, Eric Botcazou wrote:

> > So - what is it?  Are pure functions allowed to throw or not?
> 
> We keep going back to this every N years and I'm not sure why...  In Ada the 
> answer is definitely yes, we have entire library units marked Pure and thus 
> containing bunch of pure functions but they can throw like any other function.

OK, so then we definitely have all the latent issues I paper over with
the patch and that isn't even enough.

Do you have extra fixes in your tree or does -fnon-call-exceptions
somehow magically paper over the issue?

I suppose if the C or C++ frontends like to have pure/const attributed
functions not throw they could mark functions accordingly themselves.

Which also means, the Ada functions are DECL_PURE_P but not 'pure'
according to the extend.texi documentation of the user-visible
attribute?  That said, I think if my C++ testcase is valid then we
should amend this documentation (or if not then as well, to not
re-iterate this every N years).  Do you agree?

Thanks,
Richard.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Avoid DSE/DCE of pure call that throws
  2021-05-03 13:34 ` Jan Hubicka
  2021-05-03 14:02   ` Richard Biener
@ 2021-05-03 14:11   ` Michael Matz
  2021-05-03 14:16     ` Richard Biener
  1 sibling, 1 reply; 9+ messages in thread
From: Michael Matz @ 2021-05-03 14:11 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Richard Biener, jakub, ebotcazou, gcc-patches

Hello,

On Mon, 3 May 2021, Jan Hubicka wrote:

> > (it should not abort).  The documentation of 'pure' must be read
> > as that 'pure' is not valid for 'foo' since throwing an exception
> > is obviously an observable effect on the state of the program
> > (in particular for the testcase at hand).  But for example
> > IPA pure const does not cause us to infer nothrow on pure
> > declared functions and the C++ program
> > 
> > ...
> > 
> > So - what is it?  Are pure functions allowed to throw or not?
> 
> I was one adding pure functions and it was really intended to be same
> thing as const+reading memory.  So does const function throw or not?

I really want to say that const/pure and throw are mutually exclusive.  
But in reality they are orthogonal, so a const function may throw.  (It 
certainly can in C++).

This is because while we implement exceptions with memory state, that 
state in not accessible to the application.  Exceptions could for instance 
also be implemented via return type extension.  And then, as long as other 
guarantees of const or pure functions are adhered to (same input -> same 
output), throwing an exception from a const function would be completely 
natural.  E.g. if a const function, given a specific set of arguments, 
always throws the same value that would still allow to CSE two calls to it 
in a row, and it would still allow to assume that no reachable memory was 
changed by that call.

So, I think const and pure functions may validly throw (but then must 
always do so given the same inputs).


Ciao,
Michael.


> Internally we definitly want to make this separate - with symbol
> summaries it is now reasonably easy to do.  I was thinking of doing
> similar summary as modref handles to pass down separate info tracked by
> pure-const patch rather than trying to re-synthetize it to rather random
> attributes we have now...
> 
> Honza
> > 
> > 2021-05-03  Richard Biener  <rguenther@suse.de>
> > 
> > 	* calls.c (expand_call): Preserve possibly throwing calls.
> > 	* cfgexpand.c (expand_call_stmt): When a call can throw signal
> > 	RTL expansion there are side-effects.
> > 	* tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Simplify.
> > 	* tree-ssa-dse.c (pass_dse::execute): Preserve exceptions unless
> > 	-fdelete-dead-exceptions.
> > 
> > 	* g++.dg/tree-ssa/pr100382.C: New testcase.
> > ---
> >  gcc/calls.c                              |  1 +
> >  gcc/cfgexpand.c                          |  5 ++++-
> >  gcc/testsuite/g++.dg/tree-ssa/pr100382.C | 25 ++++++++++++++++++++++++
> >  gcc/tree-ssa-dce.c                       | 21 +++-----------------
> >  gcc/tree-ssa-dse.c                       |  3 ++-
> >  5 files changed, 35 insertions(+), 20 deletions(-)
> >  create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> > 
> > diff --git a/gcc/calls.c b/gcc/calls.c
> > index 883d08ba5f2..f3da1839dc5 100644
> > --- a/gcc/calls.c
> > +++ b/gcc/calls.c
> > @@ -3808,6 +3808,7 @@ expand_call (tree exp, rtx target, int ignore)
> >       side-effects.  */
> >    if ((flags & (ECF_CONST | ECF_PURE))
> >        && (!(flags & ECF_LOOPING_CONST_OR_PURE))
> > +      && (flags & ECF_NOTHROW)
> >        && (ignore || target == const0_rtx
> >  	  || TYPE_MODE (rettype) == VOIDmode))
> >      {
> > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> > index a6b48d3e48f..556a0b22ed6 100644
> > --- a/gcc/cfgexpand.c
> > +++ b/gcc/cfgexpand.c
> > @@ -2795,7 +2795,10 @@ expand_call_stmt (gcall *stmt)
> >        CALL_EXPR_ARG (exp, i) = arg;
> >      }
> >  
> > -  if (gimple_has_side_effects (stmt))
> > +  if (gimple_has_side_effects (stmt)
> > +      /* ???  Downstream in expand_expr_real_1 we assume that expressions
> > +	 w/o side-effects do not throw so work around this here.  */
> > +      || stmt_could_throw_p (cfun, stmt))
> >      TREE_SIDE_EFFECTS (exp) = 1;
> >  
> >    if (gimple_call_nothrow_p (stmt))
> > diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr100382.C b/gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> > new file mode 100644
> > index 00000000000..b9948d3034a
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> > @@ -0,0 +1,25 @@
> > +// { dg-do run }
> > +// { dg-options "-O2" }
> > +
> > +int x, y;
> > +int __attribute__((pure,noinline)) foo () { if (x) throw; return y; }
> > +
> > +int __attribute__((noinline)) bar()
> > +{
> > +  int a[2];
> > +  x = 1;
> > +  try {
> > +    int res = foo ();
> > +    a[0] = res;
> > +  } catch (...) {
> > +      return 0;
> > +  }
> > +  return 1;
> > +}
> > +
> > +int main()
> > +{
> > +  if (bar ())
> > +    __builtin_abort ();
> > +  return 0;
> > +}
> > diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
> > index 096cfc8721d..ff0389af36f 100644
> > --- a/gcc/tree-ssa-dce.c
> > +++ b/gcc/tree-ssa-dce.c
> > @@ -250,14 +250,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
> >  	    && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee))
> >  	  return;
> >  
> > -	/* Most, but not all function calls are required.  Function calls that
> > -	   produce no result and have no side effects (i.e. const pure
> > -	   functions) are unnecessary.  */
> > -	if (gimple_has_side_effects (stmt))
> > -	  {
> > -	    mark_stmt_necessary (stmt, true);
> > -	    return;
> > -	  }
> >  	/* IFN_GOACC_LOOP calls are necessary in that they are used to
> >  	   represent parameter (i.e. step, bound) of a lowered OpenACC
> >  	   partitioned loop.  But this kind of partitioned loop might not
> > @@ -269,8 +261,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
> >  	    mark_stmt_necessary (stmt, true);
> >  	    return;
> >  	  }
> > -	if (!gimple_call_lhs (stmt))
> > -	  return;
> >  	break;
> >        }
> >  
> > @@ -312,19 +302,14 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
> >    /* If the statement has volatile operands, it needs to be preserved.
> >       Same for statements that can alter control flow in unpredictable
> >       ways.  */
> > -  if (gimple_has_volatile_ops (stmt) || is_ctrl_altering_stmt (stmt))
> > -    {
> > -      mark_stmt_necessary (stmt, true);
> > -      return;
> > -    }
> > -
> > -  if (stmt_may_clobber_global_p (stmt))
> > +  if (gimple_has_side_effects (stmt) || is_ctrl_altering_stmt (stmt))
> >      {
> >        mark_stmt_necessary (stmt, true);
> >        return;
> >      }
> >  
> > -  if (gimple_vdef (stmt) && keep_all_vdefs_p ())
> > +  if ((gimple_vdef (stmt) && keep_all_vdefs_p ())
> > +      || stmt_may_clobber_global_p (stmt))
> >      {
> >        mark_stmt_necessary (stmt, true);
> >        return;
> > diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
> > index dfa6d314727..386667b9f7f 100644
> > --- a/gcc/tree-ssa-dse.c
> > +++ b/gcc/tree-ssa-dse.c
> > @@ -1219,7 +1219,8 @@ pass_dse::execute (function *fun)
> >  		 dead SSA defs.  */
> >  	      if (has_zero_uses (DEF_FROM_PTR (def_p))
> >  		  && !gimple_has_side_effects (stmt)
> > -		  && !stmt_unremovable_because_of_non_call_eh_p (cfun, stmt))
> > +		  && (!stmt_could_throw_p (fun, stmt)
> > +		      || fun->can_delete_dead_exceptions))
> >  		{
> >  		  if (dump_file && (dump_flags & TDF_DETAILS))
> >  		    {
> > -- 
> > 2.26.2
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Avoid DSE/DCE of pure call that throws
  2021-05-03 14:11   ` Michael Matz
@ 2021-05-03 14:16     ` Richard Biener
  0 siblings, 0 replies; 9+ messages in thread
From: Richard Biener @ 2021-05-03 14:16 UTC (permalink / raw)
  To: Michael Matz; +Cc: Jan Hubicka, jakub, ebotcazou, gcc-patches

On Mon, 3 May 2021, Michael Matz wrote:

> Hello,
> 
> On Mon, 3 May 2021, Jan Hubicka wrote:
> 
> > > (it should not abort).  The documentation of 'pure' must be read
> > > as that 'pure' is not valid for 'foo' since throwing an exception
> > > is obviously an observable effect on the state of the program
> > > (in particular for the testcase at hand).  But for example
> > > IPA pure const does not cause us to infer nothrow on pure
> > > declared functions and the C++ program
> > > 
> > > ...
> > > 
> > > So - what is it?  Are pure functions allowed to throw or not?
> > 
> > I was one adding pure functions and it was really intended to be same
> > thing as const+reading memory.  So does const function throw or not?
> 
> I really want to say that const/pure and throw are mutually exclusive.  
> But in reality they are orthogonal, so a const function may throw.  (It 
> certainly can in C++).
> 
> This is because while we implement exceptions with memory state, that 
> state in not accessible to the application.  Exceptions could for instance 
> also be implemented via return type extension.  And then, as long as other 
> guarantees of const or pure functions are adhered to (same input -> same 
> output), throwing an exception from a const function would be completely 
> natural.  E.g. if a const function, given a specific set of arguments, 
> always throws the same value that would still allow to CSE two calls to it 
> in a row, and it would still allow to assume that no reachable memory was 
> changed by that call.
> 
> So, I think const and pure functions may validly throw (but then must 
> always do so given the same inputs).

I've filed PR100394 since it appearantly never worked (for C++).

Richard.

> 
> Ciao,
> Michael.
> 
> 
> > Internally we definitly want to make this separate - with symbol
> > summaries it is now reasonably easy to do.  I was thinking of doing
> > similar summary as modref handles to pass down separate info tracked by
> > pure-const patch rather than trying to re-synthetize it to rather random
> > attributes we have now...
> > 
> > Honza
> > > 
> > > 2021-05-03  Richard Biener  <rguenther@suse.de>
> > > 
> > > 	* calls.c (expand_call): Preserve possibly throwing calls.
> > > 	* cfgexpand.c (expand_call_stmt): When a call can throw signal
> > > 	RTL expansion there are side-effects.
> > > 	* tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Simplify.
> > > 	* tree-ssa-dse.c (pass_dse::execute): Preserve exceptions unless
> > > 	-fdelete-dead-exceptions.
> > > 
> > > 	* g++.dg/tree-ssa/pr100382.C: New testcase.
> > > ---
> > >  gcc/calls.c                              |  1 +
> > >  gcc/cfgexpand.c                          |  5 ++++-
> > >  gcc/testsuite/g++.dg/tree-ssa/pr100382.C | 25 ++++++++++++++++++++++++
> > >  gcc/tree-ssa-dce.c                       | 21 +++-----------------
> > >  gcc/tree-ssa-dse.c                       |  3 ++-
> > >  5 files changed, 35 insertions(+), 20 deletions(-)
> > >  create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> > > 
> > > diff --git a/gcc/calls.c b/gcc/calls.c
> > > index 883d08ba5f2..f3da1839dc5 100644
> > > --- a/gcc/calls.c
> > > +++ b/gcc/calls.c
> > > @@ -3808,6 +3808,7 @@ expand_call (tree exp, rtx target, int ignore)
> > >       side-effects.  */
> > >    if ((flags & (ECF_CONST | ECF_PURE))
> > >        && (!(flags & ECF_LOOPING_CONST_OR_PURE))
> > > +      && (flags & ECF_NOTHROW)
> > >        && (ignore || target == const0_rtx
> > >  	  || TYPE_MODE (rettype) == VOIDmode))
> > >      {
> > > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> > > index a6b48d3e48f..556a0b22ed6 100644
> > > --- a/gcc/cfgexpand.c
> > > +++ b/gcc/cfgexpand.c
> > > @@ -2795,7 +2795,10 @@ expand_call_stmt (gcall *stmt)
> > >        CALL_EXPR_ARG (exp, i) = arg;
> > >      }
> > >  
> > > -  if (gimple_has_side_effects (stmt))
> > > +  if (gimple_has_side_effects (stmt)
> > > +      /* ???  Downstream in expand_expr_real_1 we assume that expressions
> > > +	 w/o side-effects do not throw so work around this here.  */
> > > +      || stmt_could_throw_p (cfun, stmt))
> > >      TREE_SIDE_EFFECTS (exp) = 1;
> > >  
> > >    if (gimple_call_nothrow_p (stmt))
> > > diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr100382.C b/gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> > > new file mode 100644
> > > index 00000000000..b9948d3034a
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/tree-ssa/pr100382.C
> > > @@ -0,0 +1,25 @@
> > > +// { dg-do run }
> > > +// { dg-options "-O2" }
> > > +
> > > +int x, y;
> > > +int __attribute__((pure,noinline)) foo () { if (x) throw; return y; }
> > > +
> > > +int __attribute__((noinline)) bar()
> > > +{
> > > +  int a[2];
> > > +  x = 1;
> > > +  try {
> > > +    int res = foo ();
> > > +    a[0] = res;
> > > +  } catch (...) {
> > > +      return 0;
> > > +  }
> > > +  return 1;
> > > +}
> > > +
> > > +int main()
> > > +{
> > > +  if (bar ())
> > > +    __builtin_abort ();
> > > +  return 0;
> > > +}
> > > diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
> > > index 096cfc8721d..ff0389af36f 100644
> > > --- a/gcc/tree-ssa-dce.c
> > > +++ b/gcc/tree-ssa-dce.c
> > > @@ -250,14 +250,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
> > >  	    && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee))
> > >  	  return;
> > >  
> > > -	/* Most, but not all function calls are required.  Function calls that
> > > -	   produce no result and have no side effects (i.e. const pure
> > > -	   functions) are unnecessary.  */
> > > -	if (gimple_has_side_effects (stmt))
> > > -	  {
> > > -	    mark_stmt_necessary (stmt, true);
> > > -	    return;
> > > -	  }
> > >  	/* IFN_GOACC_LOOP calls are necessary in that they are used to
> > >  	   represent parameter (i.e. step, bound) of a lowered OpenACC
> > >  	   partitioned loop.  But this kind of partitioned loop might not
> > > @@ -269,8 +261,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
> > >  	    mark_stmt_necessary (stmt, true);
> > >  	    return;
> > >  	  }
> > > -	if (!gimple_call_lhs (stmt))
> > > -	  return;
> > >  	break;
> > >        }
> > >  
> > > @@ -312,19 +302,14 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
> > >    /* If the statement has volatile operands, it needs to be preserved.
> > >       Same for statements that can alter control flow in unpredictable
> > >       ways.  */
> > > -  if (gimple_has_volatile_ops (stmt) || is_ctrl_altering_stmt (stmt))
> > > -    {
> > > -      mark_stmt_necessary (stmt, true);
> > > -      return;
> > > -    }
> > > -
> > > -  if (stmt_may_clobber_global_p (stmt))
> > > +  if (gimple_has_side_effects (stmt) || is_ctrl_altering_stmt (stmt))
> > >      {
> > >        mark_stmt_necessary (stmt, true);
> > >        return;
> > >      }
> > >  
> > > -  if (gimple_vdef (stmt) && keep_all_vdefs_p ())
> > > +  if ((gimple_vdef (stmt) && keep_all_vdefs_p ())
> > > +      || stmt_may_clobber_global_p (stmt))
> > >      {
> > >        mark_stmt_necessary (stmt, true);
> > >        return;
> > > diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
> > > index dfa6d314727..386667b9f7f 100644
> > > --- a/gcc/tree-ssa-dse.c
> > > +++ b/gcc/tree-ssa-dse.c
> > > @@ -1219,7 +1219,8 @@ pass_dse::execute (function *fun)
> > >  		 dead SSA defs.  */
> > >  	      if (has_zero_uses (DEF_FROM_PTR (def_p))
> > >  		  && !gimple_has_side_effects (stmt)
> > > -		  && !stmt_unremovable_because_of_non_call_eh_p (cfun, stmt))
> > > +		  && (!stmt_could_throw_p (fun, stmt)
> > > +		      || fun->can_delete_dead_exceptions))
> > >  		{
> > >  		  if (dump_file && (dump_flags & TDF_DETAILS))
> > >  		    {
> > > -- 
> > > 2.26.2
> > 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Avoid DSE/DCE of pure call that throws
  2021-05-03 14:08   ` Richard Biener
@ 2021-05-04  9:46     ` Eric Botcazou
  2021-05-04 10:42       ` Richard Biener
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2021-05-04  9:46 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, jakub, jeffreyalaw

> Do you have extra fixes in your tree or does -fnon-call-exceptions
> somehow magically paper over the issue?

This optimization is valid in Ada for pure functions.  RM 10.2.1(18/3) says:
"If a library unit is declared pure, then the implementation is permitted to 
omit a call on a library-level subprogram of the library unit if the results 
are not needed after the call.(...)[This permission applies even if the 
subprogram produces other side effects when called.]".

> I suppose if the C or C++ frontends like to have pure/const attributed
> functions not throw they could mark functions accordingly themselves.
> 
> Which also means, the Ada functions are DECL_PURE_P but not 'pure'
> according to the extend.texi documentation of the user-visible
> attribute?  That said, I think if my C++ testcase is valid then we
> should amend this documentation (or if not then as well, to not
> re-iterate this every N years).  Do you agree?

Yes, the documentation was written without considering other languages with 
exception handling, but it's originally an extension of the C language and 
documented in the manual of the C compiler, so that's not very surprising.

Pure Ada functions are "const" in the GNU C sense if all their parameters are 
passed by copy and "pure" in the GNU  C sense if their parameters not passed 
by value (i.e. by reference) are In parameters; in all the other cases, pure 
Ada functions are neither "const" nor "pure" in the GNU C sense.

I think that we need to add an explicit sentence about exception handling to 
the declaration of DECL_PURE_P:

/* Nonzero in a FUNCTION_DECL means this function should be treated
   as "pure" function (like const function, but may read global memory).  */
#define DECL_PURE_P(NODE) (FUNCTION_DECL_CHECK (NODE)>function_decl.pure_flag)

Maybe: "Note that being pure or const for a function is orthogonal to being 
no-throw, i.e. it is valid to have DECL_PURE_P set and TREE_NOTHROW cleared".

-- 
Eric Botcazou



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Avoid DSE/DCE of pure call that throws
  2021-05-04  9:46     ` Eric Botcazou
@ 2021-05-04 10:42       ` Richard Biener
  0 siblings, 0 replies; 9+ messages in thread
From: Richard Biener @ 2021-05-04 10:42 UTC (permalink / raw)
  To: Eric Botcazou; +Cc: gcc-patches, jakub, jeffreyalaw

On Tue, 4 May 2021, Eric Botcazou wrote:

> > Do you have extra fixes in your tree or does -fnon-call-exceptions
> > somehow magically paper over the issue?
> 
> This optimization is valid in Ada for pure functions.  RM 10.2.1(18/3) says:
> "If a library unit is declared pure, then the implementation is permitted to 
> omit a call on a library-level subprogram of the library unit if the results 
> are not needed after the call.(...)[This permission applies even if the 
> subprogram produces other side effects when called.]".
> 
> > I suppose if the C or C++ frontends like to have pure/const attributed
> > functions not throw they could mark functions accordingly themselves.
> > 
> > Which also means, the Ada functions are DECL_PURE_P but not 'pure'
> > according to the extend.texi documentation of the user-visible
> > attribute?  That said, I think if my C++ testcase is valid then we
> > should amend this documentation (or if not then as well, to not
> > re-iterate this every N years).  Do you agree?
> 
> Yes, the documentation was written without considering other languages with 
> exception handling, but it's originally an extension of the C language and 
> documented in the manual of the C compiler, so that's not very surprising.
> 
> Pure Ada functions are "const" in the GNU C sense if all their parameters are 
> passed by copy and "pure" in the GNU  C sense if their parameters not passed 
> by value (i.e. by reference) are In parameters; in all the other cases, pure 
> Ada functions are neither "const" nor "pure" in the GNU C sense.
> 
> I think that we need to add an explicit sentence about exception handling to 
> the declaration of DECL_PURE_P:
> 
> /* Nonzero in a FUNCTION_DECL means this function should be treated
>    as "pure" function (like const function, but may read global memory).  */
> #define DECL_PURE_P(NODE) (FUNCTION_DECL_CHECK (NODE)>function_decl.pure_flag)
> 
> Maybe: "Note that being pure or const for a function is orthogonal to being 
> no-throw, i.e. it is valid to have DECL_PURE_P set and TREE_NOTHROW cleared".

Good idea.

So the reason it doesn't "break" with Ada is because with Ada we are
allowed to elide the EH.

I'm testing the revised patch below which also prepares for a
PR100409 fix in the C++ FE (the DCE hunk).  I hope any Ada fallout
is catched by the testsuite.

Better ideas for the expand_call / expand_call_stmt kludges appreciated.

Richard.


middle-end/100394 - avoid DSE/DCE of pure call that throws

There is -fdelete-dead-exceptions now and we're tracking
nothrow and const/pure bits separately and I do remember that
const or pure does _not_ imply nothrow.

Now, in the light of the PR100382 fix which added a
stmt_unremovable_because_of_non_call_eh_p guard to DSEs "DCE"
I wondered how -fdelete-dead-exceptions applies to calls and
whether stmt_unremovable_because_of_non_call_eh_p doing

  return (fun->can_throw_non_call_exceptions
          && !fun->can_delete_dead_exceptions
          && stmt_could_throw_p (fun, stmt));

really should conditionalize itself on
fun->can_throw_non_call_exceptions.  In fact DCE happily elides
pure function calls that throw without a LHS (probably a
consistency bug).  The following testcase shows this:

int x, y;
int __attribute__((pure,noinline)) foo () { if (x) throw 1; return y; }

int main()
{
  int a[2];
  x = 1;
  try {
    int res = foo ();
    a[0] = res;
  } catch (...) {
      return 0;
  }
  return 1;
}

note that if you wrap foo () into another noinline
wrap_foo () { foo (); return 1; } function then we need to make
sure to not DCE this call either even though it only throws
externally.

2021-05-03  Richard Biener  <rguenther@suse.de>

	PR middle-end/100394
	* calls.c (expand_call): Preserve possibly throwing calls.
	* cfgexpand.c (expand_call_stmt): When a call can throw signal
	RTL expansion there are side-effects.
	* tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Simplify,
	mark all possibly throwing stmts necessary unless we can elide
	dead EH.
	* tree-ssa-dse.c (pass_dse::execute): Preserve exceptions unless
	-fdelete-dead-exceptions.
	* tree.h (DECL_PURE_P): Add note about exceptions.

	* g++.dg/torture/pr100382.C: New testcase.
---
 gcc/calls.c                             |  1 +
 gcc/cfgexpand.c                         |  5 ++++-
 gcc/testsuite/g++.dg/torture/pr100382.C | 24 ++++++++++++++++++++
 gcc/tree-ssa-dce.c                      | 29 +++++++------------------
 gcc/tree-ssa-dse.c                      |  3 ++-
 gcc/tree.h                              |  5 ++++-
 6 files changed, 43 insertions(+), 24 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/torture/pr100382.C

diff --git a/gcc/calls.c b/gcc/calls.c
index 883d08ba5f2..f3da1839dc5 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -3808,6 +3808,7 @@ expand_call (tree exp, rtx target, int ignore)
      side-effects.  */
   if ((flags & (ECF_CONST | ECF_PURE))
       && (!(flags & ECF_LOOPING_CONST_OR_PURE))
+      && (flags & ECF_NOTHROW)
       && (ignore || target == const0_rtx
 	  || TYPE_MODE (rettype) == VOIDmode))
     {
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index a6b48d3e48f..556a0b22ed6 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2795,7 +2795,10 @@ expand_call_stmt (gcall *stmt)
       CALL_EXPR_ARG (exp, i) = arg;
     }
 
-  if (gimple_has_side_effects (stmt))
+  if (gimple_has_side_effects (stmt)
+      /* ???  Downstream in expand_expr_real_1 we assume that expressions
+	 w/o side-effects do not throw so work around this here.  */
+      || stmt_could_throw_p (cfun, stmt))
     TREE_SIDE_EFFECTS (exp) = 1;
 
   if (gimple_call_nothrow_p (stmt))
diff --git a/gcc/testsuite/g++.dg/torture/pr100382.C b/gcc/testsuite/g++.dg/torture/pr100382.C
new file mode 100644
index 00000000000..ffc4182cfce
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr100382.C
@@ -0,0 +1,24 @@
+// { dg-do run }
+
+int x, y;
+int __attribute__((pure,noinline)) foo () { if (x) throw 1; return y; }
+
+int __attribute__((noinline)) bar()
+{
+  int a[2];
+  x = 1;
+  try {
+    int res = foo ();
+    a[0] = res;
+  } catch (...) {
+      return 0;
+  }
+  return 1;
+}
+
+int main()
+{
+  if (bar ())
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index 096cfc8721d..c091868a313 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -199,14 +199,6 @@ mark_operand_necessary (tree op)
 static void
 mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
 {
-  /* With non-call exceptions, we have to assume that all statements could
-     throw.  If a statement could throw, it can be deemed necessary.  */
-  if (stmt_unremovable_because_of_non_call_eh_p (cfun, stmt))
-    {
-      mark_stmt_necessary (stmt, true);
-      return;
-    }
-
   /* Statements that are implicitly live.  Most function calls, asm
      and return statements are required.  Labels and GIMPLE_BIND nodes
      are kept because they are control flow, and we have no way of
@@ -250,14 +242,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
 	    && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee))
 	  return;
 
-	/* Most, but not all function calls are required.  Function calls that
-	   produce no result and have no side effects (i.e. const pure
-	   functions) are unnecessary.  */
-	if (gimple_has_side_effects (stmt))
-	  {
-	    mark_stmt_necessary (stmt, true);
-	    return;
-	  }
 	/* IFN_GOACC_LOOP calls are necessary in that they are used to
 	   represent parameter (i.e. step, bound) of a lowered OpenACC
 	   partitioned loop.  But this kind of partitioned loop might not
@@ -269,8 +253,6 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
 	    mark_stmt_necessary (stmt, true);
 	    return;
 	  }
-	if (!gimple_call_lhs (stmt))
-	  return;
 	break;
       }
 
@@ -312,19 +294,24 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
   /* If the statement has volatile operands, it needs to be preserved.
      Same for statements that can alter control flow in unpredictable
      ways.  */
-  if (gimple_has_volatile_ops (stmt) || is_ctrl_altering_stmt (stmt))
+  if (gimple_has_side_effects (stmt) || is_ctrl_altering_stmt (stmt))
     {
       mark_stmt_necessary (stmt, true);
       return;
     }
 
-  if (stmt_may_clobber_global_p (stmt))
+  /* If a statement could throw, it can be deemed necessary unless we
+     are allowed to remove dead EH.  Test this after checking for
+     new/delete operators since we always elide their EH.  */
+  if (!cfun->can_delete_dead_exceptions
+      && stmt_could_throw_p (cfun, stmt))
     {
       mark_stmt_necessary (stmt, true);
       return;
     }
 
-  if (gimple_vdef (stmt) && keep_all_vdefs_p ())
+  if ((gimple_vdef (stmt) && keep_all_vdefs_p ())
+      || stmt_may_clobber_global_p (stmt))
     {
       mark_stmt_necessary (stmt, true);
       return;
diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
index d7cf7477028..63c876a1ff2 100644
--- a/gcc/tree-ssa-dse.c
+++ b/gcc/tree-ssa-dse.c
@@ -1220,7 +1220,8 @@ pass_dse::execute (function *fun)
 	      if (has_zero_uses (DEF_FROM_PTR (def_p))
 		  && !gimple_has_side_effects (stmt)
 		  && !is_ctrl_altering_stmt (stmt)
-		  && !stmt_unremovable_because_of_non_call_eh_p (cfun, stmt))
+		  && (!stmt_could_throw_p (fun, stmt)
+		      || fun->can_delete_dead_exceptions))
 		{
 		  if (dump_file && (dump_flags & TDF_DETAILS))
 		    {
diff --git a/gcc/tree.h b/gcc/tree.h
index 5a609b98b4c..6d3cfc4c588 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -3133,7 +3133,10 @@ set_function_decl_type (tree decl, function_decl_type t, bool set)
   (FUNCTION_DECL_CHECK (NODE)->function_decl.returns_twice_flag)
 
 /* Nonzero in a FUNCTION_DECL means this function should be treated
-   as "pure" function (like const function, but may read global memory).  */
+   as "pure" function (like const function, but may read global memory).
+   Note that being pure or const for a function is orthogonal to being
+   nothrow, i.e. it is valid to have DECL_PURE_P set and TREE_NOTHROW
+   cleared.  */
 #define DECL_PURE_P(NODE) (FUNCTION_DECL_CHECK (NODE)->function_decl.pure_flag)
 
 /* Nonzero only if one of TREE_READONLY or DECL_PURE_P is nonzero AND
-- 
2.26.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-05-04 10:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-03 13:30 [PATCH] Avoid DSE/DCE of pure call that throws Richard Biener
2021-05-03 13:34 ` Jan Hubicka
2021-05-03 14:02   ` Richard Biener
2021-05-03 14:11   ` Michael Matz
2021-05-03 14:16     ` Richard Biener
2021-05-03 14:04 ` Eric Botcazou
2021-05-03 14:08   ` Richard Biener
2021-05-04  9:46     ` Eric Botcazou
2021-05-04 10:42       ` Richard Biener

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