middle-end/106075 - non-call EH and DSE

  The following fixes a long-standing bug with DSE removing stores as
dead even though they are live across non-call exceptional flow.
This affects both GIMPLE and RTL DSE and the fix is similar in
making externally throwing statements uses of non-local stores.
Note this doesn't fix the GIMPLE side when the throwing statement
does not involve a load or a store because then the statement does
not have virtual operands and thus is not visited by GIMPLE DSE.

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

This doesn't seem to be a regression and I'm unsure as to how
important it is for Ada (I consider it not important for C/C++),
so for now I'll queue it for next stage1.

	PR middle-end/106075
	* dse.cc (scan_insn): Consider externally throwing insns
	to read from not frame based memory.
	* tree-ssa-dse.cc (dse_classify_store): Consider externally
	throwing uses to read from global memory.

	* gcc.dg/torture/pr106075-1.c: New testcase.
---
 gcc/dse.cc                                |  5 ++++
 gcc/testsuite/gcc.dg/torture/pr106075-1.c | 36 +++++++++++++++++++++++
 gcc/tree-ssa-dse.cc                       |  8 ++++-
 3 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr106075-1.c
  

> The following fixes a long-standing bug with DSE removing stores as
> dead even though they are live across non-call exceptional flow.
> This affects both GIMPLE and RTL DSE and the fix is similar in
> making externally throwing statements uses of non-local stores.
> Note this doesn't fix the GIMPLE side when the throwing statement
> does not involve a load or a store because then the statement does
> not have virtual operands and thus is not visited by GIMPLE DSE.

Thanks for looking into this.
My main motivation for poking on this is the patch to add fnspec to
throw/catch machinery
https://gcc.gnu.org/pipermail/gcc-patches/2022-June/597124.html

The eh6.C testcase is misoptimized by current trun with the patch.
I think I can adjust it for the throwing function to have no vops and it
will still get misoptimized by DSE.
> 
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> 
> This doesn't seem to be a regression and I'm unsure as to how
> important it is for Ada (I consider it not important for C/C++),
> so for now I'll queue it for next stage1.

According to compiler explorer testcase:
struct a{int a,b,c,d,e;};
void
test(struct a * __restrict a, struct a *b)
{
  *a = (struct a){0,1,2,3,4};
  *a = *b;
}
Is compiled correctly by GCC 5.4 and first miscopmiled by 6.1, so I
think it is a regression. (For C++ not very important one as
-fnon-call-exceptions is not very common for C++)

