Re: [PATCH] middle-end/106075 - non-call EH and DSE

[Richard Biener <rguenther@suse.de>]

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
> 

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