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

[Jan Hubicka <hubicka@ucw.cz>]

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