Honza
> 
> 	PR middle-end/106075
> 	* dse.cc (scan_insn): Consider externally throwing insns
> 	to read from not frame based memory.
> 	* tree-ssa-dse.cc (dse_classify_store): Consider externally
> 	throwing uses to read from global memory.
> 
> 	* gcc.dg/torture/pr106075-1.c: New testcase.
> ---
>  gcc/dse.cc                                |  5 ++++
>  gcc/testsuite/gcc.dg/torture/pr106075-1.c | 36 +++++++++++++++++++++++
>  gcc/tree-ssa-dse.cc                       |  8 ++++-
>  3 files changed, 48 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.dg/torture/pr106075-1.c
> 
> diff --git a/gcc/dse.cc b/gcc/dse.cc
> index a2db8d1cc32..7e258b81f66 100644
> --- a/gcc/dse.cc
> +++ b/gcc/dse.cc
> @@ -2633,6 +2633,11 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
>        return;
>      }
>  
> +  /* An externally throwing statement may read any memory that is not
> +     relative to the frame.  */
> +  if (can_throw_external (insn))
> +    add_non_frame_wild_read (bb_info);
> +
>    /* Assuming that there are sets in these insns, we cannot delete
>       them.  */
>    if ((GET_CODE (PATTERN (insn)) == CLOBBER)
> diff --git a/gcc/testsuite/gcc.dg/torture/pr106075-1.c b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> new file mode 100644
> index 00000000000..b9affbf1082
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> @@ -0,0 +1,36 @@
> +/* { dg-do run { target *-*-linux* } } */
> +/* { dg-additional-options "-fnon-call-exceptions" } */
> +
> +#include <unistd.h>
> +#include <signal.h>
> +#include <stdlib.h>
> +
> +int a = 1;
> +short *b;
> +void __attribute__((noipa))
> +test()
> +{
> +  a=12345;
> +  *b=0;
> +  a=1;
> +}
> +
> +void check (int i)
> +{
> +  if (a != 12345)
> +    abort ();
> +  exit (0);
> +}
> +
> +int
> +main ()
> +{
> +  struct sigaction s;
> +  sigemptyset (&s.sa_mask);
> +  s.sa_handler = check;
> +  s.sa_flags = 0;
> +  sigaction (SIGSEGV, &s, NULL);
> +  test();
> +  abort ();
> +  return 0;
> +}
> diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc
> index 46ab57d5754..b2e2359c3da 100644
> --- a/gcc/tree-ssa-dse.cc
> +++ b/gcc/tree-ssa-dse.cc
> @@ -960,6 +960,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
>    auto_bitmap visited;
>    std::unique_ptr<data_reference, void(*)(data_reference_p)>
>      dra (nullptr, free_data_ref);
> +  bool maybe_global = ref_may_alias_global_p (ref, false);
>  
>    if (by_clobber_p)
>      *by_clobber_p = true;
> @@ -1038,6 +1039,11 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
>  		      last_phi_def = as_a <gphi *> (use_stmt);
>  		    }
>  		}
> +	      /* If the stmt can throw externally and the store is
> +		 visible in the context unwound to the store is live.  */
> +	      else if (maybe_global
> +		       && stmt_can_throw_external (cfun, use_stmt))
> +		return DSE_STORE_LIVE;
>  	      /* If the statement is a use the store is not dead.  */
>  	      else if (ref_maybe_used_by_stmt_p (use_stmt, ref))
>  		{
> @@ -1116,7 +1122,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
>  	 just pretend the stmt makes itself dead.  Otherwise fail.  */
>        if (defs.is_empty ())
>  	{
> -	  if (ref_may_alias_global_p (ref, false))
> +	  if (maybe_global)
>  	    return DSE_STORE_LIVE;
>  
>  	  if (by_clobber_p)
> -- 
> 2.35.3
  

On Tue, 17 Jan 2023, Jan Hubicka wrote:

> > The following fixes a long-standing bug with DSE removing stores as
> > dead even though they are live across non-call exceptional flow.
> > This affects both GIMPLE and RTL DSE and the fix is similar in
> > making externally throwing statements uses of non-local stores.
> > Note this doesn't fix the GIMPLE side when the throwing statement
> > does not involve a load or a store because then the statement does
> > not have virtual operands and thus is not visited by GIMPLE DSE.
> 
> Thanks for looking into this.
> My main motivation for poking on this is the patch to add fnspec to
> throw/catch machinery
> https://gcc.gnu.org/pipermail/gcc-patches/2022-June/597124.html
> 
> The eh6.C testcase is misoptimized by current trun with the patch.
> I think I can adjust it for the throwing function to have no vops and it
> will still get misoptimized by DSE.

I think conceptually the "throw" may not be 'const' - it has to be
'pure' at most since the program point the control is transfered to
can inspect memory.

There should be no other changes necessary to DSE for cxa_throw to
be 'pure'.

> > 
> > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> > 
> > This doesn't seem to be a regression and I'm unsure as to how
> > important it is for Ada (I consider it not important for C/C++),
> > so for now I'll queue it for next stage1.
> 
> According to compiler explorer testcase:
> struct a{int a,b,c,d,e;};
> void
> test(struct a * __restrict a, struct a *b)
> {
>   *a = (struct a){0,1,2,3,4};
>   *a = *b;
> }
> Is compiled correctly by GCC 5.4 and first miscopmiled by 6.1, so I
> think it is a regression. (For C++ not very important one as
> -fnon-call-exceptions is not very common for C++)

Ah, yes - RTL DSE probably is too weak for this and GIMPLE DSE
didn't handle aggregates well at some point.

Richard.

> 
> Honza
> > 
> > 	PR middle-end/106075
> > 	* dse.cc (scan_insn): Consider externally throwing insns
> > 	to read from not frame based memory.
> > 	* tree-ssa-dse.cc (dse_classify_store): Consider externally
> > 	throwing uses to read from global memory.
> > 
> > 	* gcc.dg/torture/pr106075-1.c: New testcase.
> > ---
> >  gcc/dse.cc                                |  5 ++++
> >  gcc/testsuite/gcc.dg/torture/pr106075-1.c | 36 +++++++++++++++++++++++
> >  gcc/tree-ssa-dse.cc                       |  8 ++++-
> >  3 files changed, 48 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > 
> > diff --git a/gcc/dse.cc b/gcc/dse.cc
> > index a2db8d1cc32..7e258b81f66 100644
> > --- a/gcc/dse.cc
> > +++ b/gcc/dse.cc
> > @@ -2633,6 +2633,11 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
> >        return;
> >      }
> >  
> > +  /* An externally throwing statement may read any memory that is not
> > +     relative to the frame.  */
> > +  if (can_throw_external (insn))
> > +    add_non_frame_wild_read (bb_info);
> > +
> >    /* Assuming that there are sets in these insns, we cannot delete
> >       them.  */
> >    if ((GET_CODE (PATTERN (insn)) == CLOBBER)
> > diff --git a/gcc/testsuite/gcc.dg/torture/pr106075-1.c b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > new file mode 100644
> > index 00000000000..b9affbf1082
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > @@ -0,0 +1,36 @@
> > +/* { dg-do run { target *-*-linux* } } */
> > +/* { dg-additional-options "-fnon-call-exceptions" } */
> > +
> > +#include <unistd.h>
> > +#include <signal.h>
> > +#include <stdlib.h>
> > +
> > +int a = 1;
> > +short *b;
> > +void __attribute__((noipa))
> > +test()
> > +{
> > +  a=12345;
> > +  *b=0;
> > +  a=1;
> > +}
> > +
> > +void check (int i)
> > +{
> > +  if (a != 12345)
> > +    abort ();
> > +  exit (0);
> > +}
> > +
> > +int
> > +main ()
> > +{
> > +  struct sigaction s;
> > +  sigemptyset (&s.sa_mask);
> > +  s.sa_handler = check;
> > +  s.sa_flags = 0;
> > +  sigaction (SIGSEGV, &s, NULL);
> > +  test();
> > +  abort ();
> > +  return 0;
> > +}
> > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc
> > index 46ab57d5754..b2e2359c3da 100644
> > --- a/gcc/tree-ssa-dse.cc
> > +++ b/gcc/tree-ssa-dse.cc
> > @@ -960,6 +960,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> >    auto_bitmap visited;
> >    std::unique_ptr<data_reference, void(*)(data_reference_p)>
> >      dra (nullptr, free_data_ref);
> > +  bool maybe_global = ref_may_alias_global_p (ref, false);
> >  
> >    if (by_clobber_p)
> >      *by_clobber_p = true;
> > @@ -1038,6 +1039,11 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> >  		      last_phi_def = as_a <gphi *> (use_stmt);
> >  		    }
> >  		}
> > +	      /* If the stmt can throw externally and the store is
> > +		 visible in the context unwound to the store is live.  */
> > +	      else if (maybe_global
> > +		       && stmt_can_throw_external (cfun, use_stmt))
> > +		return DSE_STORE_LIVE;
> >  	      /* If the statement is a use the store is not dead.  */
> >  	      else if (ref_maybe_used_by_stmt_p (use_stmt, ref))
> >  		{
> > @@ -1116,7 +1122,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> >  	 just pretend the stmt makes itself dead.  Otherwise fail.  */
> >        if (defs.is_empty ())
> >  	{
> > -	  if (ref_may_alias_global_p (ref, false))
> > +	  if (maybe_global)
> >  	    return DSE_STORE_LIVE;
> >  
> >  	  if (by_clobber_p)
> > -- 
> > 2.35.3
>
  

> On Tue, 17 Jan 2023, Jan Hubicka wrote:
> 
> > > The following fixes a long-standing bug with DSE removing stores as
> > > dead even though they are live across non-call exceptional flow.
> > > This affects both GIMPLE and RTL DSE and the fix is similar in
> > > making externally throwing statements uses of non-local stores.
> > > Note this doesn't fix the GIMPLE side when the throwing statement
> > > does not involve a load or a store because then the statement does
> > > not have virtual operands and thus is not visited by GIMPLE DSE.
> > 
> > Thanks for looking into this.
> > My main motivation for poking on this is the patch to add fnspec to
> > throw/catch machinery
> > https://gcc.gnu.org/pipermail/gcc-patches/2022-June/597124.html
> > 
> > The eh6.C testcase is misoptimized by current trun with the patch.
> > I think I can adjust it for the throwing function to have no vops and it
> > will still get misoptimized by DSE.
> 
> I think conceptually the "throw" may not be 'const' - it has to be
> 'pure' at most since the program point the control is transfered to
> can inspect memory.

We don't use same argumentation about other control flow statements.
The following:

fn()
{
  try {
    i_read_no_global_memory ();
  } catch (...)
  {
    reutrn 1;
  }
  return 0;
}

should be detected as const.  Marking throw pure would make fn pure too.

With noncall exceptions a=b/c also can transfer to place that inspect
memory.  We may want all statements with can_throw_extenral to have VUSE
on them (like with return) since they may cause function to return, but
I think fnspec is wrong place to model this.
> > According to compiler explorer testcase:
> > struct a{int a,b,c,d,e;};
> > void
> > test(struct a * __restrict a, struct a *b)
> > {
> >   *a = (struct a){0,1,2,3,4};
> >   *a = *b;
> > }
> > Is compiled correctly by GCC 5.4 and first miscopmiled by 6.1, so I
> > think it is a regression. (For C++ not very important one as
> > -fnon-call-exceptions is not very common for C++)
> 
> Ah, yes - RTL DSE probably is too weak for this and GIMPLE DSE
> didn't handle aggregates well at some point.

Yep, we never handled it really correctly but were weaker on optimizing
and thus also producing wrong code :)

Honza
> 
> Richard.
> 
> > 
> > Honza
> > > 
> > > 	PR middle-end/106075
> > > 	* dse.cc (scan_insn): Consider externally throwing insns
> > > 	to read from not frame based memory.
> > > 	* tree-ssa-dse.cc (dse_classify_store): Consider externally
> > > 	throwing uses to read from global memory.
> > > 
> > > 	* gcc.dg/torture/pr106075-1.c: New testcase.
> > > ---
> > >  gcc/dse.cc                                |  5 ++++
> > >  gcc/testsuite/gcc.dg/torture/pr106075-1.c | 36 +++++++++++++++++++++++
> > >  gcc/tree-ssa-dse.cc                       |  8 ++++-
> > >  3 files changed, 48 insertions(+), 1 deletion(-)
> > >  create mode 100644 gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > 
> > > diff --git a/gcc/dse.cc b/gcc/dse.cc
> > > index a2db8d1cc32..7e258b81f66 100644
> > > --- a/gcc/dse.cc
> > > +++ b/gcc/dse.cc
> > > @@ -2633,6 +2633,11 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
> > >        return;
> > >      }
> > >  
> > > +  /* An externally throwing statement may read any memory that is not
> > > +     relative to the frame.  */
> > > +  if (can_throw_external (insn))
> > > +    add_non_frame_wild_read (bb_info);
> > > +
> > >    /* Assuming that there are sets in these insns, we cannot delete
> > >       them.  */
> > >    if ((GET_CODE (PATTERN (insn)) == CLOBBER)
> > > diff --git a/gcc/testsuite/gcc.dg/torture/pr106075-1.c b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > new file mode 100644
> > > index 00000000000..b9affbf1082
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > @@ -0,0 +1,36 @@
> > > +/* { dg-do run { target *-*-linux* } } */
> > > +/* { dg-additional-options "-fnon-call-exceptions" } */
> > > +
> > > +#include <unistd.h>
> > > +#include <signal.h>
> > > +#include <stdlib.h>
> > > +
> > > +int a = 1;
> > > +short *b;
> > > +void __attribute__((noipa))
> > > +test()
> > > +{
> > > +  a=12345;
> > > +  *b=0;
> > > +  a=1;
> > > +}
> > > +
> > > +void check (int i)
> > > +{
> > > +  if (a != 12345)
> > > +    abort ();
> > > +  exit (0);
> > > +}
> > > +
> > > +int
> > > +main ()
> > > +{
> > > +  struct sigaction s;
> > > +  sigemptyset (&s.sa_mask);
> > > +  s.sa_handler = check;
> > > +  s.sa_flags = 0;
> > > +  sigaction (SIGSEGV, &s, NULL);
> > > +  test();
> > > +  abort ();
> > > +  return 0;
> > > +}
> > > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc
> > > index 46ab57d5754..b2e2359c3da 100644
> > > --- a/gcc/tree-ssa-dse.cc
> > > +++ b/gcc/tree-ssa-dse.cc
> > > @@ -960,6 +960,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > >    auto_bitmap visited;
> > >    std::unique_ptr<data_reference, void(*)(data_reference_p)>
> > >      dra (nullptr, free_data_ref);
> > > +  bool maybe_global = ref_may_alias_global_p (ref, false);
> > >  
> > >    if (by_clobber_p)
> > >      *by_clobber_p = true;
> > > @@ -1038,6 +1039,11 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > >  		      last_phi_def = as_a <gphi *> (use_stmt);
> > >  		    }
> > >  		}
> > > +	      /* If the stmt can throw externally and the store is
> > > +		 visible in the context unwound to the store is live.  */
> > > +	      else if (maybe_global
> > > +		       && stmt_can_throw_external (cfun, use_stmt))
> > > +		return DSE_STORE_LIVE;
> > >  	      /* If the statement is a use the store is not dead.  */
> > >  	      else if (ref_maybe_used_by_stmt_p (use_stmt, ref))
> > >  		{
> > > @@ -1116,7 +1122,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > >  	 just pretend the stmt makes itself dead.  Otherwise fail.  */
> > >        if (defs.is_empty ())
> > >  	{
> > > -	  if (ref_may_alias_global_p (ref, false))
> > > +	  if (maybe_global)
> > >  	    return DSE_STORE_LIVE;
> > >  
> > >  	  if (by_clobber_p)
> > > -- 
> > > 2.35.3
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> HRB 36809 (AG Nuernberg)
  

On Tue, 17 Jan 2023, Jan Hubicka wrote:

> > On Tue, 17 Jan 2023, Jan Hubicka wrote:
> > 
> > > > The following fixes a long-standing bug with DSE removing stores as
> > > > dead even though they are live across non-call exceptional flow.
> > > > This affects both GIMPLE and RTL DSE and the fix is similar in
> > > > making externally throwing statements uses of non-local stores.
> > > > Note this doesn't fix the GIMPLE side when the throwing statement
> > > > does not involve a load or a store because then the statement does
> > > > not have virtual operands and thus is not visited by GIMPLE DSE.
> > > 
> > > Thanks for looking into this.
> > > My main motivation for poking on this is the patch to add fnspec to
> > > throw/catch machinery
> > > https://gcc.gnu.org/pipermail/gcc-patches/2022-June/597124.html
> > > 
> > > The eh6.C testcase is misoptimized by current trun with the patch.
> > > I think I can adjust it for the throwing function to have no vops and it
> > > will still get misoptimized by DSE.
> > 
> > I think conceptually the "throw" may not be 'const' - it has to be
> > 'pure' at most since the program point the control is transfered to
> > can inspect memory.
> 
> We don't use same argumentation about other control flow statements.
> The following:
> 
> fn()
> {
>   try {
>     i_read_no_global_memory ();
>   } catch (...)
>   {
>     reutrn 1;
>   }
>   return 0;
> }
> 
> should be detected as const.  Marking throw pure would make fn pure too.

I suppose i_read_no_global_memory is const here.  Not sure why that
should make it pure?  Only if anything throws externally (not catched
in fn) should force it to be pure, no?

Of course for IPA purposes whether 'fn' is to be considered const
or pure depends on whether its exceptions are catched in the context
where that's interesting - that is, whether the EH side-effect is
explicitely or implicitely modeled.

> With noncall exceptions a=b/c also can transfer to place that inspect
> memory.  We may want all statements with can_throw_extenral to have VUSE
> on them (like with return) since they may cause function to return, but
> I think fnspec is wrong place to model this.

Yes, I think all control transfer instructions need a VUSE.

Richard.

> > > According to compiler explorer testcase:
> > > struct a{int a,b,c,d,e;};
> > > void
> > > test(struct a * __restrict a, struct a *b)
> > > {
> > >   *a = (struct a){0,1,2,3,4};
> > >   *a = *b;
> > > }
> > > Is compiled correctly by GCC 5.4 and first miscopmiled by 6.1, so I
> > > think it is a regression. (For C++ not very important one as
> > > -fnon-call-exceptions is not very common for C++)
> > 
> > Ah, yes - RTL DSE probably is too weak for this and GIMPLE DSE
> > didn't handle aggregates well at some point.
> 
> Yep, we never handled it really correctly but were weaker on optimizing
> and thus also producing wrong code :)
> 
> Honza
> > 
> > Richard.
> > 
> > > 
> > > Honza
> > > > 
> > > > 	PR middle-end/106075
> > > > 	* dse.cc (scan_insn): Consider externally throwing insns
> > > > 	to read from not frame based memory.
> > > > 	* tree-ssa-dse.cc (dse_classify_store): Consider externally
> > > > 	throwing uses to read from global memory.
> > > > 
> > > > 	* gcc.dg/torture/pr106075-1.c: New testcase.
> > > > ---
> > > >  gcc/dse.cc                                |  5 ++++
> > > >  gcc/testsuite/gcc.dg/torture/pr106075-1.c | 36 +++++++++++++++++++++++
> > > >  gcc/tree-ssa-dse.cc                       |  8 ++++-
> > > >  3 files changed, 48 insertions(+), 1 deletion(-)
> > > >  create mode 100644 gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > > 
> > > > diff --git a/gcc/dse.cc b/gcc/dse.cc
> > > > index a2db8d1cc32..7e258b81f66 100644
> > > > --- a/gcc/dse.cc
> > > > +++ b/gcc/dse.cc
> > > > @@ -2633,6 +2633,11 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
> > > >        return;
> > > >      }
> > > >  
> > > > +  /* An externally throwing statement may read any memory that is not
> > > > +     relative to the frame.  */
> > > > +  if (can_throw_external (insn))
> > > > +    add_non_frame_wild_read (bb_info);
> > > > +
> > > >    /* Assuming that there are sets in these insns, we cannot delete
> > > >       them.  */
> > > >    if ((GET_CODE (PATTERN (insn)) == CLOBBER)
> > > > diff --git a/gcc/testsuite/gcc.dg/torture/pr106075-1.c b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > > new file mode 100644
> > > > index 00000000000..b9affbf1082
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > > @@ -0,0 +1,36 @@
> > > > +/* { dg-do run { target *-*-linux* } } */
> > > > +/* { dg-additional-options "-fnon-call-exceptions" } */
> > > > +
> > > > +#include <unistd.h>
> > > > +#include <signal.h>
> > > > +#include <stdlib.h>
> > > > +
> > > > +int a = 1;
> > > > +short *b;
> > > > +void __attribute__((noipa))
> > > > +test()
> > > > +{
> > > > +  a=12345;
> > > > +  *b=0;
> > > > +  a=1;
> > > > +}
> > > > +
> > > > +void check (int i)
> > > > +{
> > > > +  if (a != 12345)
> > > > +    abort ();
> > > > +  exit (0);
> > > > +}
> > > > +
> > > > +int
> > > > +main ()
> > > > +{
> > > > +  struct sigaction s;
> > > > +  sigemptyset (&s.sa_mask);
> > > > +  s.sa_handler = check;
> > > > +  s.sa_flags = 0;
> > > > +  sigaction (SIGSEGV, &s, NULL);
> > > > +  test();
> > > > +  abort ();
> > > > +  return 0;
> > > > +}
> > > > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc
> > > > index 46ab57d5754..b2e2359c3da 100644
> > > > --- a/gcc/tree-ssa-dse.cc
> > > > +++ b/gcc/tree-ssa-dse.cc
> > > > @@ -960,6 +960,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > >    auto_bitmap visited;
> > > >    std::unique_ptr<data_reference, void(*)(data_reference_p)>
> > > >      dra (nullptr, free_data_ref);
> > > > +  bool maybe_global = ref_may_alias_global_p (ref, false);
> > > >  
> > > >    if (by_clobber_p)
> > > >      *by_clobber_p = true;
> > > > @@ -1038,6 +1039,11 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > >  		      last_phi_def = as_a <gphi *> (use_stmt);
> > > >  		    }
> > > >  		}
> > > > +	      /* If the stmt can throw externally and the store is
> > > > +		 visible in the context unwound to the store is live.  */
> > > > +	      else if (maybe_global
> > > > +		       && stmt_can_throw_external (cfun, use_stmt))
> > > > +		return DSE_STORE_LIVE;
> > > >  	      /* If the statement is a use the store is not dead.  */
> > > >  	      else if (ref_maybe_used_by_stmt_p (use_stmt, ref))
> > > >  		{
> > > > @@ -1116,7 +1122,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > >  	 just pretend the stmt makes itself dead.  Otherwise fail.  */
> > > >        if (defs.is_empty ())
> > > >  	{
> > > > -	  if (ref_may_alias_global_p (ref, false))
> > > > +	  if (maybe_global)
> > > >  	    return DSE_STORE_LIVE;
> > > >  
> > > >  	  if (by_clobber_p)
> > > > -- 
> > > > 2.35.3
> > > 
> > 
> > -- 
> > Richard Biener <rguenther@suse.de>
> > SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> > Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> > HRB 36809 (AG Nuernberg)
>
  

> > We don't use same argumentation about other control flow statements.
> > The following:
> > 
> > fn()
> > {
> >   try {
> >     i_read_no_global_memory ();
> >   } catch (...)
> >   {
> >     reutrn 1;
> >   }
> >   return 0;
> > }
> > 
> > should be detected as const.  Marking throw pure would make fn pure too.
> 
> I suppose i_read_no_global_memory is const here.  Not sure why that
Suppose we have:

void
i_read_no_global_memory ()
{
  throw(0);
}

If cxa_throw itself was annotated as 'p' rahter than 'c' ipa-modref will
believe that cxa_throw will read any global memory and will propagate it
to all callers. So fn() will be also marked as reading all global
memory.

> should make it pure?  Only if anything throws externally (not catched
> in fn) should force it to be pure, no?
> 
> Of course for IPA purposes whether 'fn' is to be considered const
> or pure depends on whether its exceptions are catched in the context
> where that's interesting - that is, whether the EH side-effect is
> explicitely or implicitely modeled.

We have two things here. const/pure attributes 'c'/'p' fnspec
specifiers.  const/pure implies that function call can be removed when
result is not necessary. This is not the case of funcitons calling
throw() (we have -fdelete-dead-exceptions for noncall exceptions and
those would be OK).  However 'c'/'p' is about memory side effects only
and it is safe for i_read_no_global_memory.

With the C++ FE change adding fnspec to EH handling modref will detect
both i_read_no_global_memory and fn() as 'c'. It won't infer const
attribute that is something I can implement later.
We are very poor on detecting scenarios where all exceptions thrown are
actually caught. It is long time on my TODO to fix that, so probably
next stage1 is time to look into that.

> 
> > With noncall exceptions a=b/c also can transfer to place that inspect
> > memory.  We may want all statements with can_throw_extenral to have VUSE
> > on them (like with return) since they may cause function to return, but
> > I think fnspec is wrong place to model this.
> 
> Yes, I think all control transfer instructions need a VUSE.

I think it is right way to go.  So operands_scanner::parse_ssa_operands
can add vuse to anything that can_throw_external_p (like it does for
GIMPLE_RETURN) and passes like DSE can test for it and understand that
on the EH path the globally accessible memory is live and thus "used" by
the statement.

I can try to cook up a patch.

Thanks,
Honza
> 
> Richard.
> 
> > > > According to compiler explorer testcase:
> > > > struct a{int a,b,c,d,e;};
> > > > void
> > > > test(struct a * __restrict a, struct a *b)
> > > > {
> > > >   *a = (struct a){0,1,2,3,4};
> > > >   *a = *b;
> > > > }
> > > > Is compiled correctly by GCC 5.4 and first miscopmiled by 6.1, so I
> > > > think it is a regression. (For C++ not very important one as
> > > > -fnon-call-exceptions is not very common for C++)
> > > 
> > > Ah, yes - RTL DSE probably is too weak for this and GIMPLE DSE
> > > didn't handle aggregates well at some point.
> > 
> > Yep, we never handled it really correctly but were weaker on optimizing
> > and thus also producing wrong code :)
> > 
> > Honza
> > > 
> > > Richard.
> > > 
> > > > 
> > > > Honza
> > > > > 
> > > > > 	PR middle-end/106075
> > > > > 	* dse.cc (scan_insn): Consider externally throwing insns
> > > > > 	to read from not frame based memory.
> > > > > 	* tree-ssa-dse.cc (dse_classify_store): Consider externally
> > > > > 	throwing uses to read from global memory.
> > > > > 
> > > > > 	* gcc.dg/torture/pr106075-1.c: New testcase.
> > > > > ---
> > > > >  gcc/dse.cc                                |  5 ++++
> > > > >  gcc/testsuite/gcc.dg/torture/pr106075-1.c | 36 +++++++++++++++++++++++
> > > > >  gcc/tree-ssa-dse.cc                       |  8 ++++-
> > > > >  3 files changed, 48 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > > > 
> > > > > diff --git a/gcc/dse.cc b/gcc/dse.cc
> > > > > index a2db8d1cc32..7e258b81f66 100644
> > > > > --- a/gcc/dse.cc
> > > > > +++ b/gcc/dse.cc
> > > > > @@ -2633,6 +2633,11 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
> > > > >        return;
> > > > >      }
> > > > >  
> > > > > +  /* An externally throwing statement may read any memory that is not
> > > > > +     relative to the frame.  */
> > > > > +  if (can_throw_external (insn))
> > > > > +    add_non_frame_wild_read (bb_info);
> > > > > +
> > > > >    /* Assuming that there are sets in these insns, we cannot delete
> > > > >       them.  */
> > > > >    if ((GET_CODE (PATTERN (insn)) == CLOBBER)
> > > > > diff --git a/gcc/testsuite/gcc.dg/torture/pr106075-1.c b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > > > new file mode 100644
> > > > > index 00000000000..b9affbf1082
> > > > > --- /dev/null
> > > > > +++ b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > > > @@ -0,0 +1,36 @@
> > > > > +/* { dg-do run { target *-*-linux* } } */
> > > > > +/* { dg-additional-options "-fnon-call-exceptions" } */
> > > > > +
> > > > > +#include <unistd.h>
> > > > > +#include <signal.h>
> > > > > +#include <stdlib.h>
> > > > > +
> > > > > +int a = 1;
> > > > > +short *b;
> > > > > +void __attribute__((noipa))
> > > > > +test()
> > > > > +{
> > > > > +  a=12345;
> > > > > +  *b=0;
> > > > > +  a=1;
> > > > > +}
> > > > > +
> > > > > +void check (int i)
> > > > > +{
> > > > > +  if (a != 12345)
> > > > > +    abort ();
> > > > > +  exit (0);
> > > > > +}
> > > > > +
> > > > > +int
> > > > > +main ()
> > > > > +{
> > > > > +  struct sigaction s;
> > > > > +  sigemptyset (&s.sa_mask);
> > > > > +  s.sa_handler = check;
> > > > > +  s.sa_flags = 0;
> > > > > +  sigaction (SIGSEGV, &s, NULL);
> > > > > +  test();
> > > > > +  abort ();
> > > > > +  return 0;
> > > > > +}
> > > > > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc
> > > > > index 46ab57d5754..b2e2359c3da 100644
> > > > > --- a/gcc/tree-ssa-dse.cc
> > > > > +++ b/gcc/tree-ssa-dse.cc
> > > > > @@ -960,6 +960,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > > >    auto_bitmap visited;
> > > > >    std::unique_ptr<data_reference, void(*)(data_reference_p)>
> > > > >      dra (nullptr, free_data_ref);
> > > > > +  bool maybe_global = ref_may_alias_global_p (ref, false);
> > > > >  
> > > > >    if (by_clobber_p)
> > > > >      *by_clobber_p = true;
> > > > > @@ -1038,6 +1039,11 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > > >  		      last_phi_def = as_a <gphi *> (use_stmt);
> > > > >  		    }
> > > > >  		}
> > > > > +	      /* If the stmt can throw externally and the store is
> > > > > +		 visible in the context unwound to the store is live.  */
> > > > > +	      else if (maybe_global
> > > > > +		       && stmt_can_throw_external (cfun, use_stmt))
> > > > > +		return DSE_STORE_LIVE;
> > > > >  	      /* If the statement is a use the store is not dead.  */
> > > > >  	      else if (ref_maybe_used_by_stmt_p (use_stmt, ref))
> > > > >  		{
> > > > > @@ -1116,7 +1122,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > > >  	 just pretend the stmt makes itself dead.  Otherwise fail.  */
> > > > >        if (defs.is_empty ())
> > > > >  	{
> > > > > -	  if (ref_may_alias_global_p (ref, false))
> > > > > +	  if (maybe_global)
> > > > >  	    return DSE_STORE_LIVE;
> > > > >  
> > > > >  	  if (by_clobber_p)
> > > > > -- 
> > > > > 2.35.3
> > > > 
> > > 
> > > -- 
> > > Richard Biener <rguenther@suse.de>
> > > SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> > > Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> > > HRB 36809 (AG Nuernberg)
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> HRB 36809 (AG Nuernberg)
  

On Tue, 17 Jan 2023, Jan Hubicka wrote:

> > > We don't use same argumentation about other control flow statements.
> > > The following:
> > > 
> > > fn()
> > > {
> > >   try {
> > >     i_read_no_global_memory ();
> > >   } catch (...)
> > >   {
> > >     reutrn 1;
> > >   }
> > >   return 0;
> > > }
> > > 
> > > should be detected as const.  Marking throw pure would make fn pure too.
> > 
> > I suppose i_read_no_global_memory is const here.  Not sure why that
> Suppose we have:
> 
> void
> i_read_no_global_memory ()
> {
>   throw(0);
> }
> 
> If cxa_throw itself was annotated as 'p' rahter than 'c' ipa-modref will
> believe that cxa_throw will read any global memory and will propagate it
> to all callers. So fn() will be also marked as reading all global
> memory.

Sure - but for the purpose of local optimizations in 
i_read_no_global_memory cxa_throw has to appear to read memory.
Having a VUSE there dependent on whether the function performs any
load or store would be quite ugly.  Instead modref could special-case
cxa_throw and not treat it as reading memory (like it already does
for the return stmt I suppose - that also has a VUSE).

> > should make it pure?  Only if anything throws externally (not catched
> > in fn) should force it to be pure, no?
> > 
> > Of course for IPA purposes whether 'fn' is to be considered const
> > or pure depends on whether its exceptions are catched in the context
> > where that's interesting - that is, whether the EH side-effect is
> > explicitely or implicitely modeled.
> 
> We have two things here. const/pure attributes 'c'/'p' fnspec
> specifiers.  const/pure implies that function call can be removed when
> result is not necessary. This is not the case of funcitons calling
> throw() (we have -fdelete-dead-exceptions for noncall exceptions and
> those would be OK).  However 'c'/'p' is about memory side effects only
> and it is safe for i_read_no_global_memory.
> 
> With the C++ FE change adding fnspec to EH handling modref will detect
> both i_read_no_global_memory and fn() as 'c'. It won't infer const
> attribute that is something I can implement later.
> We are very poor on detecting scenarios where all exceptions thrown are
> actually caught. It is long time on my TODO to fix that, so probably
> next stage1 is time to look into that.
> 
> > 
> > > With noncall exceptions a=b/c also can transfer to place that inspect
> > > memory.  We may want all statements with can_throw_extenral to have VUSE
> > > on them (like with return) since they may cause function to return, but
> > > I think fnspec is wrong place to model this.
> > 
> > Yes, I think all control transfer instructions need a VUSE.
> 
> I think it is right way to go.  So operands_scanner::parse_ssa_operands
> can add vuse to anything that can_throw_external_p (like it does for
> GIMPLE_RETURN) and passes like DSE can test for it and understand that
> on the EH path the globally accessible memory is live and thus "used" by
> the statement.
>
> I can try to cook up a patch.

The problem is IIRC GIMPLE_RESX which doesn't derive from
gimple_statement_with_memory_ops_base.  There's a bugzilla I can't find
right now refering to this issue.

Richard.

> Thanks,
> Honza
> > 
> > Richard.
> > 
> > > > > According to compiler explorer testcase:
> > > > > struct a{int a,b,c,d,e;};
> > > > > void
> > > > > test(struct a * __restrict a, struct a *b)
> > > > > {
> > > > >   *a = (struct a){0,1,2,3,4};
> > > > >   *a = *b;
> > > > > }
> > > > > Is compiled correctly by GCC 5.4 and first miscopmiled by 6.1, so I
> > > > > think it is a regression. (For C++ not very important one as
> > > > > -fnon-call-exceptions is not very common for C++)
> > > > 
> > > > Ah, yes - RTL DSE probably is too weak for this and GIMPLE DSE
> > > > didn't handle aggregates well at some point.
> > > 
> > > Yep, we never handled it really correctly but were weaker on optimizing
> > > and thus also producing wrong code :)
> > > 
> > > Honza
> > > > 
> > > > Richard.
> > > > 
> > > > > 
> > > > > Honza
> > > > > > 
> > > > > > 	PR middle-end/106075
> > > > > > 	* dse.cc (scan_insn): Consider externally throwing insns
> > > > > > 	to read from not frame based memory.
> > > > > > 	* tree-ssa-dse.cc (dse_classify_store): Consider externally
> > > > > > 	throwing uses to read from global memory.
> > > > > > 
> > > > > > 	* gcc.dg/torture/pr106075-1.c: New testcase.
> > > > > > ---
> > > > > >  gcc/dse.cc                                |  5 ++++
> > > > > >  gcc/testsuite/gcc.dg/torture/pr106075-1.c | 36 +++++++++++++++++++++++
> > > > > >  gcc/tree-ssa-dse.cc                       |  8 ++++-
> > > > > >  3 files changed, 48 insertions(+), 1 deletion(-)
> > > > > >  create mode 100644 gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > > > > 
> > > > > > diff --git a/gcc/dse.cc b/gcc/dse.cc
> > > > > > index a2db8d1cc32..7e258b81f66 100644
> > > > > > --- a/gcc/dse.cc
> > > > > > +++ b/gcc/dse.cc
> > > > > > @@ -2633,6 +2633,11 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores)
> > > > > >        return;
> > > > > >      }
> > > > > >  
> > > > > > +  /* An externally throwing statement may read any memory that is not
> > > > > > +     relative to the frame.  */
> > > > > > +  if (can_throw_external (insn))
> > > > > > +    add_non_frame_wild_read (bb_info);
> > > > > > +
> > > > > >    /* Assuming that there are sets in these insns, we cannot delete
> > > > > >       them.  */
> > > > > >    if ((GET_CODE (PATTERN (insn)) == CLOBBER)
> > > > > > diff --git a/gcc/testsuite/gcc.dg/torture/pr106075-1.c b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > > > > new file mode 100644
> > > > > > index 00000000000..b9affbf1082
> > > > > > --- /dev/null
> > > > > > +++ b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > > > > @@ -0,0 +1,36 @@
> > > > > > +/* { dg-do run { target *-*-linux* } } */
> > > > > > +/* { dg-additional-options "-fnon-call-exceptions" } */
> > > > > > +
> > > > > > +#include <unistd.h>
> > > > > > +#include <signal.h>
> > > > > > +#include <stdlib.h>
> > > > > > +
> > > > > > +int a = 1;
> > > > > > +short *b;
> > > > > > +void __attribute__((noipa))
> > > > > > +test()
> > > > > > +{
> > > > > > +  a=12345;
> > > > > > +  *b=0;
> > > > > > +  a=1;
> > > > > > +}
> > > > > > +
> > > > > > +void check (int i)
> > > > > > +{
> > > > > > +  if (a != 12345)
> > > > > > +    abort ();
> > > > > > +  exit (0);
> > > > > > +}
> > > > > > +
> > > > > > +int
> > > > > > +main ()
> > > > > > +{
> > > > > > +  struct sigaction s;
> > > > > > +  sigemptyset (&s.sa_mask);
> > > > > > +  s.sa_handler = check;
> > > > > > +  s.sa_flags = 0;
> > > > > > +  sigaction (SIGSEGV, &s, NULL);
> > > > > > +  test();
> > > > > > +  abort ();
> > > > > > +  return 0;
> > > > > > +}
> > > > > > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc
> > > > > > index 46ab57d5754..b2e2359c3da 100644
> > > > > > --- a/gcc/tree-ssa-dse.cc
> > > > > > +++ b/gcc/tree-ssa-dse.cc
> > > > > > @@ -960,6 +960,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > > > >    auto_bitmap visited;
> > > > > >    std::unique_ptr<data_reference, void(*)(data_reference_p)>
> > > > > >      dra (nullptr, free_data_ref);
> > > > > > +  bool maybe_global = ref_may_alias_global_p (ref, false);
> > > > > >  
> > > > > >    if (by_clobber_p)
> > > > > >      *by_clobber_p = true;
> > > > > > @@ -1038,6 +1039,11 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > > > >  		      last_phi_def = as_a <gphi *> (use_stmt);
> > > > > >  		    }
> > > > > >  		}
> > > > > > +	      /* If the stmt can throw externally and the store is
> > > > > > +		 visible in the context unwound to the store is live.  */
> > > > > > +	      else if (maybe_global
> > > > > > +		       && stmt_can_throw_external (cfun, use_stmt))
> > > > > > +		return DSE_STORE_LIVE;
> > > > > >  	      /* If the statement is a use the store is not dead.  */
> > > > > >  	      else if (ref_maybe_used_by_stmt_p (use_stmt, ref))
> > > > > >  		{
> > > > > > @@ -1116,7 +1122,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > > > >  	 just pretend the stmt makes itself dead.  Otherwise fail.  */
> > > > > >        if (defs.is_empty ())
> > > > > >  	{
> > > > > > -	  if (ref_may_alias_global_p (ref, false))
> > > > > > +	  if (maybe_global)
> > > > > >  	    return DSE_STORE_LIVE;
> > > > > >  
> > > > > >  	  if (by_clobber_p)
> > > > > > -- 
> > > > > > 2.35.3
> > > > > 
> > > > 
> > > > -- 
> > > > Richard Biener <rguenther@suse.de>
> > > > SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> > > > Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> > > > HRB 36809 (AG Nuernberg)
> > > 
> > 
> > -- 
> > Richard Biener <rguenther@suse.de>
> > SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> > Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> > HRB 36809 (AG Nuernberg)
>
  

> On Tue, 17 Jan 2023, Jan Hubicka wrote:
> 
> > > > We don't use same argumentation about other control flow statements.
> > > > The following:
> > > > 
> > > > fn()
> > > > {
> > > >   try {
> > > >     i_read_no_global_memory ();
> > > >   } catch (...)
> > > >   {
> > > >     reutrn 1;
> > > >   }
> > > >   return 0;
> > > > }
> > > > 
> > > > should be detected as const.  Marking throw pure would make fn pure too.
> > > 
> > > I suppose i_read_no_global_memory is const here.  Not sure why that
> > Suppose we have:
> > 
> > void
> > i_read_no_global_memory ()
> > {
> >   throw(0);
> > }
> > 
> > If cxa_throw itself was annotated as 'p' rahter than 'c' ipa-modref will
> > believe that cxa_throw will read any global memory and will propagate it
> > to all callers. So fn() will be also marked as reading all global
> > memory.
> 
> Sure - but for the purpose of local optimizations in 
> i_read_no_global_memory cxa_throw has to appear to read memory.

Yes, I think every stmt that can throw externally need VUSE (just like
return_stmt needs it).  Even if throw(0) was replaced by a=b/c with
-fnon-call-exceptions.  It is still not clear to me why this should
imply that we need 'p' instead of 'c' in fnspecs.

So I think we should try to make the following to work:

diff --git a/gcc/tree-ssa-operands.cc b/gcc/tree-ssa-operands.cc
index 57e393ae164..d24f1721eb2 100644
--- a/gcc/tree-ssa-operands.cc
+++ b/gcc/tree-ssa-operands.cc
@@ -951,6 +951,9 @@ operands_scanner::parse_ssa_operands ()
   enum gimple_code code = gimple_code (stmt);
   size_t i, n, start = 0;
 
+  if (stmt_can_throw_external (fn, stmt))
+    append_vuse (gimple_vop (fn));
+
   switch (code)
     {
     case GIMPLE_ASM:

> Having a VUSE there dependent on whether the function performs any
> load or store would be quite ugly.  Instead modref could special-case
> cxa_throw and not treat it as reading memory (like it already does
> for the return stmt I suppose - that also has a VUSE).

modref looks into statements with VUSEs on them and checks what
reads/stores are done.  So return statement with VUSE is walked and no
load is recorded because no actual load is found.
Similarly that would happen with __cxa_throw if it was 'c'.
With 'p' it has nothing to analyze so it would trust the fact that
cxa_throw itself reads some global state.
> 
> The problem is IIRC GIMPLE_RESX which doesn't derive from
> gimple_statement_with_memory_ops_base.  There's a bugzilla I can't find
> right now refering to this issue.

I never tried to play with gimple hiearchy. It is hard to fix resx?  I
wonder if we have other cases.  I guess for a=b/c we are luck just
because gimple_assign can also be load or store so it has memory_ops...

Thanks,
Honza
  

On Wed, 18 Jan 2023, Jan Hubicka wrote:

> > On Tue, 17 Jan 2023, Jan Hubicka wrote:
> > 
> > > > > We don't use same argumentation about other control flow statements.
> > > > > The following:
> > > > > 
> > > > > fn()
> > > > > {
> > > > >   try {
> > > > >     i_read_no_global_memory ();
> > > > >   } catch (...)
> > > > >   {
> > > > >     reutrn 1;
> > > > >   }
> > > > >   return 0;
> > > > > }
> > > > > 
> > > > > should be detected as const.  Marking throw pure would make fn pure too.
> > > > 
> > > > I suppose i_read_no_global_memory is const here.  Not sure why that
> > > Suppose we have:
> > > 
> > > void
> > > i_read_no_global_memory ()
> > > {
> > >   throw(0);
> > > }
> > > 
> > > If cxa_throw itself was annotated as 'p' rahter than 'c' ipa-modref will
> > > believe that cxa_throw will read any global memory and will propagate it
> > > to all callers. So fn() will be also marked as reading all global
> > > memory.
> > 
> > Sure - but for the purpose of local optimizations in 
> > i_read_no_global_memory cxa_throw has to appear to read memory.
> 
> Yes, I think every stmt that can throw externally need VUSE (just like
> return_stmt needs it).  Even if throw(0) was replaced by a=b/c with
> -fnon-call-exceptions.  It is still not clear to me why this should
> imply that we need 'p' instead of 'c' in fnspecs.
> 
> So I think we should try to make the following to work:
> 
> diff --git a/gcc/tree-ssa-operands.cc b/gcc/tree-ssa-operands.cc
> index 57e393ae164..d24f1721eb2 100644
> --- a/gcc/tree-ssa-operands.cc
> +++ b/gcc/tree-ssa-operands.cc
> @@ -951,6 +951,9 @@ operands_scanner::parse_ssa_operands ()
>    enum gimple_code code = gimple_code (stmt);
>    size_t i, n, start = 0;
>  
> +  if (stmt_can_throw_external (fn, stmt))
> +    append_vuse (gimple_vop (fn));
> +
>    switch (code)
>      {
>      case GIMPLE_ASM:

It's going to be a bit tricky since in many places we use
gimple_vuse () != NULL to check whether an assignment is a
load/store.  But yes, the above is sort-of what we'd need to do.

> > Having a VUSE there dependent on whether the function performs any
> > load or store would be quite ugly.  Instead modref could special-case
> > cxa_throw and not treat it as reading memory (like it already does
> > for the return stmt I suppose - that also has a VUSE).
> 
> modref looks into statements with VUSEs on them and checks what
> reads/stores are done.  So return statement with VUSE is walked and no
> load is recorded because no actual load is found.
> Similarly that would happen with __cxa_throw if it was 'c'.
> With 'p' it has nothing to analyze so it would trust the fact that
> cxa_throw itself reads some global state.

I see.  But does __cxa_throw stmt_can_throw_external ()?  Otherwise
the operand scanner elides VUSE on const function calls.

> > 
> > The problem is IIRC GIMPLE_RESX which doesn't derive from
> > gimple_statement_with_memory_ops_base.  There's a bugzilla I can't find
> > right now refering to this issue.
> 
> I never tried to play with gimple hiearchy. It is hard to fix resx?  I
> wonder if we have other cases.  I guess for a=b/c we are luck just
> because gimple_assign can also be load or store so it has memory_ops...

Fixing resx would come at the cost of deriving from _with_ops, but not
sure if that waste of space is too important.

Richard.

> Thanks,
> Honza
>